search

bcgit / bc-csharp

BouncyCastle.NET Cryptography Library (Mirror)
https://www.bouncycastle.org/csharp
MIT License
1.65k stars 552 forks source link

DTLS: wrong PSK no longer terminates connection immediately #466

Open patagonaa opened 1 year ago

patagonaa commented 1 year ago

During the DTLS Connection ID implementation, code was added to discard records with invalid MACs. This has the effect that connections with invalid credentials (at least when using TLS-PSK) aren't immediately terminated with a bad_record_mac TlsFatalAlert anymore: https://github.com/bcgit/bc-csharp/blob/04ddd2869a38e326e200534f5065e1a17f7f715d/crypto/src/tls/DtlsRecordLayer.cs#L780-L787

RFC 9146 6. "Peer Address Update" says

DTLS implementations MUST silently discard records with bad MACs or that are otherwise invalid.

and RFC 7366 3. says

[...] if the MAC verification fails, then processing SHALL terminate immediately. [...] For DTLS, the record MUST be discarded, and a fatal bad_record_mac MAY be generated

So while the new implementation is (in my opinion) technically correct, it is still somewhat flawed. Even when the MAC verification already fails during the handshake (i.e. when receiving the finished message), the packets are just discarded instead of returning a TlsFatalAlert. Clients with invalid credentials now have to wait (and usually resend multiple times) until they run into a timeout, instead of immediately getting the information that their credentials are invalid.

I think during the handshake, bad_record_mac alerts should still be thrown.

patagonaa commented 1 year ago

From what I can tell, maybe the check for bad_record_mac in the DtlsRecordLayer may be removed entirely, as there is a similar check in DtlsTransport (depending on the value of TlsServer.IgnoreCorruptDtlsRecords), which is only active once the Handshake is completed.

also, TlsServer.IgnoreCorruptDtlsRecord should be set to true by default imo, at least when a connection ID has been negotiated.

peterdettman commented 1 year ago

I had to think on this, but in the end I don't think that we should treat the Finished message case any differently. I found at least this related issue for openssl, where they also reject the idea:

https://github.com/openssl/openssl/issues/10906

I already plan to change the default value of IgnoreCorruptDtlsRecord to true in the next major version (presumably a 3.0 release later this year).

patagonaa commented 1 year ago

As I already said, I would prefer if there was a way to shorten the handshake process if the PSK is wrong. The client devices we're communicating with are pretty slow, so we can't use a lower handshake timeout.

If I read the code correctly, IgnoreCorruptDtlsRecord doesn't do anything right now anyway, because the bad_record_mac error is ignored in DtlsRecordLayer instead of thrown to DtlsTransport (where IgnoreCorruptDtlsRecord is handled). If the check in DtlsRecordLayer were to be removed, the alert would still be raised and the connection ended during the handshake, and after the handshake IgnoreCorruptDtlsRecord would work correctly.

Though ultimately, it's your call to decide on an interpretation of the spec.