pkg/tinydtls: handling of close_notify

[janosbrodbeck]

Contribution description

Right now sock_dtls_session_destroy() does not do all the time what the API states.

RIOT/sys/include/net/sock/dtls.h

Line 699 in 08f1f97

* @brief Destroys an existing DTLS session

The sock implementation for tinyDTLS only initiates closing of a session, which results in sending a close_notify. But when the remote peer never ACK's the closing (e.g. not receiving or not answering), the session slot is blocked until device reboot. TinyDTLS endlessly stays in a STATE_CLOSING session state for that peer and there is no way of resetting a peer with the current DTLS sock API.

The main problem is that sock_dtls_session_destroy() does not allow a return value for feedback. This is easily fixable...but currently it is not possible to get the success of the session closing procedure for tinyDTLS since it only sends the close_notify and then returns.

I've looked into e.g. wolfSSL and their session close function seems to work a bit different: They send the close_notify and block for a short time and then return the success at this point of time. The user can decide how to handle a possible failure, e.g. try again or simply destroy the session object. DTLS libraries like wolfSSL, mbedTLS, tinyDTLS provide two distinct functions for that: session_close and peer_reset (= freeing of the peer object).

Our DTLS sock API should also provide two functions for that (and allow a return value!). No single function can provide a solution for all needs here. How dangerous that is shows the current WIP wolfSSL implementation (#15698) for the sock API of @pokgak. In the current WIP approach sock_dtls_session_destroy() tries to close the session two times (recommended by wolfSSL), and when it fails two times it just returns. There is no way to tell the user that the session is not destroyed. And since there is also no force destroy, you cannot destroy the session.

Road to fix this:

1. This PR simply resets the peer after close_notify was sent by tinyDTLS. It's allowed by the RFC, but mentions the possibility of truncation attack.
   But the current alternative is to live with the possibility of getting entire unusable peer slots in tinyDTLS (not detectable on user level). Note, in wireless networks it is not that unlikely that packets are not received depending on the link.
2. Mid-term we should provide two distinct functions with return value by the API. IMHO session_destroy should be renamed to session_close. For tinyDTLS we need to find a solution to provide feedback.
3. Additionally, maybe a generic session_get_status() would also make sense.

I would suggest this PR as an interim solution.

Feedback or other opinions are appreciated.

Testing procedure

Issues/PRs references

#16108

[janosbrodbeck]

Removed dtls_close() from sock_dtls_sesion_destroy() since dtls_reset_peer() includes it already, when no close_notify was sent.

[benpicco]

This looks reasonable - is there any drawback to merging this?

[janosbrodbeck]

• (-) Possibility of truncation attack
• (-) Enforces this decision

• (+) sock_dtls_session_destory() does what it states with this PR

Nonetheless, it should be noted in the documentation before merge. I don't know from any more drawbacks by merging this. This does not fix the root cause, and there are still situations which lead to a not successful handshake we cannot address right now. To fix those, we need to refactor this completely.

[janosbrodbeck]

Just to mention one situation we still do not address right now:

When we close a session, send the close_notify and immediately free the peer, but the remote peer does not receive this notify: In this case, the session is still active for the remote peer but for us, it is closed and cleaned up. If we now try to re-connect to this device, the success depends on the functionality of the used DTLS stack of the remote. If it supports re-establishing a session even when the session is already established from the device's perspective, all is fine, but if not...no chance until the other device wipes this session.
E.g. tinyDTLS does not support this, while californium/scandium supports it.

But again: This is a problem even without this PR.

[benpicco]

Maybe add a comment about the shortcomings of this solution, but if it's better than the status quo, I don't see a reason not to merge it.

[miri64]

Maybe @obgm can also have a look?

[obgm]

Thanks @miri64 for bringing this to my attention. I agree with @janosbrodbeck in his assessment of the situation and the benefits and risks of merging this PR.

Note that tinydtls has had some changes recently, especially eclipse-tinydtls/tinydtls#53 which changed the close_notify alert level from fatal to warning. IMO this has no direct effect on the issue that dealt with in this PR. I also recommend updating pkg/tinydtls soon (there are a few important PRs under review that I also recommend waiting for, though).

[janosbrodbeck]

I've added a note to the function documentation about the force reset. And I removed the draft state now.

[miri64]

Maybe link the tinyDTLS issue here as well, so we can track this in case this somehow gets resolved: eclipse-tinydtls/tinydtls#95

[janosbrodbeck]

Good idea! Squashed it right away.
I also watch the upstream changes to look out for fixes. A solution on tinyDTLS side would be the best of course. Just have to be careful later when supporting further DTLS stacks that they behave similarly.

[boaks]

Let me ask about the motivation to "close" the session:

When/why is that done?

• at the end of a communication phase, in order to pause/end communication for longer?
• in order to execute a new handshake to overcome some NAT issues (and bugs in DTLS server side implementation according RFC6347 - 4.2.8. Establishing New Associations with Existing Parameters) ?

The reason for this question is simple:

• yes the close should work proper and not cause additional trouble
• but, much more important: the server sides must implement 4.2.8!

The close may get lost, and so doesn't really help. 4.2.8 is in my opinion the only right way to do it.
For tinydtls I therefore prepared PR89 to fix the 4.2.8 behavior.

(In my Californium - Interoperability Tests it seems, that only mbedtls does 4.2.8 proper, openssl, gnutls and tinydtls (without PR 89) fails.)

[boaks]

Possibility of truncation attack

I'm still very confused about that and UDP. e.g. Wiki

SSL 2.0 used the TCP connection close to indicate the end of data. This meant that truncation attacks were possible: the attacker simply forges a TCP FIN, leaving the recipient unaware of an illegitimate end of data message (SSL 3.0 fixed this problem by having an explicit closure alert).

There is just nothing as a TCP FIN for UDP. So, any references, what that attack means for UDP?

[janosbrodbeck]

Yes, 4.2.8 is a crucial problem part here. With this workaround, we can run into the problem that we force close a session and cannot re-establish the session with the same peer until the other side timeouts/resets the session on their side. I mentioned this problem in comment #16422 (comment). To be honest, I consider 4.2.8 as a MUST and not a SHOULD for a DTLS stack, especially when used on constrained devices.

But this is and was a problem even without this workaround, since we do not get informed by the API right now if the closing was successful (= ACK from the remote part). In the case closing was unsuccessful, it could make tinyDTLS unusable in the long run without noticing by the application.

This workaround should never stay forever, it's only supposed to be a temporary solution for clients, so they don't become unusable over time if the close_notify is not ACK-ed.

[boaks]

To be honest, I consider 4.2.8 as a MUST and not a SHOULD for a DTLS stack,

:-) :-) :-) I always read MUST, I overseen the SHOULD ... :-) :-) :-)

This workaround should never stay forever, it's only supposed to be a temporary solution for clients, so they don't become unusable over time if the close_notify is not ACK-ed.

Yes, agreed. But even with a working "close_notify", everyone must be aware, that dtls-server, which don't implement 4.2.8 are always a bad choice. Assuming NATs, your "ip-endpoint" may have even been used by a other peer without close before.

[janosbrodbeck]

There is just nothing as a TCP FIN for UDP. So, any references, what that attack means for UDP?

That's actually a good question, what this means for UPD. I've taken the truncation part directly out of the TLS RFC. I'm honestly also too little in the picture, what it takes for truncation attacks all to effectively exploit.

[boaks]

I wrote a e-mail to the tls-mailing list.
Maybe that brings some more light.