1149850 - Disable DTLS Renegotiation

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

As we've discovered today, renegotiation doesn't work in DTLS.  It could be fixed, probably with not a great deal of work, but it's obvious no one uses it.

There is a little bit of dead code to remove as a result, but it's probably not that much.

Comment 1

[Eric Rescorla (:ekr)]

Restricting access until we can evaluate.

Comment 2

[Eric Rescorla (:ekr)]

Attachment: 0001-Remove-renegotiation-for-DTLS.patch

Comment 3

[Eric Rescorla (:ekr)]

Wan-Teh,

After discussing this with Martin, we concluded that since we are
deprecating renegotiation, we should not put in the effort to fix
the bug he found where it didn't work with DTLS. As far as I know,
nobody uses it with DTLS.

Any objections?

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8587055 [details] [diff] [review]
0001-Remove-renegotiation-for-DTLS.patch

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

This looks fine.  I think that we should a) default the option to NEVER and b) prevent the option from ever being set to something other than NEVER.

Comment 5

[Wan-Teh Chang]

Comment on attachment 8587055 [details] [diff] [review]
0001-Remove-renegotiation-for-DTLS.patch

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

::: external_tests/ssl_gtest/tls_agent.h
@@ +86,4 @@
>    // Marks the state as CONNECTING in anticipation of renegotiation.
>    void PrepareRenegotiate();
>    // Prepares for renegotiation, then actually triggers it.
> +  void StartRenegotiate(bool expect_success=true);

It is best to avoid the default argument. I assume it is a new C feature.

::: lib/ssl/ssl3con.c
@@ +5402,4 @@
>  	PORT_SetError(SSL_ERROR_RX_UNEXPECTED_HELLO_REQUEST);
>  	return SECFailure;
>      }
> +    if (IS_DTLS(ss) || ss->opt.enableRenegotiation == SSL_RENEGOTIATE_NEVER) {

Please find a convenient place to document that renegotiation is not supported in DTLS, and why (nobody needs it).

Comment 6

[Wan-Teh Chang]

(In reply to Eric Rescorla (:ekr) from comment #3)
> Wan-Teh,
> 
> After discussing this with Martin, we concluded that since we are
> deprecating renegotiation, we should not put in the effort to fix
> the bug he found where it didn't work with DTLS. As far as I know,
> nobody uses it with DTLS.
> 
> Any objections?

This is fine by me if nobody needs it. This could become a
chicken-and-egg problem though and prevent others from
using renegotiation in DTLS.

Comment 7

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt] from comment #4)
> Comment on attachment 8587055 [details] [diff] [review]
> 0001-Remove-renegotiation-for-DTLS.patch
> 
> Review of attachment 8587055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine.  I think that we should a) default the option to NEVER and
> b) prevent the option from ever being set to something other than NEVER.

I assume you mean in DTLS?

Comment 8

[Eric Rescorla (:ekr)]

