1549225 - Disable DSA signature schemes for TLS 1.3

Status: RESOLVED FIXED

(NSS :: Libraries, task, P2)

Description

[Martin Thomson [:mt:]]

I've confirmed that we don't claim to support any MD5 schemes, so that's safe. But DSA is a concern.

A few things:

1. We shouldn't offer to accept DSA by default any more. The time for that has long passed. Remove them from defaultSignatureSchemes. This will require a notice in the release notes. I'll confirm with nss-dev@ first.

2. We should not send DSA signature schemes if ss->vrange.max >= SSL_LIBRARY_VERSION_TLS_1_3 during the construction of the ClientHello. This might result in handshake failure so we probably need to add a check to ssl3_config_match_init.

3. We should regard DSA schemes as not supported if the version is TLS 1.3. ssl_SignatureSchemeValid currently permits their use. Adding && spkiOid != SEC_OID_ANSIX9_DSA_SIGNATURE to that would suffice.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Bug 1549225 - Up front Signature Scheme validation, r?ueno

This patch started as an attempt to ensure that a DSA signature scheme would not
be advertised if we weren't willing to negotiate versions less than TLS 1.3.
Then I realized that we didn't do the same for PKCS#1 RSA.

Then I realized that we were still willing to try to establish connections when
we had a certificate that we couldn't use.

Then I realized that ssl3_config_match_init() wasn't being run consistently. On
resumption, we only ran it when we were PARANOID. That's silly because we
weren't checking policies.

Then I realized that we were allowing ECDSA certificates to be used when the
named group in the certificate was disabled. We weren't enforcing that
consistently either.

So that escalated quickly.

Now I think that the checks around what is allowed are more robust up front. As
a result, we should be offering fewer options that we're unwilling or unable to
follow through on. A good number of tests needed tweaking as a result because
we were sneaking past the checks in a few places.

I didn't see any real problems when digging around here. We were advertising
signature schemes that we would subsequently be unable to use because the
minimum version was TLS 1.3 and those schemes couldn't be used in TLS 1.3, but
that's not really a problem other than some wasted bytes.

Comment 2

[Hubert Kario]

(In reply to Martin Thomson [:mt:] from comment #1)

This patch started as an attempt to ensure that a DSA signature scheme would not
be advertised if we weren't willing to negotiate versions less than TLS 1.3.
Then I realized that we didn't do the same for PKCS#1 RSA.

But what about certificates? Are you sure we can depend on signature_algorithms_cert for that?

Then I realized that we were allowing ECDSA certificates to be used when the
named group in the certificate was disabled.

but supported_groups do not identify the groups the client is willing to accept in certificates in TLS 1.3, signature_algorithms do. Or are you talking about TLS 1.2 and earlier?

Comment 3

[Martin Thomson [:mt:]]

But what about certificates? Are you sure we can depend on signature_algorithms_cert for that?

NSS currently only checks the leaf certificate key for suitability. signature_algorithms_cert remains future work.

The change that I made here only pulls in the existing checks for suitability into the pre-connection checks.

but supported_groups do not identify the groups the client is willing to accept in certificates in TLS 1.3, signature_algorithms do. Or are you talking about TLS 1.2 and earlier?

Oops, I don't see any checks on supported_groups. That's something we check in TLS 1.2 when we are selecting certificates. This change doesn't touch that code. We were already checking against the group for TLS 1.2. The code here only checks that two things: that we are willing and able to validate a signature using a given signature scheme when we advertise that in signature_algorithms; and that we are able to produce at least one signature with the certificates that are configured and the signature schemes that are enabled. None of these checks are perfect, but it should reduce the cases where we advertise something that we can't follow through on.

Comment 4

[Hubert Kario]

(In reply to Martin Thomson [:mt:] from comment #3)

But what about certificates? Are you sure we can depend on signature_algorithms_cert for that?

NSS currently only checks the leaf certificate key for suitability. signature_algorithms_cert remains future work.

The change that I made here only pulls in the existing checks for suitability into the pre-connection checks.

I meant for other implementations: of course we're not going to accept PKCS#1 v1.5 schemes in TLS 1.3 CertificateVerify, but we will accept it in the Certificate. And actually that's the only scheme that Firefox will accept in certificates. So my point is that those schemes should be advertised in TLS 1.3-only ClientHello or CertificateRequest.

but supported_groups do not identify the groups the client is willing to accept in certificates in TLS 1.3, signature_algorithms do. Or are you talking about TLS 1.2 and earlier?

Oops, I don't see any checks on supported_groups. That's something we check in TLS 1.2 when we are selecting certificates.
This change doesn't touch that code.

Then what this test case is doing?:

TEST_P(TlsConnectTls13, Tls13CertDisabledGroup) {
  Reset(TlsAgent::kServerEcdsa256);
  static const std::vector<SSLNamedGroup> k25519 = {ssl_grp_ec_curve25519};
  server_->ConfigNamedGroups(k25519);
  ConnectExpectAlert(server_, kTlsAlertHandshakeFailure);
  server_->CheckErrorCode(SSL_ERROR_NO_SUPPORTED_SIGNATURE_ALGORITHM);
  client_->CheckErrorCode(SSL_ERROR_NO_CYPHER_OVERLAP);
}

ConfigNamedGroups does not influence supported_groups negotiation?

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mt, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 6

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/9b418f0a4912e0a7c928d0b0774e1815238984ee includes a fix for the problem Hubert identified (thanks!). The correct test was:

TEST_P(TlsConnectTls13, Tls13CertDisabledGroup) {
  Reset(TlsAgent::kServerEcdsa256);
  static const std::vector<SSLNamedGroup> k25519 = {ssl_grp_ec_curve25519};
  server_->ConfigNamedGroups(k25519);
  Connect();
}