1553612 - client drops ciphers on hello retry request

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Hubert Kario]

Attachment: capture.pcap

When tstclnt is configured with explicit list of supported groups, it removes DHE ciphersuites in the 2nd Client Hello. This is not an allowed modification according to https://tools.ietf.org/html/rfc8446#section-4.1.2.

Version:
nss-3.44.0

Reproducer:
Configure server with support for P384 and P521
Connect using:
tstclnt -I x25519,P256,P384 -d nssdb -4 -h localhost -p 4433

Result:
2nd client hello omits DHE ciphersuites (see attachment)

Comment 1

[Dennis Jackson]

Attachment: Bug 1553612 - Ensure clients offer consistent ciphersuites after HRR. r=jschanck,bbeurdouche

After a HRR, TLS1.3 clients must include the same list of ciphersuites
in the second client hello as was sent in the first client hello.
This changeset adds a cache for the ciphersuites sent with CH1 which is
reused with CH2. If ECH is offered by the client, two seperate ciphersuite lists are maintained.

Comment 2

[Martin Thomson [:mt:]]

I spent a little while trying to work out what might have caused the list of suites to change in response to the HRR.

My first theory was that the client was removing unnecessary groups from its set of supported groups in response to the key_share from the server. As we include cipher suites depending on whether they are supported, that seemed like a good thing to investigate. But I couldn't trace this to any code.

So I wrote a simple test:

TEST_F(TlsConnectStreamTls13, DheAndEcdheThenHrr) {
  EnsureTlsSetup();
  client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_1,
                           SSL_LIBRARY_VERSION_TLS_1_3);
  client_->ConfigNamedGroups({ssl_grp_ec_secp256r1, ssl_grp_ec_secp384r1, ssl_grp_ffdhe_2048});
  server_->ConfigNamedGroups({ssl_grp_ec_secp384r1});
  Connect();
}

This resulted in a HelloRetryRequest that more or less replicated the conditions from the report, but both ClientHello messages contain exactly the same set of ciphers.

I was able to replicate with the tstclnt command line Hubert shared. (For posterity, I'm including my reproducer here.)

../nss-tools/nss-run.sh selfserv -p 4433 -d ../tests_results/security/localhost.1/ssl_gtests -n rsa -I P384 &
../nss-tools/nss-run.sh -t 50 tstclnt -h 127.0.0.1 -4 -p 4433 -I x25519,P256,P384 -b -o -D

The more complicated test in D132263 also reproduces the issue. The difference shows: Hubert didn't configure the stack for FFDHE groups, but I did. Once FFDHE is enabled, DHE cipher suites will be kept and the bug is "fixed".

Note that you might be surprised to learn that we offer DHE suites when DHE groups are not enabled. Our named group configuration options are explicitly not used for filtering our DHE unless named groups are required. That is, we're willing to accept DHE suites no matter what groups we have enabled and the set of named groups only controls use in TLS 1.3 or when SSL_REQUIRE_DH_NAMED_GROUPS is enabled.

In any case, I found the bug. It doesn't require the sort of changes that are in D132263. All it requires is that we correctly check the version when we check if the key exchange method is enabled:

--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -715,17 +715,17 @@ ssl_KEAEnabled(const sslSocket *ss, SSLK
                  * 4. The weak group is enabled. */
                 if (!ss->opt.requireDHENamedGroups &&
                     !ss->xtnData.peerSupportsFfdheGroups &&
                     ss->version < SSL_LIBRARY_VERSION_TLS_1_3 &&
                     ss->ssl3.dheWeakGroupEnabled) {
                     return PR_TRUE;
                 }
             } else {
-                if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3 &&
+                if (ss->vrange.min < SSL_LIBRARY_VERSION_TLS_1_3 &&
                     !ss->opt.requireDHENamedGroups) {
                     /* The client enables DHE cipher suites even if no DHE groups
                      * are enabled. Only if this isn't TLS 1.3 and named groups
                      * are not required. */
                     return PR_TRUE;
                 }
             }
             return ssl_NamedGroupTypeEnabled(ss, ssl_kea_dh);

The problem here is that we check against ss->version, which is originally 0 and becomes 0x0304 when we handle a HelloRetryRequest. That means that we construct the cipher suite list incorrectly the second time around: we mistakenly filter out DHE ciphersuites because they are no longer valid for use with available key exchange methods. The correct check uses the minimum version that is configured.

The first clause (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) is always true in all cases except after HelloRetryRequest. For initial ClientHello construction, this was hidden by the fact that we filtered DHE suites out using ss->vrange elsewhere. For HelloRetryRequest, this just highlighted a gap in our testing.

Now, I won't deny that the method in D132263 will address the problem more comprehensively. If we have other such inconsistencies, we'll have similar bugs. But that potentially masks real issues as well, so I would prefer the more surgical approach to fixing this.

Comment 3

[Dennis Jackson]

Thanks for the investigation and detailed explanation. Since ssl3_config_match is used for both advertising ciphersuites and validating the server's choice, I'd assumed that keeping it invariant between client hellos wasn't desirable to begin with.

Whilst I'm happy this approach works for all the scenarios I could think of, I wouldn't be surprised if we go on to find some edge cases in the ECH spec.

Comment 4

[Martin Thomson [:mt:]]

So there might be some things we can do to preserve the invariant. For example, we could teach the test framework to parse out ClientHello messages and perform the check. Not just for a single test case, but for all of them. I don't know if this is something worth doing though. It would make the tests a tiny bit more complicated and more difficult to maintain. We might make it reasonably self-contained though, so I guess that I Will leave that thought with you as the one who might end up doing the work.

Comment 5

[Dennis Jackson]

https://hg.mozilla.org/projects/nss/rev/6d4f9e74d8ef804e6a0d9231433c66f36c11343a