(In reply to Wan-Teh Chang from comment #6)
> (In reply to Eric Rescorla (:ekr) from comment #3)
> > Wan-Teh,
> > 
> > After discussing this with Martin, we concluded that since we are
> > deprecating renegotiation, we should not put in the effort to fix
> > the bug he found where it didn't work with DTLS. As far as I know,
> > nobody uses it with DTLS.
> > 
> > Any objections?
> 
> This is fine by me if nobody needs it. This could become a
> chicken-and-egg problem though and prevent others from
> using renegotiation in DTLS.

My sense is that we actually want to prevent them doing so.

Comment 9

[Eric Rescorla (:ekr)]

(In reply to Wan-Teh Chang from comment #5)
> Comment on attachment 8587055 [details] [diff] [review]
> 0001-Remove-renegotiation-for-DTLS.patch
> 
> Review of attachment 8587055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: external_tests/ssl_gtest/tls_agent.h
> @@ +86,4 @@
> >    // Marks the state as CONNECTING in anticipation of renegotiation.
> >    void PrepareRenegotiate();
> >    // Prepares for renegotiation, then actually triggers it.
> > +  void StartRenegotiate(bool expect_success=true);
> 
> It is best to avoid the default argument. I assume it is a new C feature.

These tests are in C++. However, I can remove the default argument
if you prefer.


> ::: lib/ssl/ssl3con.c
> @@ +5402,4 @@
> >  	PORT_SetError(SSL_ERROR_RX_UNEXPECTED_HELLO_REQUEST);
> >  	return SECFailure;
> >      }
> > +    if (IS_DTLS(ss) || ss->opt.enableRenegotiation == SSL_RENEGOTIATE_NEVER) {
> 
> Please find a convenient place to document that renegotiation is not
> supported in DTLS, and why (nobody needs it).

Willdo.

Comment 10

[Eric Rescorla (:ekr)]

Martin,

I took a look at what it would take to nail the option to NEVER.

Unfortunately, the difficulty is that the options are not conditioned
on the variant. See, for instance:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslsock.c#886

We could override the defaults in the socket, e.g., here:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/sslsock.c#2945

This would mean that you could get:
SSL_GetDefault(SSL_ENABLE_RENEGOTIATION, ...) --> SSL_RENEGOTIATE_REQUIRES_XTN

and then
fd = ssl_NewSocket(..., DTLS)
SSL_OptionGet(fd, SSL_ENABLE_RENEGOTIATION, ...) --> SSL_RENEGOTIATE_NEVER

Would you prefer that?

Comment 11

[Wan-Teh Chang]

Comment on attachment 8587055 [details] [diff] [review]
0001-Remove-renegotiation-for-DTLS.patch

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

::: lib/ssl/ssl3con.c
@@ +5402,4 @@
>  	PORT_SetError(SSL_ERROR_RX_UNEXPECTED_HELLO_REQUEST);
>  	return SECFailure;
>      }
> +    if (IS_DTLS(ss) || ss->opt.enableRenegotiation == SSL_RENEGOTIATE_NEVER) {

Eric, Martin: it may be a cleaner change to simply force ss->opt.enableRenegotiation to be SSL_RENEGOTIATE_NEVER for all DTLS sockets. What do you think? This seems to be what Eric suggested in the NSS conference call this morning. I'm sorry I was a little absent-minded when Eric talked about this bug.

Comment 12

[Martin Thomson [:mt:]]

I think that matches what I suggested.  That avoids those three extra checks.

Comment 13

[Eric Rescorla (:ekr)]

I will do that.

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking sec-audit. When can we make this bug public?

Comment 15

[Martin Thomson [:mt:]]

Public is fine with me.  ekr?

Comment 16

[Eric Rescorla (:ekr)]

Let's have a patch first. I'll try to get to that this week.

Comment 17

[Martin Thomson [:mt:]]

Attachment: bug1149850-1.patch

Found this old bug, seemed relatively important.

Comment 18

[Wan-Teh Chang]

Comment on attachment 8757178 [details] [diff] [review]
bug1149850-1.patch

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

::: lib/ssl/sslsock.c
@@ +736,5 @@
>          case SSL_ENABLE_RENEGOTIATION:
> +            if (IS_DTLS(ss) && on != SSL_RENEGOTIATE_NEVER) {
> +                PORT_SetError(SEC_ERROR_INVALID_ARGS);
> +                rv = SECFailure;
> +                on = SSL_RENEGOTIATE_NEVER;

It seems that a failed function should have no side
effect. If you want to be secure by default, then we
should initialize ss->opt.enableRenegotiation to
SSL_RENEGOTIATE_NEVER for DTLS (which I think you did
below). So this line should be replaced with "break".

Comment 19

[Martin Thomson [:mt:]]

Attachment: bug1149850-1.patch

The net effect is the same.

Comment 20

[Martin Thomson [:mt:]]

ekr, ping

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 8757665 [details] [diff] [review]
bug1149850-1.patch

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

LGTM

Comment 22

[Martin Thomson [:mt:]]

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