Closed Bug 1714245 Opened 3 years ago Closed 2 years ago

Remove PSKs with incompatible hashes after HRR

Categories

(NSS :: Libraries, enhancement, P4)

enhancement

Tracking

(firefox111 fixed)

RESOLVED FIXED
Tracking Status
firefox111 --- fixed

People

(Reporter: mt, Assigned: lschwarz)

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main111-])

Attachments

(1 file)

When we receive a TLS HelloRetryRequest, that fixes the choice of cipher suite. That means that the PSK we have might not be valid if it uses a different hash.

Rather than include the PSK in this case, we can save a bunch of effort by dropping it. This is permitted by the protocol.

Saving effort also means that we don't need to think too hard about how the transcript for the PSK binder is constructed with mismatched hash functions (one for the ClientHello compression, another for the binder). It looks like our code is correct in this regard, but it would need careful validation to be sure. As this is a nonsensical situation it is better to avoid the question entirely.

Group: crypto-core-security

Marking security until Benjamin has a chance to take a look, just to be sure.

Flags: needinfo?(bbeurdouche)
Keywords: sec-audit
Flags: needinfo?(bbeurdouche)

Dennis, when you have a few mins could you look at this bug please? : ) Thanks!

Flags: needinfo?(djackson)
Assignee: nobody → djackson
Status: NEW → ASSIGNED
Flags: needinfo?(djackson)

RFC 8446 §4.1.4

In addition, in its updated ClientHello, the client SHOULD NOT offer any pre-shared keys associated with a hash other than that of the selected cipher suite. This allows the client to avoid having to compute partial hash transcripts for multiple hashes in the second ClientHello.

If I have understood correctly, dropping PSKs with the wrong hash function is required by the spec, so we should definitely do this.

Flags: needinfo?(mt)

Right. We don't have to drop them and it was easier to keep them at the time. There are lots of reasons to do the work though.

Separately, if you can work out whether we have any concrete problems in existing code, we can work out whether to keep this as a security bug. We were cautious initially, but I haven't seen any cause for concern. Confirmation that it is not a security bug would be helpful.

Flags: needinfo?(mt)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:djackson, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Flags: needinfo?(djackson)
Flags: needinfo?(djackson)
Assignee: djackson → lschwarz
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Group: crypto-core-security → core-security-release
Target Milestone: --- → 3.88
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main111-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: