NSS doesn't properly handle DTLS version 1.1

RESOLVED FIXED in 3.23

Status

NSS
Libraries
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ekr, Assigned: ekr)

Tracking

3.18
3.23

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8711519 [details] [diff] [review]
0001-Handle-bogus-DTLS-version-numbers-better.patch
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

2 years ago
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?
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
Created attachment 8711755 [details] [diff] [review]
0001-Handle-bogus-DTLS-version-numbers-better.patch
(Assignee)

Comment 7

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8711519 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
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)
(Assignee)

Comment 11

2 years ago
(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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.23
(Assignee)

Comment 13

2 years ago
MT, thanks for landing that.
You need to log in before you can comment on or make changes to this bug.