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

OpenSSL 3.1 fails to connect to TinyDTLS server #175

Closed
mrdeep1 opened this issue Oct 31, 2022 · 23 comments
Closed

OpenSSL 3.1 fails to connect to TinyDTLS server #175

mrdeep1 opened this issue Oct 31, 2022 · 23 comments
Labels
please close Please verify, that the issue is solved and close it.

Comments

@mrdeep1
Copy link
Contributor

mrdeep1 commented Oct 31, 2022

With the server running as

tests/dtls-server -v999 -p 4433 -A 127.0.0.1

This works fine for OpenSSL < 3.0, but fails for >= 3.0

openssl s_client -psk 73656372657450534b -psk_identity Client_identity -dtls -cipher 'PSK:!NULL' -connect 127.0.0.1:4433

It appears that support for RFC 5746 is needed as per CAN-2009-3555.

@boaks
Copy link
Contributor

boaks commented Oct 31, 2022

Openssl is anyway a bad choice for IoT ;-).

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Oct 31, 2022

That maybe, but my (PSK) libcoap regression tests are all messed up when running Ubuntu 22!

@boaks
Copy link
Contributor

boaks commented Oct 31, 2022

I'm running still 20.04.
If possible, please provide a capture and logs.
Maybe the tinydtls server doesn't proper ignore it.
Or do you test a "renegotiation" for DTLS?

@boaks
Copy link
Contributor

boaks commented Oct 31, 2022

SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION maybe a the fast way with openssl 3.0.

Not sure, how to proceed with that in general:

In my opinion (the discussion have been a couple of years ago in Eclipse/Californium), renegotiation was in too many cases misinterpreted as a way to overcome broken DTLS in the context of NATs. It doesn't work there.
With UDP, we never need a TCP connect and with that, it doesn't make a difference, if the client uses a fresh dtls-handshake or one encrypted by the current DTLS context.

So, the first question is more:

  • who is using the "renegotiation" feature and for which purpose?
  • depending on the answers, it may be easier (and code saving) to remove the left overs.
    Or to try to fix it in the sense of RFC5746.

My interpretation of RFC5746 is, that it doesn't support to implement "no renegotiation at all". It looks, like the way to do that is to "negotiate the secure renegotiation" and then block, when that is really requested. Depending on the outcome, if renegotiation must be supported, I will try to ask the tls list about the way, to indicate, that renegotiation isn't supported at all.

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Oct 31, 2022

If possible, please provide a capture and logs.
Maybe the tinydtls server doesn't proper ignore it.

traces.zip

Or do you test a "renegotiation" for DTLS?

Not that I am aware of.

It all appears to hinge around unsafe legacy renegotiation disabled as in

4017A0F9977F0000:error:0A000152:SSL routines:final_renegotiate:unsafe legacy renegotiation disabled:../ssl/statem/extensions.c:879:

SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION

I'm not comfortable with just somehow enabling this. I have to assume that the OpenSSL people thought this through.

@boaks
Copy link
Contributor

boaks commented Oct 31, 2022

I have to assume that the OpenSSL people thought this through.

Unfortunately, I assume, that a lot of TLS stuff is taken to DTLS without considering UDP.
Even on the IETF tls-group level, not only in openssl.

AFAIK, openssl doesn't go for the SHOULD in RFC 6347 - Section 4.2.8

In cases where a server believes it has an existing association on a
given host/port quartet and it receives an epoch=0 ClientHello, it
SHOULD proceed with a new handshake but MUST NOT destroy the existing
association until the client has demonstrated reachability either by
completing a cookie exchange or by completing a complete handshake
including delivering a verifiable Finished message.

If we then really consider the threat of a renegotiation attack as mitigated by RFC5746, UDP and NATs will change the threat fundamentally. Assuming the (simplified) attack is based on the lose coupled triple of (ip endpoint, appl. data, security context), using functions as "receive(ip, data)" and "context(ip, context)" but not "receive(ip, data, context)", then RFC5746 can't mitigate this attack for DTLS 1.2.
Following the SHOULD from above, it's always possible, that a new handshake changes the context between "receive" and "context" and that will be exactly misinterpreted as under that attack.
The mitigation then depends on additional assumptions.
On the client side, assuming no support for "DTLS role exchange" simplifies the implementation, because there will be no unaware handshake. For servers, that's not that easy. In Eclipse/Californium we adapted the API to use the "receive(ip, data, context)" approach. We extended that even to "send(ip, data, context)" so that we can even reject sending data to the wrong other side.

So, I still consider to use SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION is not too bad for now.

More requires then also more support from others asking and discussing the topic in the tls-mailing list.

@boaks
Copy link
Contributor

boaks commented Oct 31, 2022

Thanks for the logs.
Yes, openssl chose now to consider RFC5746 as mandatory by default.
I already asked someone worth more background, if that will become more common.
As I explained above, that's in my opinion a missdecision. My experience from trying to catchup DTLS specific questions in the tls-mailing-list are not that good. So it depends on the interests of others, how that ends.

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Oct 31, 2022

Thanks for doing all the digging around. Not entirely sure how clear the mud is getting!

The following in the appropriate place in libcoap code cures the issue I am seeing.

