DTLS 1.0 backward compatibility broken since 1.69

[nils-ohlmeier]

In the Jitsi project (https://github.com/jitsi/) we just tried to upgrade our BC version from 1.68 to 1.70 and ran into the following issue:

We have one endpoint which uses BC 1.68 (which is the one we tried to upgrade to BC 1.70) and for the most part uses DTLS 1.2 to negotiate SRTP keys with other endpoints. But one of our endpoints still only uses DTLS 1.0. It appears that when establishing a connection with this endpoint this code got utilized up till 1.68: https://github.com/bcgit/bc-java/blob/master/tls/src/main/java/org/bouncycastle/tls/crypto/impl/bc/BcTlsDSSSigner.java#L40-L42

But with 1.69 the following signer is used and then throws because of the algorithm being null: https://github.com/bcgit/bc-java/blob/master/tls/src/main/java/org/bouncycastle/tls/crypto/impl/bc/BcTlsECDSA13Signer.java#L41-L44

Is this change intentionally breaking with DTLS 1.0, or is this a bug in the new TLS 1.3 implementation?

[peterdettman]

Please check the construction of BcDefaultTlsCredentialedSigner. It sounds like you have an unconditional non-null SignatureAndHashAlgorithm argument. It should be constructed with a null value for use with pre-(D)TLS 1.2 (I appreciate that BcTlsECDSASigner was more "tolerant" of this, and the API here is not ideal, but this should resolve the issue).

To determine whether to pass a non-null SignatureAndHashAlgorithm, I suggest using:

TlsUtils.isSignatureAlgorithmsExtensionAllowed(context.getServerVersion())

Please also check the server-side for similar code.

If I wanted to do a code review of the use of the BC (D)TLS API in Jitsi, would this be the place to look? Anywhere else?

[nils-ohlmeier]

Thanks a lot for the help so far @peterdettman !

Based on your advice above I created the following PR https://github.com/jitsi/jitsi-media-transform/pull/413 to only pass the SignatureAndHashAlgorithm if supported by the DTLS client. And I replaced our call to the deprecated PRF() function with the newer version, as the deprecated call would throw a null pointer exception after the suggested change. It would be highly appreciated if you could have a look at my PR to ensure that I'm in fact improving things and not making it worse.

If you have capacity a review of our usage of BouncyCastle exactly in the directory you pointed at would be super awesome!

[peterdettman]

Per my previous comment it should be:

TlsUtils.isSignatureAlgorithmsExtensionAllowed(context.serverVersion)

Note 'serverVersion'. clientVersion is the version that the client initially offered, but the actual negotiated version (which the server decides) is serverVersion (which may be an earlier version than clientVersion).

There is also another use of BcDefaultTlsCredentialedSigner in TlsClientImpl that has the same issue with the signatureAndHashAlgorithm.

[nils-ohlmeier]

Thank you @peterdettman! I didn't consider the negotiation as part of the state information. Makes total sense in hindsight. I don't think it is a big deal, as for these two endpoints we have both versions under our control, so we know that always the DTLS 1.0 as client will connect to the DTLS 1.2 server. Which also makes the client side a little less important.

But I'll create a follow up PR to get things cleaned up!

Feel free to close the issue, since it obviously was a coding issue on our end all along, or keep it open as an easy way of communication in case you get a chance to review our BouncyCastle usage.

[Neustradamus]

What is the status of this ticket?

The current BouncyCastle version is 1.75:

• https://www.bouncycastle.org/releasenotes.html

[peterdettman]

Closed - Not a Bug.