Closed Bug 1242308 Opened 5 years ago Closed 5 years ago

NSS doesn't properly handle DTLS version 1.1

Categories

(NSS :: Libraries, defect)

3.18
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ekr, Assigned: ekr)

Details

Attachments

(1 file, 1 obsolete file)

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 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 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?
(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.
(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 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.
Attachment #8711755 - Flags: review?(martin.thomson)
Attachment #8711519 - Attachment is obsolete: true
MT, any reason we can't open this up?
Flags: needinfo?(martin.thomson)
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. */

//
Attachment #8711755 - Flags: review?(martin.thomson) → review+
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.
Flags: needinfo?(martin.thomson)
(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. */
> 
> //
Group: crypto-core-security
https://hg.mozilla.org/projects/nss/rev/fa49b4c30042
Assignee: nobody → ekr
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.23
MT, thanks for landing that.
You need to log in before you can comment on or make changes to this bug.