753136 - A TLS hello extension handler should be able to cause ssl3_HandleHelloExtensions to fail

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

ssl3_HandleHelloExtensions ignores the return values of TLS hello extension
handlers.  I first reported this problem in bug 537356 comment 52:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3ext.c&rev=1.6&mark=1305-1308,1313#1301

This makes it impossible for NSS to abort the handshake when it has received
an extension with a truly bad extension data.

This can be fixed as follows.

Step 1: review all the hello extension handlers and change their "return SECFailure"
statements to "return SECSuccess", unless it is a truly bad extension data.

This may make the code very confusing.  An alternative design is to still return
SECFailure, but use a special new error code to cause the handshake to be aborted.

Either way, this will a time-consuming step.  It may be best to do this in a
"pair programming" way.

Step 2: review all the call sites of ssl3_HandleHelloExtensions and make sure
we abort the handshake and send an appropriate alert message on failure (or
alternatively, on the new special error code).

Comment 1

[Eric Rescorla (:ekr)]

Wan-Teh

I tend to favor the new error code. Alternately, we might be able to set a flag value in the handshake?

Regardless, I would be happy to pair program this with you (though it should probably be your hands on the keyboard), since i was my request that prompted the reconsideration of this.

Comment 2

[Wan-Teh Chang]

Ekr: the reason for pair programming is that it's not clear to me
whether a particular bad extension data should cause the handshake
to be aborted.

A good example is a non-empty server_name extension data from the
server.  RFC 6066 Section 3. Server Name Indication says:

                                        ...  In this event, the server
   SHALL include an extension of type "server_name" in the (extended)
   server hello.  The "extension_data" field of this extension SHALL be
   empty.

Note that it does not specify what the client should do if the
extension_data is not empty.

Here is the relevant NSS client-side code:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3ext.c&rev=1.23&mark=340,349-354#338

It seems strange to simply remove that block of code, but that
may be what we're supposed to do because aborting the handshake
may be too drastic a response.

I suspect I will run into a lot of issues like this, so it
would be nice to be able to consult with you and resolve them
in real time.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #0)
> Step 1: review all the hello extension handlers and change their "return
> SECFailure"
> statements to "return SECSuccess", unless it is a truly bad extension data.
> 
> This may make the code very confusing.  An alternative design is to still
> return SECFailure, but use a special new error code to cause the handshake
> to be aborted.

I don't think we can use the second option because it throws away information that might be valuable to the caller or PR_Send/PR_Recv/etc.

For example, what if the error encountered was something like SEC_ERROR_NO_MEMORY? In that case we need to return the SEC_ERROR_NO_MEMORY error all the way back to the caller of PR_Send/PR_Recv/etc. And, for other kinds of errors, I imagine that sometimes we will implement extensions similar to Snap Start where the application may want to know the exact error code (especially if it is something like SEC_ERROR_UNKNOWN_ISSUER or SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED) so that it can implement some fallback logic (e.g. retry without the extension enabled). 

Thus, I would lean towards the first option:
1. Return SECSuccess with a comment saying we're explicitly ignoring the error, when we're ignoring an error. (Also, it may be worth logging the error especially when we return SECSuccess to ignore it.)
2. Change the ssl3_Handle*Xtn functions to themselves send alerts where alerts are appropriate.
3. Change ssl3_HandleHelloExtensions to return the value from the handler if it isn't SECSuccess.
4. Change ssl3_HandleHelloExtensions to send alerts when needed (there are already TODO comments in that function about sending alerts).
5. Change ssl3_HandleServerHello to "goto loser" instead of "goto alert_loser" when ssl3_HandleHelloExtensions fails. (ssl3_HandleClientHello already does this.)

> 
> Either way, this will a time-consuming step.  It may be best to do this in a
> "pair programming" way.
> 
> Step 2: review all the call sites of ssl3_HandleHelloExtensions and make sure
> we abort the handshake and send an appropriate alert message on failure (or
> alternatively, on the new special error code).

It seems likely that now, or in the future, we will have to send different alerts depending on what error is encountered with which extension. This makes me think that ssl3_Handle*Xtn functions themselves should send the alert, because they have the most context for determining which alert to send. (It might not be sufficient to have a mapping from SEC_ERROR_* to alert for this, because I suspect that in different extensions, or in different points within an extension, the mapping will not be a function.)

For that reason, I think that ssl3_HandleHelloExtensions, and its callers, should assume that, if an alert was needed, the ssl3_Handle*Xtn function already sent it.

(In reply to Wan-Teh Chang from comment #2)
> it's not clear to me
> whether a particular bad extension data should cause the handshake
> to be aborted.

(In reply to Wan-Teh Chang from comment #2)
> Here is the relevant NSS client-side code:
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/
> ssl3ext.c&rev=1.23&mark=340,349-354#338
> 
> It seems strange to simply remove that block of code, but that
> may be what we're supposed to do because aborting the handshake
> may be too drastic a response.

It would be confusing to remove the code that validates the extension is in the required format. It is enough to change:
     return SECFailure;
to
     return SECSuccess; /*

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

It is enough to change:
     return SECFailure;
to:
     return SECSuccess; /* Ignore malformed extension */

Comment 5

[Wan-Teh Chang]

Brian: thank you for your comments.  With your proposed change, the
code will look like this:

    if (!ss->sec.isServer) {
        /* Verify extension_data is empty. */
        if (data->data || data->len ||
            !ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn)) {
            /* malformed or was not initiated by the client.*/
            return SECSuccess; /* Ignore malformed extension */
        }
        return SECSuccess;
    }

Since the code that validates the extension has no side effect,
the above is equivalent to:

    if (!ss->sec.isServer) {
        return SECSuccess;
    }

which is the dilemma, because the extension validation is gone.
This is why I wonder if we should still return SECFailure but
set a minor error code, such as:

    if (!ss->sec.isServer) {
        /* Verify extension_data is empty. */
        if (data->data || data->len ||
            !ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn)) {
            /* malformed or was not initiated by the client.*/
            PORT_SetError(SSL_ERROR_BAD_EXTENSION_DATA_IGNORE);
            return SECFailure;
        }
        return SECSuccess;
    }

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Wan-Teh Chang from comment #5)
> Brian: thank you for your comments.  With your proposed change, the
> code will look like this:
> 
>     if (!ss->sec.isServer) {
>         /* Verify extension_data is empty. */
>         if (data->data || data->len ||
>             !ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn)) {
>             /* malformed or was not initiated by the client.*/
>             return SECSuccess; /* Ignore malformed extension */
>         }
>         return SECSuccess;
>     }

I agree that doesn't make sense. It seems like this example is more complicated, especially because the code looks buggy to me. I think it should be rewritten as:

     if (!ss->sec.isServer) {
         /* The spec. says that the extension_data "SHALL" be empty
          * but it does not say what we should do when the peer does
          * not adhere to that. Historically, we have just ignored
          * the bad extension data.
          */
         if (data->len > 0) {
             PR_LOG("Ignoring invalid ssl_server_name_xtn extension data");
         }
         ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type;
         return SECSuccess;
     }

Note that I removed !ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn) because I believe that condition is always false, and I removed data->data because it is OK for data->data to be non-NULL as long as data->len is zero, and I added ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type; because I believe that is required of all ssl3_Handle*Xtn functions.

> This is why I wonder if we should still return SECFailure but
> set a minor error code, such as:
> 
>     if (!ss->sec.isServer) {
>         /* Verify extension_data is empty. */
>         if (data->data || data->len ||
>             !ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn)) {
>             /* malformed or was not initiated by the client.*/
>             PORT_SetError(SSL_ERROR_BAD_EXTENSION_DATA_IGNORE);
>             return SECFailure;
>         }
>         return SECSuccess;
>     }

OK, that is much better than what I thought you meant. But, it is not the convention that is normally used elsewhere. Not a huge deal, but IMO it isn't clearer than returning SECSuccess with an appropriate comment in these cases.

