1242308 - NSS doesn't properly handle DTLS version 1.1

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Eric Rescorla (:ekr)]

There is no DTLS version 1.1. If NSS encounters it on the wire, it treats it as DTLS version 1.3, which means that it will presently negotiate the highest version of DTLS, which is generally DTLS 1.2.

Marking security sensitive because I want a double check prior to disclosure.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: 0001-Handle-bogus-DTLS-version-numbers-better.patch

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8711519 [details] [diff] [review]
0001-Handle-bogus-DTLS-version-numbers-better.patch

Review of attachment 8711519 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine.  Did you want to add a test?

Comment 3

[Wan-Teh Chang]

Comment on attachment 8711519 [details] [diff] [review]
0001-Handle-bogus-DTLS-version-numbers-better.patch

Review of attachment 8711519 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/ssl/dtlscon.c
@@ +99,4 @@
>      }
>  
>      /* Return a fictional higher version than we know of */
> +    return SSL_LIBRARY_VERSION_TLS_1_3 + 1;

Maybe we should define a macro SSL_LIBRARY_VERSION_TLS_MAX:

#define SSL_LIBRARY_VERSION_TLS_MAX SSL_LIBRARY_VERSION_TLS_1_3

and use it here, to be more future-proof?

Comment 4

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt:] from comment #2)
> Comment on attachment 8711519 [details] [diff] [review]
> 0001-Handle-bogus-DTLS-version-numbers-better.patch
> 
> Review of attachment 8711519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine.  Did you want to add a test?

Yes, I'll write a test.

Comment 5

[Eric Rescorla (:ekr)]

(In reply to Wan-Teh Chang from comment #3)
> Comment on attachment 8711519 [details] [diff] [review]
> 0001-Handle-bogus-DTLS-version-numbers-better.patch
> 
> Review of attachment 8711519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/ssl/dtlscon.c
> @@ +99,4 @@
> >      }
> >  
> >      /* Return a fictional higher version than we know of */
> > +    return SSL_LIBRARY_VERSION_TLS_1_3 + 1;
> 
> Maybe we should define a macro SSL_LIBRARY_VERSION_TLS_MAX:
> 
> #define SSL_LIBRARY_VERSION_TLS_MAX SSL_LIBRARY_VERSION_TLS_1_3
> 
> and use it here, to be more future-proof?

Yes that sounds like a good change.

Comment 6

[Eric Rescorla (:ekr)]

Attachment: 0001-Handle-bogus-DTLS-version-numbers-better.patch

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 8711755 [details] [diff] [review]
0001-Handle-bogus-DTLS-version-numbers-better.patch

Review of attachment 8711755 [details] [diff] [review]:
-----------------------------------------------------------------

Revised patch rebased onto MT's False Start patch.

Comment 8

[Eric Rescorla (:ekr)]

MT, any reason we can't open this up?

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8711755 [details] [diff] [review]
0001-Handle-bogus-DTLS-version-numbers-better.patch

Review of attachment 8711755 [details] [diff] [review]:
-----------------------------------------------------------------

Given that we're trying to get 3.22 closed out this week, let's hold off on landing this for a while.  I have a few patches that I want to land soon.

::: external_tests/ssl_gtest/ssl_loopback_unittest.cc
@@ +368,4 @@
>    CheckSrtp();
>  }
>  
> +/* Attempt to negotiate the bogus DTLS 1.1 version. */

//

Comment 10

[Martin Thomson [:mt:]]

Oh, and there is no reason not to open this.  We'll read this as DTLS 1.3 and negotiate down, since we don't support DTLS 1.3.  It would break once we actually implemented DTLS 1.3, but only to the extent that we'd try to negotiate a protocol higher than the other end can handle.  We won't implement DTLS 1.3 until after we land this patch, so we're safe.

Comment 11

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt:] from comment #9)
> Comment on attachment 8711755 [details] [diff] [review]
> 0001-Handle-bogus-DTLS-version-numbers-better.patch
> 
> Review of attachment 8711755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Given that we're trying to get 3.22 closed out this week, let's hold off on
> landing this for a while.  I have a few patches that I want to land soon.

That's fine.


> ::: external_tests/ssl_gtest/ssl_loopback_unittest.cc
> @@ +368,4 @@
> >    CheckSrtp();
> >  }
> >  
> > +/* Attempt to negotiate the bogus DTLS 1.1 version. */
> 
> //

Comment 12

[Martin Thomson [:mt:]]

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

Comment 13

[Eric Rescorla (:ekr)]

MT, thanks for landing that.