Closed Bug 1600144 Opened 2 years ago Closed 2 years ago

Check that DTLS message sequence number is set right for CH2.

Categories

(NSS :: Libraries, defect, P2)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ekr, Assigned: mt)

Details

(Keywords: sec-other)

Attachments

(1 file)

CH2 should have MSN=1 (see RFC 6347 S 4.2.2). We need to verify that this is correct and is handled correctly in the server side (which is new code with TLS 1.3).

Marking this "security" because it's a holiday and I haven't had time to think through the implications, though I don't think it should be anything other than correctness. The failure case would presumably be mix-and-match with is_present(cookie) != (seq == 1). It's not clear to me how this would be a vulnerability, but we need to think through it.

Update: here is the code in dtlscon.c which seems to handle this:

https://searchfox.org/nss/source/lib/ssl/dtlscon.c#334

        /* If we're a server and we receive what appears to be a retried
         * ClientHello, and we are expecting a ClientHello, move the receive
         * sequence number forward.  This allows for a retried ClientHello if we
         * send a stateless HelloRetryRequest. */
        if (message_seq > ss->ssl3.hs.recvMessageSeq &&
            message_seq == 1 &&
            fragment_offset == 0 &&
            ss->ssl3.hs.ws == wait_client_hello &&
            (SSLHandshakeType)type == ssl_hs_client_hello) {
            SSL_TRC(5, ("%d: DTLS[%d]: Received apparent 2nd ClientHello",
                        SSL_GETPID(), ss->fd));
            ss->ssl3.hs.recvMessageSeq = 1;
        }

This doesn't enforce that there is a cookie, which I think is fine for HRR because HRR doesn't rely on the server enforcing cookies but rather on the CH being in policy (whatever that means) or not. The nature of stateless HRR is that you can't enforce cookie presents. However I think we should at least consider checking that when we see a CH2 there is a cookie. MT, WDYT?

Flags: needinfo?(mt)

I think that the only thing we can realistically do in this case is assign ss->ssl3.hs.helloRetry = PR_TRUE. Then, we should modify this:

https://searchfox.org/nss/rev/ce7d80af16477b22af4c9bb5d60bdc32541e4b50/lib/ssl/ssl3con.c#8714-8716

Rather than just set the value directly, we could replace that with a check:

        if (ss->ssl3.hs.helloRetry) {
            if (!ssl3_FindExtension(ss, ssl_tls13_cookie_xtn)) {
                // generate a fatal error here
            }
        } else if (ssl3_FindExtension(ss, ssl_tls13_cookie_xtn)) {
            ss->ssl3.hs.helloRetry = PR_TRUE;
        }

Sound reasonable?

Flags: needinfo?(mt)

This seems like the only option. Do you see a way in which the current state is problematic? I note that with TLS there is no way to detect this case.

My analysis of the HelloRetryRequest handling showed that the code is generally very good about only using the second ClientHello for determining connection parameters. We run the full processing path for the second ClientHello and only add validation.

In this case, a client might send a ClientHello with message_seq==1 and we will accept that as though it were the first ClientHello. That's not strictly good, but the only way to pass Finished is if both client and server agree that this is what happened. It would be bad if we didn't have transcript integrity, because then an attacker might be able to erase the first ClientHello.

In the end, we don't really rely on the correct disposition of message sequence numbers for the security of the protocol. That is all that is really in question here.

We already have the necessary check, once we set the flag: https://searchfox.org/nss/rev/ce7d80af16477b22af4c9bb5d60bdc32541e4b50/lib/ssl/tls13con.c#1758-1763

Uploading the tweak and test now.

Assignee: nobody → mt
Severity: normal → minor
Keywords: sec-other
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Target Milestone: --- → 3.48

The logic that deals with stateless HelloRetryRequest in DTLS
allows this one-off increment to the message_seq field in case the server was
operating statelessly. However, when it does, it should insist on the
ClientHello carrying a cookie; concretely, it should set the flag that says that
a HelloRetryRequest was sent, even if it doesn't currently remember that it sent
one. That is the only way that this condition could be met.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: 3.48 → 3.49
Group: crypto-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.