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

Rfc5746 #181

Merged
merged 2 commits into from
Jul 27, 2023
Merged

Rfc5746 #181

merged 2 commits into from
Jul 27, 2023

Conversation

boaks
Copy link
Contributor

@boaks boaks commented Nov 29, 2022

Issue #175

@boaks
Copy link
Contributor Author

boaks commented Nov 29, 2022

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
Copy link
Contributor Author

boaks commented Nov 29, 2022

@mrdeep1

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

@mrdeep1
Copy link
Contributor

mrdeep1 commented Nov 29, 2022

Will do - it will be later today

@mrdeep1
Copy link
Contributor

mrdeep1 commented Nov 29, 2022

Unfortunately, it fails to match a cipher suite and so gives up. Using original c84e36f build 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
Copy link
Contributor

mrdeep1 commented Nov 29, 2022

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
Copy link
Contributor

mrdeep1 commented Nov 29, 2022

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);
     }

@boaks boaks force-pushed the rfc5746 branch 3 times, most recently from 2c41e13 to bcbcfcd Compare November 29, 2022 17:45
crypto.c Outdated
/* FIXME: we use the default SHA256 here, might need to support other
hash functions as well */
dtls_hash_init(&handshake->hs_state.hs_hash);
if (handshake) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this test as handshake cannot be NULL at this point (line 153).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, "old lines" reintroduced from "old PR" (#147).

Removed.

@boaks boaks force-pushed the rfc5746 branch 3 times, most recently from 6c5c5f9 to 84f834a Compare December 7, 2022 15:22
@mrdeep1
Copy link
Contributor

mrdeep1 commented Dec 10, 2022

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
Copy link
Contributor Author

boaks commented Dec 11, 2022

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".

Supports RFC5746 minimal version without renegotiation.
Add detailed documentation about the message length calculations.
Add TLS_EMPTY_RENEGOTIATION_INFO_SCSV to DTLS_CH_LENGTH_MAX.
Remove eclipse_curves from ServerHello length.

Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
Signed-off-by: Achim Kraus <achim.kraus@cloudcoap.net>
Copy link
Contributor

@obgm obgm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

There is a minor detail that might lead to a buffer overrun in dtls_send_client_hello(). I will open a PR for that right after merging this.

@obgm obgm merged commit b3d8468 into eclipse:main Jul 27, 2023
5 checks passed
@boaks boaks deleted the rfc5746 branch August 7, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants