eclipse / tinydtls

Eclipse tinydtls
https://projects.eclipse.org/projects/iot.tinydtls
Other
105 stars 57 forks source link

Rfc5746 #181

Closed boaks closed 1 year ago

boaks commented 1 year ago

Issue #175

boaks commented 1 year ago

The client always include the TLS_EMPTY_RENEGOTIATION_INFO_SCSV and the server answers with an empty renegotiation info, if either the TLS_EMPTY_RENEGOTIATION_INFO_SCSV or a empty renegotiation info is contained in the Client_Hello. That enables to execute handshakes with implementations, which requires RFC5746 support.

For now, it's optional in tinydtls, if the other peer supports it.

I will create a second PR, which adapts the get_cipher_suites into a get_dtls_paramaterand a specific struct dtls_user_parameter and add a flags "renegotiation_info_required" there.

boaks commented 1 year ago

@mrdeep1

If you find the time, you may test it against OpenSsl 3.1.

mrdeep1 commented 1 year ago

Will do - it will be later today

mrdeep1 commented 1 year ago

Unfortunately, it fails to match a cipher suite and so gives up. Using original c84e36fbuild for TinyDTLS, all works. It makes no difference if OpenSSL (with or without the fix in place) or MbedTLS is used as a client for the test. However GnuTLS does work - hmm.

test.zip has a pcap and log file of the failure.

mrdeep1 commented 1 year ago

With this fix, things progress, but a bad server_hello (not server_hello_done) is reported by the client which then raises an alert.

diff --git a/dtls.c b/dtls.c
index 8e8dc13..9bde81a 100644
--- a/dtls.c
+++ b/dtls.c
@@ -1259,7 +1259,7 @@ dtls_update_parameters(dtls_context_t *ctx,
       config->renegotiation_info = 1;
     } else {
       config->cipher_index = get_cipher_index(config->cipher_suites, dtls_uint16_to_int(data));
-      ok = known_cipher(ctx, config->cipher_index, 0);
+      ok |= known_cipher(ctx, config->cipher_index, 0);
     }
     i -= sizeof(uint16);
     data += sizeof(uint16);

See logs test1.txt

mrdeep1 commented 1 year ago

Seems to be working with this:-

diff --git a/dtls.c b/dtls.c
index 8e8dc13..ef98ecb 100644
--- a/dtls.c
+++ b/dtls.c
@@ -1257,7 +1257,7 @@ dtls_update_parameters(dtls_context_t *ctx,
   while ((i >= (int)sizeof(uint16)) && (!ok || !config->renegotiation_info)) {
     if (dtls_uint16_to_int(data) == TLS_EMPTY_RENEGOTIATION_INFO_SCSV) {
       config->renegotiation_info = 1;
-    } else {
+    } else if (!ok) {
       config->cipher_index = get_cipher_index(config->cipher_suites, dtls_uint16_to_int(data));
       ok = known_cipher(ctx, config->cipher_index, 0);
     }
mrdeep1 commented 1 year ago

With 84f834a libcoap regression tests work for when SSL_CTX_set_options(context->dtls.ctx, SSL_OP_LEGACY_SERVER_CONNECT ) is set or not for OpenSSL. So, this LGTM.

boaks commented 1 year ago

Thanks for testing!

The current plan is to have a final review and merge the PR chain "step by step", starting with PR #147 and #148. Depending on the time needed for #147, we may merged that first to "develop".