+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+    SSL_CTX_set_options(context->dtls.ctx, SSL_OP_LEGACY_SERVER_CONNECT );
+#endif /* OPENSSL_VERSION_NUMBER >= 0x30000000L */

Apparently SSL_OP_LEGACY_SERVER_CONNECT was enabled by default in OpenSSL 0.9.8m and later (but not 3.0 onwards)

I feel a lot more comfortable with "my" clients being able to talk to "RFC 5746 unpatched" servers, but not to support access from clients to "my" coded server by setting SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION instead. Certainly (libcoap) tinydtls client can talk to openssl server with this code update as well as vice-versa.

@boaks
Copy link
Contributor

boaks commented Oct 31, 2022

The real issue is, that RFC 5746 does not consider "no renegotiation at all".
Everything else, as implementing RFC 5746 for DTLS 1.2, is in my opinion obfuscation without providing security.

@boaks
Copy link
Contributor

boaks commented Nov 1, 2022

RFC7925

To prevent the renegotiation attack [RFC5746], this specification
REQUIRES the TLS renegotiation feature to be disabled. Clients MUST
respond to server-initiated renegotiation attempts with an alert
message (no_renegotiation), and clients MUST NOT initiate them.

RFC5746

In order to enable clients to probe, even servers that do not support
renegotiation MUST implement the minimal version of the extension
described in this document for initial handshakes, thus signaling
that they have been upgraded.

So the extensions in RFC5746 are more indicating a updated implementation, not really the feature.

@obgm

I would propose to send a e-mail to the tinydtls mailing list and open an issue to discuss removing the renegotiation as of RFC 7925. Check, if someone requires it. Are you OK with that?

And then add the minimal RFC5764 support, if renegotiation could be removed.

If renegotiation could not be removed, it requires a "full RFC5746", even if that in my opinion doesn't help at all for DTLS 1.2 ;-).

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Nov 1, 2022

Minimal RFC5764 sending back a 0 length "renegotiated_connection" field works for me (and what GnuTLS and MbedTLS do).

@obgm
Copy link
Contributor

obgm commented Nov 1, 2022

Thanks @boaks and @mrdeep1 for your assessment and proposed solution.

I agree with boaks's proposal in following RFC 7925 (it removes renegotiation exactly because it is too difficult to get it right), and add the minimal RFC 5746 support to improve interoperability. Checking on the mailing list first is always good practice, so go ahead (from another context, I had not the feeling that people depend on renegotiation).

@boaks
Copy link
Contributor

boaks commented Nov 2, 2022

See issue #176 .

The e-mail was sent.
tinydtls-dev seems to be updated with a larger delay.

@boaks
Copy link
Contributor

boaks commented Nov 7, 2022

I implemented RFC 5746 for Eclipse/Californium. My first impression using TLS_EMPTY_RENEGOTIATION_INFO_SCSV is, that it will be easier with/after PR #147 / #148.

WDYT?

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Nov 7, 2022

I just checked OpenSSL (using libcoap) against Eclipse/Californium using PSK which works as expected.

I'm not convinced that we need to fully implement RFC 5746 - just a subset and if a subsequent renegotiation comes along refuse to handle it.

That said, as the SCSV is a pseudo algorithm, without revisiting your CCM code changes, it makes sense that it is a part of that work.

@boaks
Copy link
Contributor

boaks commented Nov 7, 2022

I just checked OpenSSL (using libcoap) against Eclipse/Californium using PSK which works as expected.

Against the Californium Sandbox?
That has not been updated since the PR in Californium has been merged.

I'm not convinced that we need to fully implement RFC 5746 - just a subset and if a subsequent renegotiation comes along refuse to handle it.

I always agreed, that we only need such a minimal version. Especially, if we remove renegotiation at all (see discussion above and issue #176). The minimal version includes the TLS_EMPTY_RENEGOTIATION_INFO_SCSV for the client to indicate support for RFC5746, and the empty RenegotiationInfoExtension for the server to response to that request.

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Nov 7, 2022

Against the Californium Sandbox?

Doh - just realized I was running it with my fixed code. Yes it does fail otherwise.

For the SCSV, yes you are correct.

@boaks
Copy link
Contributor

boaks commented Nov 7, 2022

I update the Californium sandbox today.
Tomorrow, the RFC5746 (minimal version) should be available.

@boaks
Copy link
Contributor

boaks commented Nov 7, 2022

(Californium's sandbox is updated. It supports now a minimal version of RFC5746.)

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Nov 7, 2022

Thanks - works with my unpatched libcoap code.

This was referenced Nov 28, 2022
@boaks
Copy link
Contributor

boaks commented Nov 29, 2022

PR #181 adds RFC5746 minimal version support.

@boaks boaks added the please close Please verify, that the issue is solved and close it. label Feb 7, 2024
@boaks
Copy link
Contributor

boaks commented Feb 7, 2024

@mrdeep1

For me this issue is solved with PR #181.
If you agree, please close it.

@mrdeep1
Copy link
Contributor Author

mrdeep1 commented Feb 12, 2024

Yes, this works as expected. Thanks.

@mrdeep1 mrdeep1 closed this as completed Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please close Please verify, that the issue is solved and close it.
Projects
None yet
Development

No branches or pull requests

3 participants