1481209 - Missing ChangeCipherSpecs messages in TLS 1.3

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P3)

Description

[Franziskus Kiefer [:franziskus]]

The 1.3 spec says

> If not offering early data, the client sends a dummy change_cipher_spec record (see the third paragraph of Section 5.1) immediately before its second flight

NSS currently doesn't always send change_cipher_spec, e.g. if an alert occurs before. This fails a couple bogo tests, e.g. UnknownExtension-Client-TLS13. Not sure which behaviour is better here.

Comment 1

[Eric Rescorla (:ekr)]

Is it always alerts? If so, this seems like it's conformant, because alerts are not really part of a flight.

Comment 2

[Franziskus Kiefer [:franziskus]]

Looks like it's always alerts. Then we should get this fixed in bogo.

Comment 3

[David Benjamin]

Hrm. I thought we'd handled that:
https://boringssl.googlesource.com/boringssl/+/master/ssl/test/runner/conn.go#914

Or is the issue that you're sending an encrypted alert without a ChangeCipherSpec?

If so, I'm curious how you're managing to do that. While I suppose not required by the spec, sending the CCS seems prudent; a middlebox that wants a CCS there will get upset whether it's an alert or handshake data (they are, after all, meant to be indistinguishable by content type). Getting the alert through is less important, but if you're sending it at all, seems you ought to want it through. In our implementation, we queue up the CCS at the same time as installing new keys.

If you still want to skip the CCS in this case, I can probably make that logic a bit more complicated to tolerate that (read a CCS before the record but only enforce its presence after we've decided what we're sending), though I like testing that BoringSSL *does* send the CCS in this case, so it might be conditioned on a shim config.

(I probably won't get to this for a bit; I'm OOO until Wednesday and then I'll be resurfacing in a new office with several weeks of backlog.)

Comment 4

[Eric Rescorla (:ekr)]

I don't believe we are sending encrypted alert without CCS. It looks to me like they are coupled in NSS as well. Franziskus, can you elaborate?

Comment 5

[Martin Thomson [:mt:]]

If this is another case where the server first flight has problems and where NSS sends an alert in the clear.  We already agreed that this was an area that we disagree on.  I'll have to dig around for the bug number, but the story there is that we don't send alerts encrypted with handshake keys, even though BoringSSL sometimes expects them that way.

We also don't always read those alerts successfully (usually as a server).  Look for gtests where we expect one peer to send one type of alert, then the other generates SSL_ERROR_BAD_MAC_READ in response for a sampling of these cases.

Comment 6

[Franziskus Kiefer [:franziskus]]

Failing tests all seem to send alerts in the clear.

> We already agreed that this was an area that we disagree on.  I'll have to dig around for the bug number, but the story there is that we don't send alerts encrypted with handshake keys, even though BoringSSL sometimes expects them that way.

If that's the case, we should probably just disable those test mentioning that bug.

Comment 7

[Franziskus Kiefer [:franziskus]]

Attachment: Bug 1481209 - update bogo config error description, r=mt

Comment 8

[Phabricator Automation]

Comment on attachment 8998173 [details]
Bug 1481209 - update bogo config error description, r=mt

Martin Thomson [:mt:] has approved the revision.

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

Comment 9

[David Benjamin]

Oh, that makes sense. Since BoGo expects encrypted alerts to be prefaced by a CCS, seeing an unencrypted alert would read as a missing CCS, but it's really that we're expecting an encrypted alert. Just the error is confusing.

In that case, I shall assume no changes are needed on our end. It sounds like the change I had in mind in comment #3 wouldn't have helped any.

(FWIW, BoGo does not expect CCS before unencrypted alerts, so in tests where both sides agree on that, things should work out fine.)

Comment 10

[Martin Thomson [:mt:]]

Maybe we can tweak this function (tls13_SetAlertCipherSpec) so that some of the places where CCS is expected get a CCS.  Not sure if it is worthwhile though.  I'm happy with the current situation, even if it means slightly less test coverage from the bogo side.

Comment 11

[Franziskus Kiefer [:franziskus]]

updated bogo config to describe why the tests are disabled.

https://hg.mozilla.org/projects/nss/rev/d26e112957a9e82586e5d03b4018f319972a6fec