Please see the code immediately below:

    /* Server side - consume client data and register server sender. */
    /* do not parse the data if don't have user extension handling function. */
    if (!ss->sniSocketConfig) {
        return SECSuccess;
    }

Consider the cases where we did not enable the session ticket option on the server, but the client sent an invalid session ticket extension. We ignore the invalid extension data and return SECSuccess.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

s/sent an invalid session ticket extension/
  sent an invalid server name indication extension/ in the last sentence.

Comment 9

[Martin Thomson [:mt:]]

I just discovered a need for this when looking at Bug 996250 (ALPN support).  What Brian and Wan-Teh are saying both seems to be workable.  Is there any objections to me taking a look at the extension handlers and proposing something?  Obviously, there will likely be a need for discussion on the finer points here and there, but at least I can make some sort of forward progress.

Comment 10

[Martin Thomson [:mt:]]

I want to fix this bug.

(In reply to Wan-Teh Chang from comment #5)
> Since the code that validates the extension has no side effect,
> the above is equivalent to:
> 
>     if (!ss->sec.isServer) {
>         return SECSuccess;
>     }
> 
> which is the dilemma, because the extension validation is gone.
> This is why I wonder if we should still return SECFailure but
> set a minor error code, such as:
> 
>     if (!ss->sec.isServer) {
>         /* Verify extension_data is empty. */
>         if (data->data || data->len ||
>             !ssl3_ExtensionNegotiated(ss, ssl_server_name_xtn)) {
>             /* malformed or was not initiated by the client.*/
>             PORT_SetError(SSL_ERROR_BAD_EXTENSION_DATA_IGNORE);
>             return SECFailure;
>         }
>         return SECSuccess;
>     }

This is probably a good point to use as a flagship for deciding how to fix this.  My opinion is that if we aren't actually surfacing an error, there is no point validating extensions.  Currently, things seem to work fine.  Bad extensions are ignored.  

Here's a somewhat radical plan to move things along:

1. Turn all SECFailure instances in handlers into SECSuccess.  That might mean losing some information, but for the first run, the multiple exit points can be preserved.
2. ssl3_HandleHelloExtensions can be made to fail if a handler returns SECFailure.  That won't happen.
3. With care, examine all the lines changed in step 1 and do one of the following:
  a. Switch to SECFailure, forcing the handshake to abort.
  b. Collapse the logic, as with the above example.
4. Look at providing some form of feedback when extensions are bad in some way.  This could be extra logging, new error or warning code or something else.

Comment 11

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_HandleServerNameXtn (leaving bug 998524 alone)

Comment 12

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_ClientHandleNextProtoNegoXtn

Comment 13

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_ClientHandleAppProtoXtn

Comment 14

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_ClientHandleStatusRequestXtn

Comment 15

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_ClientHandleSessionTicketXtn

Comment 16

[Martin Thomson [:mt:]]

Attachment: ssl3_ServerHandleSessionTicketXtn

Comment 17

[Martin Thomson [:mt:]]

Attachment: Whitespace cleanup ssl3ecc.c

Comment 18

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_HandleSupportedCurvesXtn

Comment 19

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_HandleSupportedPointFormatsXtn

Comment 20

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_HandleSupportedPointFormatsXtn

Comment 21

[Martin Thomson [:mt:]]

Attachment: Fixing ssl3_ServerHandleSigAlgsXtn

Comment 22

[Martin Thomson [:mt:]]

It's late now, so I'm going to leave off the analysis.  I haven't switched over ssl3_HandleHelloExtensions yet, but this here includes the changes to the different handlers.  Here are the notes that I took during this:

General:
   - functions that use ssl3_ConsumeHandshakeNumber cause a
     fatal alert to be sent, we have to fail the handshake then
   - this does lead to a number of cases where we should be
     failing the handshake with a fatal decode_error alert, 
     but those can be switched later to
        return ssl3_DecodeError(ss);

ssl3_ClientHandleAppProtoXtn
   - If NPN is negotiated, ignore the extension
   - If the length is out of range, ignore the extension
   - If the sub-lengths are invalid, ignore the extension
   - If an allocation fails, that's fatal

ssl3_ClientHandleNextProtoNegoXtn
   - If ALPN is negotiated, ignore the extension
      (note that if we send both ALPN and NPN, and the server does too,
       that results in neither being used)
   - If the extension is invalid, ignore the extension
   - If we don't have a handler, ignore the extension
   - If the handler fails, ignore the extension
   - If the handler overruns a buffer, that's fatal
   - If an allocation fails, that's fatal

ssl3_ClientHandleSessionTicketXtn
   - If the extension contains data, ignore the extension
   - Duplicates ssl3_ClientHandleStatusRequestXtn

ssl3_ClientHandleStatusRequestXtn
   - If the extension contains data, ignore the extension
   - Duplicates ssl3_ClientHandleSessionTicketXtn

ssl3_HandleRenegotiationInfoXtn
   - Leaving this as fatal; there's a fatal alert being sent
     and this needs to bail or we have a problem

ssl3_HandleServerNameXtn
   - calls in to functions that generate fatal alerts cause
     SECFailure, otherwise it's SECSuccess
   - [BUG] type handling is busted: bug 998524

ssl3_HandleSupportedCurvesXtn
   - Two uses of ssl3_ConsumeHandshakeNumber: fatal
   - otherwise, we disable ECC if things don't work out, but
     the handshake can still proceed.

ssl3_HandleSupportedPointFormatsXtn
   - Leaving a failure to register an extension sender as fatal
   - If the data is malformed, ignore the extension
   - If the formats are not supported, ignore the extension
   - [BUG] a length of 256 is valid, but rejected

ssl3_HandleUseSRTPXtn
   - Lots of failed checks cause the extension to be ignored
   - Fail on ssl3_ConsumeHandshakeNumber failures
   - Fail on registering a hello sender
   - Send abort and fail for mismatch on MKI
   - [BUG] needs to be split into two separate handlers,
      since neither shares code

ssl3_ServerHandleNextProtoNegoXtn
   - Not really implemented, leaving this lame

ssl3_ServerHandleSessionTicketXtn
   - Only one change:
   - If the extension doesn't parse, ignore the extension
   - Unable to generate session ticket keys is still fatal
   - As is an inability to allocate
   - No new session ID indicates that we either have a busted
     allocator or RNG: fatal
   - Not sure about the certificate thing, but it looks like
     most of the errors are allocator issues: fatal

ssl3_ServerHandleSigAlgsXtn
   - Fatal ssl3_ConsumeHandshakeNumber instance
   - malformed, ignore
   - fatal on allocation failure

ssl3_ServerHandleStatusRequestXtn
   - no change, fail if sender can't be registered

Comment 23

[Martin Thomson [:mt:]]

Attachment: 0008-Bug-753136-Fixing-ssl3_HandleUseSRTPXtn.patch

Fixing mislabeled patch.

Comment 24

[Martin Thomson [:mt:]]

Attachment: 0013-Bug-753136-Hard-fail-on-extension-failures.patch

Hard failure on extension error.  Allows for some limited failures in handshakes based on the content of extensions.

Comment 25

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-753136-Whitespace-cleanup.patch

diff -w should come back clean with this patch.

I've run untabify over the files I touched and lined things up again using hard spaces.  There was a lot of trailing whitespace.

Comment 26

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-753136-Fixing-ssl3_HandleServerNameXtn-leaving-b.patch

Notes in Comment 22.

Note: Other than the first and last, these patches should apply safely in any order.

Comment 27

[Martin Thomson [:mt:]]

Attachment: 0003-Bug-753136-Fixing-ssl3_ClientHandleNextProtoNegoXtn.patch

Comment 28

[Martin Thomson [:mt:]]

Attachment: 0004-Bug-753136-Fixing-ssl3_ClientHandleAppProtoXtn.patch

Comment 29

[Martin Thomson [:mt:]]

Attachment: 0005-Bug-753136-Fixing-ssl3_ClientHandleStatusRequestXtn.patch

Comment 30

[Martin Thomson [:mt:]]

Attachment: 0006-Bug-753136-Fixing-ssl3_ClientHandleSessionTicketXtn.patch

Comment 31

[Martin Thomson [:mt:]]

Attachment: 0007-Bug-753136-Fixing-ssl3_ServerHandleSessionTicketXtn.patch

Comment 32

[Martin Thomson [:mt:]]

Attachment: 0008-Bug-753136-Fixing-ssl3_HandleUseSRTPXtn.patch

Comment 33

[Martin Thomson [:mt:]]

Attachment: 0009-Bug-753136-Fixing-ssl3_ServerHandleSigAlgsXtn.patch

Comment 34

[Martin Thomson [:mt:]]

Attachment: 0010-Bug-753136-Fixing-ssl3_HandleSupportedCurvesXtn.patch

Comment 35

[Martin Thomson [:mt:]]

Attachment: 0011-Bug-753136-Fixing-ssl3_HandleSupportedPointFormatsXt.patch

Comment 36

[Martin Thomson [:mt:]]

Attachment: 0012-Bug-753136-Hard-fail-on-extension-failures.patch

This should apply cleanly without anything else, but it probably shouldn't be applied until everything else is applied.

Comment 37

[Wan-Teh Chang]

Comment on attachment 8412799 [details] [diff] [review]
0001-Bug-753136-Whitespace-cleanup.patch

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

Martin:

Thank you for the patch. In the past we've avoided such whitespace changes
because they obscure revision history. But many people who need to modify
NSS asked me whether they need to use tabs, so I am now willing to make
such changes.

I found that in two files you did not replace all the tabs by spaces (one
of them is sslsock.c). Was that an oversight or did you intend to preserve
the tabs in those two files?

A few lines were incorrectly indented. I will fix those for you.

Comment 38

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-753136-Whitespace-cleanup.patch

I don't know if this is better or worse.

A few other parts of the gecko tree are running astyle uniformly across files.  Maybe that's a better approach than this sort of piecemeal one.

Comment 39

[Wan-Teh Chang]

Attachment: 0001-Bug-753136-Whitespace-cleanup.patch by Martin Thomson (as checked in)

Martin: yes, attachment 8419511 [details] [diff] [review] is much better. Thanks.

I fixed the few remaining indentation problems and checked it in:
https://hg.mozilla.org/projects/nss/rev/eb1beae9ffc4

Comment 40

[Martin Thomson [:mt:]]

I just ran the tool from https://github.com/mozilla/pkix-compatibility-testing against a build with these patches rebased to the latest gecko-dev.

The tests have to be run iteratively due to the way that we deal with intermediate CA certificates.

The following 10/211096 domains continue to react differently between the two builds:

These 7 only apply with the new code:

-home.toku-talk.com 0x805a2ff4 Unable to communicate securely with peer: requested domain name does not match the server's certificate.
-sealinfo.geotrust.com 0x805a1ff3 Peer's Certificate issuer is not recognized.
-www.colombotelegraph.com 0x8000ffff
-www.mattressnextday.co.uk 0x805a1ff3 Peer's Certificate issuer is not recognized.
-www.hagenhosting.com 0x8000ffff
-www.obesityhelp.com 0x805a1ff5 Peer's Certificate has expired.
-www.stromvergleich.de 0x805a2ff4 Unable to communicate securely with peer: requested domain name does not match the server's certificate.

These 3 with the old code:

+www.auctionflex.com 0x8000ffff
+www.fleaflicker.com 0x804b0014
+www.visionsolutions.com 0x8000ffff

Comment 41

[Martin Thomson [:mt:]]

Wan-Teh, do you think that you would have time to look at this in the next few weeks?

I've found that I need this for that other bug, and it also looks like it would be good if we could fail the handshake for session-hash too (at least optionally).

Comment 42

[Martin Thomson [:mt:]]

Attachment: MozReview Request: bz://753136/mt

/r/3217 - Bug 753136 - Fixing ssl3_HandleServerNameXtn (leaving bug 998524 alone)
/r/3219 - Bug 753136 - Fixing ssl3_ClientHandleNextProtoNegoXtn
/r/3221 - Bug 753136 - Fixing ssl3_ClientHandleAppProtoXtn
/r/3223 - Bug 753136 - Fixing ssl3_ClientHandleStatusRequestXtn
/r/3225 - Bug 753136 - Fixing ssl3_ClientHandleSessionTicketXtn
/r/3227 - Bug 753136 - Fixing ssl3_ServerHandleSessionTicketXtn
/r/3229 - Bug 753136 - Fixing ssl3_HandleUseSRTPXtn
/r/3231 - Bug 753136 - Fixing ssl3_ServerHandleSigAlgsXtn
/r/3233 - Bug 753136 - Fixing ssl3_HandleSupportedCurvesXtn
/r/3235 - Bug 753136 - Fixing ssl3_HandleSupportedPointFormatsXtn
/r/3237 - Bug 753136 - Hard fail on extension failures

Pull down these commits:

hg pull review -r e3ccfdaeb392655c1814dd781432aa7d43327ef0

Comment 43

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/3237/#review2649

::: security/nss/lib/ssl/ssl3ext.c
(Diff revision 1)
> -                /* Treat all bad extensions as unrecognized types. */
> +                    return rv;

The impact of this change is that the caller will send an alert
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#6396
which means that if the extension handler generates an alert, you now send two alerts. I think you need some way to suppress double-sending

To make matters worse, it looks like other times we don't get an alert:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#7785

Comment 44

[Martin Thomson [:mt:]]

The thinking here was that sending two alerts was less detrimental than sending a fatal alert followed by a ServerHello.

You will note that the client sends an alert, but the server does not.  Fun times, eh?

Maybe the most user-friendly way to deal with this is to tag the session with a 'fatal alert sent' flag when an alert is sent.  Then it can avoid sending two.

Comment 45

[Eric Rescorla (:ekr)]

I see a lot of places where you have been very conservative about failing
the handshake. For instance, let's take a look at:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3ecc.c#1237

    if (list_len < 0 || data->len != list_len || (data->len % 2) != 0) {
        /* malformed */
        goto loser;
    }

The impact of this is that you just try to disable ECC if anything is
malformed. However, as you observe in your patch, because list_len < 0
is a decode error, a fatal alert has already been sent. In the other
conditions, ECC is disabled and the handshake continues. I would argue
that we should fail here.

I realize this would be a change to the behavior of NSS, which has tended
to just try to recover from these, but I wonder how much that's a result
of not being able to fail in extension handlers. I would argue that generally
if extensions are malformed, we should be failing the handshake, absent
some evidence that there are otherwise valid clients or servers who would
be affected.

Bob, Wan-Teh, what do you think?

Comment 46

[Eric Rescorla (:ekr)]

(In reply to Martin Thomson [:mt] from comment #44)
> The thinking here was that sending two alerts was less detrimental than
> sending a fatal alert followed by a ServerHello.
> 
> You will note that the client sends an alert, but the server does not.  Fun
> times, eh?
> 
> Maybe the most user-friendly way to deal with this is to tag the session
> with a 'fatal alert sent' flag when an alert is sent.  Then it can avoid
> sending two.

I would be in favor of that.

Comment 47

[Wan-Teh Chang]

(In reply to Eric Rescorla (:ekr) from comment #45)
> 
> ... I would argue that generally
> if extensions are malformed, we should be failing the handshake, absent
> some evidence that there are otherwise valid clients or servers who would
> be affected.
> 
> Bob, Wan-Teh, what do you think?

This is fine by me. This will make the extension handling code easier to
understand, avoiding the counter-intuitive code that returns SECSuccess
on extension parsing errors.

Note: I'm going to review the patch for ssl3_ClientHandleNextProtoNegoXtn
in favor of watching the football game.

Comment 48

[Wan-Teh Chang]

Attachment: 0003-Bug-753136-Fixing-ssl3_ClientHandleNextProtoNegoXtn.patch, v2

I updated Martin's patch to the current code. I also made
two changes to Martin's patch (marked below). I added an
"Ignore the extension" comment so that we can easily identify
the places to change if we want to fail the handshake in the
future.

Here are Martin's notes about the patch, updated:
ssl3_ClientHandleNextProtoNegoXtn
   - If ALPN is negotiated, that's fatal because this is
     a server error, similar to sending more than one
     extension of the same type. [a change to Martin's
     patch]
   - If the extension is invalid, ignore the extension
   - If we don't have a handler, that's fatal [a change
     to Martin's patch]
   - If the handler fails, ignore the extension
   - If the handler overruns a buffer, that's fatal
   - If an allocation fails, that's fatal

Comment 49

[Wan-Teh Chang]

Attachment: 0003-Bug-753136-Fixing-ssl3_ClientHandleNextProtoNegoXtn.patch, v2.1

I regenerated the patch with function names.

Also, I propose we fail the handshake with an illegal_parameter
alert if the peer sends more than one extension of the same type.
TLS 1.2 (RFC 5246) merely says "MUST NOT", without specifying
which alert should be sent. illegal_parameter is the closest
alert I can find.

Comment 50

[Wan-Teh Chang]

Attachment: 0004-Bug-753136-Fixing-ssl3_ClientHandleAppProtoXtn.patch, v2

I changed Martin's patch in one place, noted below.

Here are Martin's notes about this patch, updated:
ssl3_ClientHandleAppProtoXtn
   - If NPN is negotiated, that's fatal because this is
     a server error, similar to sending more than one
     extension of the same type. [a change to Martin's
     patch]
   - If the length is out of range, ignore the extension
   - If the sub-lengths are invalid, ignore the extension
   - If an allocation fails, that's fatal

Comment 51

[Martin Thomson [:mt:]]

Comment on attachment 8557700 [details] [diff] [review]
0004-Bug-753136-Fixing-ssl3_ClientHandleAppProtoXtn.patch, v2

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

Wan-Teh, you are too good. I'm about to get on a plane, so don't do too much before I get a chance. :)

I was thinking that I might generate a few fatal alerts, in line with my comment before.

Comment 52

[Wan-Teh Chang]

Attachment: 0005-Bug-753136-Fixing-ssl3_ClientHandleStatusRequestXtn.patch, v2

Patch checked in:
https://hg.mozilla.org/projects/nss/rev/9bc7e2608fd6

Comment 53

[Wan-Teh Chang]

Attachment: 0006-Bug-753136-Fixing-ssl3_ClientHandleSessionTicketXtn.patch, v2

Patch checked in:
https://hg.mozilla.org/projects/nss/rev/775ef8656605

Comment 54

[Martin Thomson [:mt:]]

Comment on attachment 8557697 [details] [diff] [review]
0003-Bug-753136-Fixing-ssl3_ClientHandleNextProtoNegoXtn.patch, v2.1

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

::: lib/ssl/ssl3ext.c
@@ +627,5 @@
>  
>      rv = ssl3_ValidateNextProtoNego(data->data, data->len);
> +    if (rv != SECSuccess) {
> +        return SECSuccess;  /* Ignore the extension. */
> +    }

I think that here we should be returning rv, and failing the handshake if the extension is not valid.  I didn't do that originally, but :ekr is right, tolerating bad extensions isn't good for the ecosystem, especially when things like this have default behaviour that is consequently triggered.

e.g., we pick a default protocol for WebRTC in the absence of ALPN and this will make it appear as though the peer doesn't support ALPN.

Comment 55

[Martin Thomson [:mt:]]

Comment on attachment 8412803 [details] [diff] [review]
0002-Bug-753136-Fixing-ssl3_HandleServerNameXtn-leaving-b.patch

Marking these as obsolete now that I'm spending more time on fixing them up.  I'll complete the review process on rietveld and upload whatever gets approved there.

Comment 56

[Martin Thomson [:mt:]]

Comment on attachment 8557697 [details] [diff] [review]
0003-Bug-753136-Fixing-ssl3_ClientHandleNextProtoNegoXtn.patch, v2.1

Wan-Teh, I've marked this as obsolete since I've gone to the effort of adding the alerts that you marked with TODO.  I've also been a little more aggressive about failing the handshake too.

Comment 57

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/3223/#review4433

::: security/nss/lib/ssl/ssl3ext.c
(Diff revision 2)
> +static SECStatus ssl3_ClientHandleUseSRTPXtn(sslSocket * ss, PRUint16 ex_type,
> +                                             SECItem *data);
> +static SECStatus ssl3_ServerHandleUseSRTPXtn(sslSocket * ss, PRUint16 ex_type,
> -    SECItem *data);
> +                                             SECItem *data);

This is bad.

Comment 58

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/3223/#review4435

Ship It!

Comment 59

[Martin Thomson [:mt:]]

Comment on attachment 8557634 [details]
MozReview Request: bz://753136/mt

/r/3217 - Bug 753136 - Fixing ssl3_HandleServerNameXtn
/r/3219 - Bug 753136 - Fixing ssl3_ClientHandleNextProtoNegoXtn and ssl3_ClientHandleAppProtoXtn
/r/3221 - Bug 753136 - Fixing ssl3_ServerHandleSessionTicketXtn
/r/3223 - Bug 753136 - Fixing ssl3_HandleUseSRTPXtn
/r/3225 - Bug 753136 - Fixing ssl3_ServerHandleSigAlgsXtn
/r/3227 - Bug 753136 - Fixing ssl3_HandleSupportedCurvesXtn
/r/3229 - Bug 753136 - Fixing ssl3_HandleSupportedPointFormatsXtn
/r/3231 - Bug 753136 - Fixing ssl3_ServerHandleDraftVersionXtn
/r/3233 - Bug 753136 - Fix ssl3_HandleRenegotiationInfoXtn
/r/3235 - Bug 753136 - Hard fail on extension failures
/r/3237 - Bug 753136 - Track when alerts are sent and send a catch-all alert on extension failures
/r/5497 - Bug 753136 - More extensive extension exercising

Pull down these commits:

hg pull review -r dd578b71e3075e501210b58a8434fec5eda5d8da

Comment 60

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/3223/#review4517

::: security/nss/lib/ssl/ssl3ext.c
(Diff revision 2)
> +        (void)SSL3_SendAlert(ss, alert_fatal, illegal_parameter);

This needs a PORT_SetError() or call ssl3_DecodeError(ss)

Comment 61

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/1a0d3cf19ba0
https://hg.mozilla.org/projects/nss/rev/2fef116cbe98
https://hg.mozilla.org/projects/nss/rev/d09c7af0f9c0
https://hg.mozilla.org/projects/nss/rev/9beaa95036fc
https://hg.mozilla.org/projects/nss/rev/1b11045d1472
https://hg.mozilla.org/projects/nss/rev/b4bd63404b82
https://hg.mozilla.org/projects/nss/rev/b2fae9d52e14
https://hg.mozilla.org/projects/nss/rev/9dde22ab19d1

Comment 62

[Martin Thomson [:mt:]]

Comment on attachment 8557634 [details]
MozReview Request: bz://753136/mt

Clearing since this already landed.

Comment 64

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fix ssl3_HandleRenegotiationInfoXtn

Comment 65

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Hard fail on extension failures

Comment 66

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Track when alerts are sent and send a catch-all alert on extension failures

Comment 67

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - More extensive extension exercising

Comment 68

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_HandleServerNameXtn

Comment 69

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_ClientHandleNextProtoNegoXtn and ssl3_ClientHandleAppProtoXtn

Comment 70

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_ServerHandleSessionTicketXtn

Comment 71

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_HandleUseSRTPXtn

Comment 72

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_ServerHandleSigAlgsXtn

Comment 73

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_HandleSupportedCurvesXtn

Comment 74

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_HandleSupportedPointFormatsXtn

Comment 75

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 753136 - Fixing ssl3_ServerHandleDraftVersionXtn

Comment 76

[Eric Rescorla (:ekr)]

Comment on attachment 8557700 [details] [diff] [review]
0004-Bug-753136-Fixing-ssl3_ClientHandleAppProtoXtn.patch, v2

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

Thisappears to be OBE