1438266 - Be strict about versions in the TLS 1.3 supported_version ServerHello extension

Status: NEW

(NSS :: Libraries, enhancement, P2)

Description

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

The new BoGo test SupportedVersionSelection-TLS12 [1] verifies that if the server sends the TLS 1.3 supported_versions extension in the ServerHello, that it not try to negotiate only TLS12. Currently NSS tolerates this behavior, but BoGo has the opinion that we should consider the extension to be invalid and abort the connection.

[1] https://github.com/google/boringssl/blob/67968895b3b8cace34ea6dbdf65ac922667959ac/ssl/test/runner/runner.go#L5590

Comment 1

[Daniel Veditz [:dveditz]]

Why is this a hidden security bug? how does it put our users at risk?

Comment 2

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

Going to redirect to Tim. We were being conservative during a triage.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Asking for TLS 1.3 and then receiving SH.supported_versions=[TLS12] should fail because the server *should* include the downgrade sentinel in SH.random if it really wants TLS v1.2. BoGo however seems to only modify SH.supported_versions and so we don't abort because the sentinel isn't there. The presence of SH.supported_versions should probably act as a 2nd sentinel? We should also abort if we didn't include TLS12 in CH.supported_versions (not sure if we do).

I'd leave this hidden until we figure out the desired behavior. We don't want to expose 1.3 users to potential downgrades. Although I don't think that would work, a proper server would set the sentinel and sign it.

Comment 4

[Eric Rescorla (:ekr)]

Tim, you're right that the spec requires this. Sounds like a bug in BoGo.

Comment 5

[David Benjamin]

I don't think this is a security or downgrade risk, no. If an attacker can change the contents of the supported_versions extension, they presumably can also delete it, which would have the same consequence.

(Even without the sentinel, I don't think we actually believe TLS 1.2's Finished hash anti-downgrade mechanism is catastrophically broken. Downgrade protection is not a TLS 1.3 innovation. It's just stronger in TLS 1.3... for clients which disable RSA decryption... which is not actually practical yet. So we're really just banking on the Finished hash. And then there's False Start which does actually depend on the downgrade sentinel. But that's okay because False Start requires ECDHE, so the RSA decryption hole is avoided. Isn't this protocol marvelous?)

Anyway, no, the motivation of this check wasn't security or anything. It was just an extension of the "no unexpected ServerHello extensions" rule. Just as TLS 1.3 defining ServerHello.key_share does not make it allowed in TLS 1.2, our interpretation was that ServerHello.supported_versions was likewise not allowed in TLS 1.2, and it falls out of our usual logic.

Of course, at the time we implemented all this, this was still 7e01 and there wasn't a spec. I do prefer our interpretation, but not strongly. Our interpretation is marginally more convenient for the way we implement things---TLS 1.2 ServerHello extensions logic is largely shared with TLS 1.3 EncryptedExtensions, not TLS 1.3 ServerHello---and I generally prefer there be only one way to specify something, not two. But the other way would be easy enough to do I suppose. It just seems unnecessary.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Opening this up.

Comment 7

[Phabricator Automation]

Attachment: Bug 1438266 - Disable SupportedVersionSelection-TLS12 BoGo test to fix bustage r=franziskus

Comment 8

[Phabricator Automation]

Comment on attachment 8954350 [details]
Bug 1438266 - Disable SupportedVersionSelection-TLS12 BoGo test to fix bustage r=franziskus

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.

https://phabricator.services.mozilla.com/D650

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Disabled the test to unbreak the 3.36 release:

https://hg.mozilla.org/projects/nss/rev/08267a784d00