Closed
Bug 1292898
Opened 8 years ago
Closed 8 years ago
If you negotiate TLS 1.2, NSS will still offer TLS 1.3 upon server-initiated renegotiation
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.27
People
(Reporter: ekr, Assigned: ekr)
Details
Attachments
(1 file, 1 obsolete file)
8.37 KB,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
This currently asserts in tls13_SetupClientHello but might continue in release mode. If this can complete, it might be a security issue.
Comment 1•8 years ago
|
||
Eric: could you describe the bug with the client and server clearly identified? The current description is not clear: If you negotiate TLS 1.2, NSS will still offer TLS 1.3 upon server-initiated renegotiation
Assignee | ||
Comment 2•8 years ago
|
||
Sorry about the confusion. 1. Client is connected for TLS 1.3. 2. Server negotiates TLS 1.2 3. Server then sends HelloRequest 4. Client tries to send a TLS 1.3 ClientHello 5. Client asserts in tls13_SetupClientHello because the list of keys is non-empty. However, if this assert is non-empty, you might be able to somehow reconnect to the server in TLS 1.3 mode on the second connection which would not negotiate RI. Not sure if this is an actual security issue but we should fix in any case.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ekr
Comment 3•8 years ago
|
||
I believe "offer"ing it is still desirable, assuming this comment is still a concern: https://dxr.mozilla.org/mozilla-central/rev/763fe887c37cee5fcfe0f00e94fdffc84a41ea1c/security/nss/lib/ssl/ssl3con.c#5660 However, the implementation should not accept any other version, so it's not really offering it. (Just added a test for this in BoringSSL: https://boringssl.googlesource.com/boringssl/+/e7e36aae253be9d054b5df108ac9fea42e1a0557 )
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8779106 -
Flags: review?(martin.thomson)
Comment 5•8 years ago
|
||
Comment on attachment 8779106 [details] [diff] [review] 0001-Bug-1292898-Reject-SSL_ReHandshake-when-in-TLS-1.3-m.patch Review of attachment 8779106 [details] [diff] [review]: ----------------------------------------------------------------- Overall, I don't see any problem with landing this as-is. I can't see a security problem here. Mostly this is a case of "do stupid things, get bad results". I don't understand the comment on the test though; without a little more info, I would have thought that running the test with 1.3 enabled would be valuable. ::: external_tests/ssl_gtest/ssl_loopback_unittest.cc @@ +82,5 @@ > CheckConnected(); > } > > +// This test does not work in TLS 1.3 because the server will fail > +// if you attempt to renegotiate and end up with 1.3. Why do you not want to test that path then? I thought that this would still be a useful test to have anyway. If the client (or server) is reconfigured so that we end up with 1.3, then failing seems like a reasonable place to end up. Or am I misunderstanding your comment and it's that the call to SSL_ReHandshake() that fails? @@ +104,5 @@ > + client_->PrepareForRenegotiate(); > + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, > + test_version); > + server_->SetExpectedVersion(test_version); > + server_->SetExpectedCipherSuite(0); Why do you need to set the expected cipher suite? ::: lib/ssl/ssl3con.c @@ +13967,5 @@ > dtls_RehandshakeCleanup(ss); > } > > + if (ss->opt.enableRenegotiation == SSL_RENEGOTIATE_NEVER || > + ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) { yikes ::: lib/ssl/sslsecur.c @@ +99,5 @@ > ssl_preinfo_all); > (ss->handshakeCallback)(ss->fd, ss->handshakeCallbackData); > } > + > + ssl_FreeEphemeralKeyPairs(ss); We were leaking? Damn.
Attachment #8779106 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Martin Thomson [:mt:] from comment #5) > Comment on attachment 8779106 [details] [diff] [review] > 0001-Bug-1292898-Reject-SSL_ReHandshake-when-in-TLS-1.3-m.patch > > Review of attachment 8779106 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall, I don't see any problem with landing this as-is. I can't see a > security problem here. Mostly this is a case of "do stupid things, get bad > results". > > I don't understand the comment on the test though; without a little more > info, I would have thought that running the test with 1.3 enabled would be > valuable. > > ::: external_tests/ssl_gtest/ssl_loopback_unittest.cc > @@ +82,5 @@ > > CheckConnected(); > > } > > > > +// This test does not work in TLS 1.3 because the server will fail > > +// if you attempt to renegotiate and end up with 1.3. > > Why do you not want to test that path then? I thought that this would still > be a useful test to have anyway. If the client (or server) is reconfigured > so that we end up with 1.3, then failing seems like a reasonable place to > end up. > > Or am I misunderstanding your comment and it's that the call to > SSL_ReHandshake() that fails? It is now. > > @@ +104,5 @@ > > + client_->PrepareForRenegotiate(); > > + server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_0, > > + test_version); > > + server_->SetExpectedVersion(test_version); > > + server_->SetExpectedCipherSuite(0); > > Why do you need to set the expected cipher suite? > > ::: lib/ssl/ssl3con.c > @@ +13967,5 @@ > > dtls_RehandshakeCleanup(ss); > > } > > > > + if (ss->opt.enableRenegotiation == SSL_RENEGOTIATE_NEVER || > > + ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) { > > yikes > > ::: lib/ssl/sslsecur.c > @@ +99,5 @@ > > ssl_preinfo_all); > > (ss->handshakeCallback)(ss->fd, ss->handshakeCallbackData); > > } > > + > > + ssl_FreeEphemeralKeyPairs(ss); > > We were leaking? Damn.
Assignee | ||
Comment 7•8 years ago
|
||
MT, good catch. We were not handling the client-initiated upgrade side properly. We now have a fix and a test.
Attachment #8779106 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8779788 -
Flags: review?(martin.thomson)
Comment 8•8 years ago
|
||
Comment on attachment 8779788 [details] [diff] [review] 0001-Bug-1292898-Reject-SSL_ReHandshake-when-in-TLS-1.3-m.patch Review of attachment 8779788 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/ssl/ssl3con.c @@ +7116,5 @@ > goto alert_loser; > } > + /* Check that the server negotiated the same version as it did > + * in the first handshake. */ > + if (ss->firstHsDone && (version != ss->ssl3.crSpec->version)) { I definitely shouldn't be reviewing this now, but any reason you can't use ss->version, maybe before it's overwritten? I really want to remove the version from the cipher spec; it's redundant and confusing.
Assignee | ||
Comment 9•8 years ago
|
||
Yeah, the problem case is: - Client offers 1.3, server negotiates 1.2 - Client re-offers 1.3 (which you have to do for compat reasons listed in c3) and then server picks 1.2 again. I guess we could do this check right before ssl3_NegotiateVersion, though, and that would be safe.
Assignee | ||
Comment 10•8 years ago
|
||
Oh, sorry, I just remembered. ss->version is overwritten when we send the client hello, http://searchfox.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#5729
Comment 11•8 years ago
|
||
Yeah, we should probably just be using ss->vrange.max in ssl3_SendClientHello(), but that's a (small) project.
Comment 12•8 years ago
|
||
Comment on attachment 8779788 [details] [diff] [review] 0001-Bug-1292898-Reject-SSL_ReHandshake-when-in-TLS-1.3-m.patch Review of attachment 8779788 [details] [diff] [review]: ----------------------------------------------------------------- I can live with the ss->version/ssl3.crSpec->version stuff much better if you filed a bug to remove versions from the cipher specs. ::: external_tests/ssl_gtest/ssl_loopback_unittest.cc @@ +82,4 @@ > CheckConnected(); > } > > +TEST_P(TlsConnectStream, ConnectTls10AndServerRenegotiateHigher) { Would you mind moving all these tests into ssl_version_unittest.cc? @@ +139,5 @@ > + test_version); > + // Reset version and cipher suite so that the preinfo callback > + // doesn't fail. > + server_->SetExpectedVersion(0); > + server_->SetExpectedCipherSuite(0); Rather than add SetExpectedCipherSuite(), maybe just have a ResetPreInfo() function that you can call here. It will do the same thing, but with a neater name. ::: lib/ssl/ssl3con.c @@ +9083,5 @@ > } > } > + /* This is a second check for TLS 1.3 and re-handshake to stop us > + * from re-handshake up to TLS 1.3, so it happens after version > + * negotiation. */ Have you worked out how you might reach this code? Do the new tests fail if you don't include it? Not that I'm against belt and braces, but we should be sure that this code is needed.
Attachment #8779788 -
Flags: review?(martin.thomson) → review+
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8779788 [details] [diff] [review] 0001-Bug-1292898-Reject-SSL_ReHandshake-when-in-TLS-1.3-m.patch Review of attachment 8779788 [details] [diff] [review]: ----------------------------------------------------------------- ::: external_tests/ssl_gtest/ssl_loopback_unittest.cc @@ +82,4 @@ > CheckConnected(); > } > > +TEST_P(TlsConnectStream, ConnectTls10AndServerRenegotiateHigher) { Done @@ +139,5 @@ > + test_version); > + // Reset version and cipher suite so that the preinfo callback > + // doesn't fail. > + server_->SetExpectedVersion(0); > + server_->SetExpectedCipherSuite(0); Done. ::: lib/ssl/ssl3con.c @@ +9083,5 @@ > } > } > + /* This is a second check for TLS 1.3 and re-handshake to stop us > + * from re-handshake up to TLS 1.3, so it happens after version > + * negotiation. */ It's exercised here by my frobbing the server: StreamOnly/TlsConnectStream.ConnectTls10AndServerRenegotiateHigher/0 The case is you negotiate 1.x and then re-handshake to 1.3
Assignee | ||
Comment 14•8 years ago
|
||
Landed as: https://hg.mozilla.org/projects/nss/rev/7b21ff6bd8e6 Matt, please unhide.
Flags: needinfo?(mwobensmith)
Updated•8 years ago
|
Group: crypto-core-security
Flags: needinfo?(mwobensmith)
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Target Milestone: --- → 3.27
You need to log in
before you can comment on or make changes to this bug.
Description
•