1520459 - Issues with handling record_size_limit extension

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Hubert Kario]

There are two problems in handling record_size_limit extension:

1. the NSS server does not abort connection when the 2nd CH in HRR handshake includes a modified record_size_limit extension – reproducer: "modified extension in 2nd CH in HRR handshake"
2. when the extension is malformed (has too big payload/is padded) it is rejected with illegal_parameter alert instead of the expected decode_error - reproducers: "padded extension in TLS 1.3" and "padded extension in TLS 1.2"

tested with currend default branch (c8f7602ce9e6)

The test set is available in https://github.com/tomato42/tlsfuzzer/pull/494 script name test-record-size-limit.py

It expects to run against a server configured with an RSA certificate:
selfserv -d sql:/tmp/nssdb -n server -p 4433 -V tls1.0: -H 1 -u

the necessary options to run against NSS server:
test-record-size-limit.py --reply-AD-size 137 --cookie --hrr-supported-groups --supported-groups

Comment 1

[Hubert Kario]

There are few more tests added in another PR: https://github.com/tomato42/tlsfuzzer/pull/506 and they have uncovered one more issue with HRR:

3. when the extension is dropped or added in the 2nd CH in HRR handshake, the connection is not aborted. Reproducer: 'added extension in 2nd CH in HRR handshake' and 'removed extension in 2nd CH in HRR handshake'

Comment 2

[Martin Thomson [:mt:]]

Just to make this clear, there is no requirement in RFC 8449 or 8446 that states that the second ClientHello needs to contain identical values for extensions. Section 4.1.4 of RFC 8446 only specifies that certain values are updated, but is silent on whether the remainder of the ClientHello can be updated. It does specify checks on the cipher suite, which we do check.

Though the implication is that changes to other extensions are not expected, there is no prohibition on that.

On that basis, I think that 1 and 3 here are invalid. 2 is easy enough to fix.

Comment 3

[Martin Thomson [:mt:]]

Ekr pointed me to the requirement that the client not change the value. That is fine, but 2 and 3 are only valid if the server is required to check, which isn't in the spec. Such a requirement would be incompatible with a stateless retry, which NSS supports.

Comment 4

[Hubert Kario]

No, RFC 8446 is clear that the whole ClientHello, and that includes extensions, must be identical, see section 4.1.2:

In that case [HRR handshake], the client MUST send the same
ClientHello without modification, except as follows:

• If a "key_share" extension was supplied in the HelloRetryRequest,
  replacing the list of shares with a list containing a single
  KeyShareEntry from the indicated group.

• Removing the "early_data" extension (Section 4.2.10) if one was
  present. Early data is not permitted after a HelloRetryRequest.

• Including a "cookie" extension if one was provided in the
  HelloRetryRequest.

• Updating the "pre_shared_key" extension if present by recomputing
  the "obfuscated_ticket_age" and binder values and (optionally)
  removing any PSKs which are incompatible with the server's
  indicated cipher suite.

• Optionally adding, removing, or changing the length of the
  "padding" extension [RFC7685].

• Other modifications that may be allowed by an extension defined in
  the future and present in the HelloRetryRequest.

I don't see any mention of record_size_limit in the above list (despite RFC 8449 and record_size_limit cited explicitly in RFC 8446) or reference to Hello Retry Request or second Client Hello in RFC 8449, so the above requirement stands.

the server is required to check, which isn't in the spec.

so which value should the server select if the values are different between the two messages? That's unspecified by the standard and the RFC 8446 states that unless the server sends an extension in HRR message indicating that it supports the extension, the client MUST NOT modify any other extensions. And there is no permission to send record_size_limit in HRR in either RFC 8446 or RFC 8449. So, sorry, but no, the only sane behaviour of the server is to check if both the presence and the value of record_size_limit is unchanged between first and second CH.

Yes, I guess you can lawyer yourself out of checking unsupported extensions (not that I agree with such interpretation of standards), but record_size_limit is supported.

Also I do not agree that checking CH extensions for changes is intrinsically incompatible with stateless retry, but that is off topic here.

Comment 5

[Martin Thomson [:mt:]]

I guess we disagree then.

The server can use the second ClientHello. That's simple, obvious, and sane.

And I believe that checking and stateless HelloRetryRequest is intrinsically incompatible. I'd be interested in a proof to the contrary, if you have one.

Comment 6

[Hubert Kario]

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

The server can use the second ClientHello. That's simple, obvious, and sane.

and which part of the standard says so?

Comment 7

[J.C. Jones [:jcj] (he/him)]

Marking p3 based on comment 2 and comment 3.

Comment 8

[Martin Thomson [:mt:]]

Attachment: Bug 1520459 - Send decode_error for padded record_size_limit extension, r?jcj

This is all I plan to do for this bug.

Comment 9

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/9fc1875fe9bc6c7586956fa21862c4a272585c54 contains the alert fix.

Marking resolved because the other pieces are not valid. If Hubert manages to convince the TLS working group that I'm wrong, then we can open another bug. Right now, that seems unlikely.