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)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ekr, Assigned: ekr)

Details

Attachments

(1 file, 1 obsolete file)

This currently asserts in tls13_SetupClientHello but might continue in release mode. If this can complete, it might be a security issue.
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
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: nobody → ekr
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 )
Attachment #8779106 - Flags: review?(martin.thomson)
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+
(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.
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
Attachment #8779788 - Flags: review?(martin.thomson)
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.
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.
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
Yeah, we should probably just be using ss->vrange.max in ssl3_SendClientHello(), but that's a (small) project.
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+
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
Landed as:
https://hg.mozilla.org/projects/nss/rev/7b21ff6bd8e6

Matt, please unhide.
Flags: needinfo?(mwobensmith)
Group: crypto-core-security
Flags: needinfo?(mwobensmith)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: