Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The TLS 1.3 client cannot correctly obtain the server certificate (tls_parse_certificate) and fails to verify the certificate (_private_tls_verify_rsa) #89

Open
lizelglg opened this issue Nov 24, 2023 · 12 comments

Comments

@lizelglg
Copy link

lizelglg commented Nov 24, 2023

Hello, my English is very poor, so everything I say is translated by a machine. I don't know if it can successfully translate my meaning, or if you can understand my translated content.

tlse/tlse.c

Line 6795 in 687c75d

CHECK_SIZE(size_of_all_certificates, buf_len - res, TLS_NEED_MORE_DATA);

I used TLS 1.3 to call "www.binance.com/fapi/v1/time" and found that the certificate retrieval failed during the handshake. I changed it to __CHECK_SIZE(size_of_all_certificates, buf_len - res+1, TLS_NEED_MORE_DATA);

tlse/tlse.c

Line 6855 in 687c75d

remaining -= 2;

I added "res2+=2", there,
and delete
" if ((size) && (size >= remaining)) {
res2 += size;
remaining -= size;
}"

This can correctly obtain the three certificates of Binance, but then there is a problem with "_private_tls_verify_rsa",return 7,there:

tlse/tlse.c

Line 1813 in 687c75d

err = rsa_verify_hash_ex(buffer, len, hash, hash_len, LTC_PKCS_1_PSS, hash_idx, 0, &rsa_stat, &key);

Because I don't understand the TLS protocol, I can only temporarily comment out this function, which allows me to communicate with the server temporarily. I would like to know if the TLS1.3 functionality is not fully implemented in this code. Could you please fix this issue, and also if there are any other areas that could be associated with this issue that need to be fixed?Thank you.

2023/11/25 Additional help:
I have another new question:
I found that there are many static global variables in the source code of tlse.c. If I change them to variables inside functions or put them in the TLSContext, can I ensure that each TLS in multiple threads does not affect each other (because I am worried that the functions in libtomcrypt.c are also not thread-safe)?

@headscott
Copy link

I have/had the same problems. I asked at the Github libtom/libtomcrypt for help, because it failed in their pkcs_1_pss_decode method. They told me that you can remove salt completly, because it is not used. And instead of the call

rsa_verify_hash_ex(buffer, len, hash, hash_len, LTC_PKCS_1_PSS, hash_idx, 0, &rsa_stat, &key);

you should use:

rsa_verify_hash_ex(buffer, len, hash, hash_len, LTC_PKCS_1_PSS, hash_idx, hash_len, &rsa_stat, &key);

Same with:

rsa_sign_hash_ex(hash, hash_len, out, outlen, LTC_PKCS_1_PSS, NULL, find_prng("sprng"), hash_idx, hash_type == sha256 ? 32 : 48, &key);

here you should do:

rsa_sign_hash_ex(hash, hash_len, out, outlen, LTC_PKCS_1_PSS, NULL, find_prng("sprng"), hash_idx, hash_len, &key);

This fixed the problem, that the decode method fails on testing for DB == 0x00. But now it fails in this line:

if (XMEMCMP(mask, hash, hLen) == 0) { *res = 1; }

I am not sure why the hash and the mask are different now. If you have an idea please let me know. I am struggling with that too.

@lizelglg
Copy link
Author

I have/had the same problems. I asked at the Github libtom/libtomcrypt for help, because it failed in their pkcs_1_pss_decode method. They told me that you can remove salt completly, because it is not used. And instead of the call

rsa_verify_hash_ex(buffer, len, hash, hash_len, LTC_PKCS_1_PSS, hash_idx, 0, &rsa_stat, &key);

you should use:

rsa_verify_hash_ex(buffer, len, hash, hash_len, LTC_PKCS_1_PSS, hash_idx, hash_len, &rsa_stat, &key);

Same with:

rsa_sign_hash_ex(hash, hash_len, out, outlen, LTC_PKCS_1_PSS, NULL, find_prng("sprng"), hash_idx, hash_type == sha256 ? 32 : 48, &key);

here you should do:

rsa_sign_hash_ex(hash, hash_len, out, outlen, LTC_PKCS_1_PSS, NULL, find_prng("sprng"), hash_idx, hash_len, &key);

This fixed the problem, that the decode method fails on testing for DB == 0x00. But now it fails in this line:

if (XMEMCMP(mask, hash, hLen) == 0) { *res = 1; }

I am not sure why the hash and the mask are different now. If you have an idea please let me know. I am struggling with that too.

https://github.com/Anthony-Mai/TinyTls/blob/9e04c8eeb767db2fdca6364ec1c17ff149b9b9e8/src/ssl/TinyTls.cpp#L3365C20-L3365C20
Because I don't understand encryption at all, I don't understand the logic of the error.However, I found another source code for TLS 1.3, which works correctly.Due to my technical limitations, it is difficult to understand the differences between the two source codes.All I see is that this source code has a "pubkey" involved in the calculation, but I don't see any equivalent members in the certificate structure in "tlse", I don't understand it at all.
I wonder if you can understand where the difference is

@headscott
Copy link

I found the solution for tls_parse_verify_tls13!!! First, as I already said, you have to change the values in the rsa_sign_hash_ex and rsa_verify_hash_ex calls. After that, you should also add an '!' here:

instead of

if (context->is_server)
        memcpy(signing_data + 64, "TLS 1.3, server CertificateVerify", 33);
    else
        memcpy(signing_data + 64, "TLS 1.3, client CertificateVerify", 33);

you need to do

if (!context->is_server)
        memcpy(signing_data + 64, "TLS 1.3, server CertificateVerify", 33);
    else
        memcpy(signing_data + 64, "TLS 1.3, client CertificateVerify", 33);

These lines just need to be changed inside tls_parse_verify_tls13! For me it just worked well. As a client you have to verify that the server sent its CertificateVerify message.

@eduardsui
Copy link
Owner

Hello!

I've checked your fixes and added to the main branch. I still need to check the rsa_sign_hash_ex, but it seems ok.

Thank you,
Eduard

@headscott
Copy link

Hey,

was something wrong with rsa_sign_hash_ex? Or why did you undo the changes? Or was it just because you still need to check it, but you have it in mind? xD

Thank you too,
Fabian

@eduardsui
Copy link
Owner

The latest version of TLSe works both with the old tomcrypt library and the github master branch. There are minor details that need to be checked and I need some time to study them.

@sjaeckel
Copy link
Collaborator

You should really try to get it running with the develop branch of libtomcrypt, master is pretty old.

@eduardsui
Copy link
Owner

eduardsui commented Feb 15, 2024

@sjaeckel I've meant develop branch :). It already works with the develop branch (CRYPT >= 0x0118).

@sjaeckel
Copy link
Collaborator

Ah, that's cool!

@mundusnine
Copy link

libtomcrypt.c should probably be rebuilt @eduardsui ? Did you have a script to do that or did you do it by hand ? If you don't have a script I could make one for the future.

@eduardsui
Copy link
Owner

Yes, I have a script but is written for tomcrypt 0x0117 - it should work as it is only concatenating all the sources in a single C file and replaces the "#include" directives. I will test it in about two weeks. I think I should add the script to the tomcrypt repository.

@sjaeckel
Copy link
Collaborator

As already mentioned #78 (comment) I've also started to work on that as well, but that's not in a state that is acceptable to be merged.

Feel free to provide your way :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants