eduardsui / tlse

Single C file TLS 1.2/1.3 implementation, using tomcrypt as crypto library
Other
545 stars 90 forks source link

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 11 months ago

lizelglg commented 11 months ago

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.

https://github.com/eduardsui/tlse/blob/687c75de82e3c826ddb2464df2b12686f5c6f0d1/tlse.c#L6795 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);

https://github.com/eduardsui/tlse/blob/687c75de82e3c826ddb2464df2b12686f5c6f0d1/tlse.c#L6855 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: https://github.com/eduardsui/tlse/blob/687c75de82e3c826ddb2464df2b12686f5c6f0d1/tlse.c#L1813

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

eduardsui commented 9 months ago

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

sjaeckel commented 9 months ago

Ah, that's cool!

mundusnine commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

As already mentioned https://github.com/eduardsui/tlse/issues/78#issuecomment-1415900688 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 :)