996250 - Implement server-side components of ALPN (RFC 7301)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Martin Thomson [:mt:]]

WebRTC is going to need NSS to support ALPN on both sides of a connection, not just one.  Bug 996238 describes why this is.

Comment 1

[Martin Thomson [:mt:]]

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

Prep work.  I don't know what the whitespace conventions on NSS are, these were so inconsistent.  These files are now consistent.

Comment 2

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-996250-Server-side-support-for-ALPN.patch

Server side support of ALPN.

Until I did this, I didn't realize that it would need to depend on Bug 753136.  This patch doesn't fail hard, though it does send a fatal alert as ALPN requires.  What is bad about this is that it also sends a ServerHello.

That's clearly bad, particularly since NSS completes the handshake afterwards.  I think that this will have to depend on Bug 753136.  Is that right?  Or can I somehow force an abort from down in a hello extension handler?

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8407820 [details]
0001-Bug-996250-Whitespace-cleanup.patch

These are crazy-bad.  I have no idea how my limited testing was showing passing results.  I've something closer to good now, but will need a little more time.

Comment 4

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-996250-Server-side-support-for-ALPN.patch

OK, the last one was completely bad.  This works and has been tested many different ways.  Tests are in bug 996238 if you feel crazy enough to run that stuff.

Comment 5

[Wan-Teh Chang]

Attachment: 0001-Bug-996250-Server-side-support-for-ALPN.patch by Martin Thomson, v1

This is Martin's patch (attachment 8412825 [details] [diff] [review]) applied to the current
NSS trunk.

I'll suggest some changes to Martin's patch in the next attachment.
Please diff that patch against this patch to see my suggested changes.

Comment 6

[Wan-Teh Chang]

Attachment: 0001-Bug-996250-Server-side-support-for-ALPN.patch by Martin Thomson, v2

Martin: please review the diffs between this patch and the v1 patch.
Thanks.

Comment 7

[Wan-Teh Chang]

Comment on attachment 8432918 [details] [diff] [review]
0001-Bug-996250-Server-side-support-for-ALPN.patch by Martin Thomson, v2

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

Martin: I annotated some of my suggested changes. Other changes are
just to fix coding style nits such as folding lines longer than 80
characters.

::: lib/ssl/SSLerrs.h
@@ +416,5 @@
> +ER3(SSL_ERROR_NEXT_PROTOCOL_NO_CALLBACK, (SSL_ERROR_BASE + 129),
> +"The application cleared the next protocol callback between the time we sent the ClientHello and the time we received the ServerHello.")
> +
> +ER3(SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL, (SSL_ERROR_BASE + 130),
> +"The next protocol callback picked a default protocol for ALPN.")

Please review the error messages I wrote for the two new error codes.
These are based on the comments in the source code where we set these
error codes. I'm not very happy with them.

::: lib/ssl/ssl3ext.c
@@ +63,4 @@
>  static PRInt32 ssl3_ClientSendAppProtoXtn(sslSocket *ss, PRBool append,
>                                            PRUint32 maxBytes);
> +static PRInt32 ssl3_ServerSendAppProtoXtn(sslSocket *ss, PRBool append,
> +                                          PRUint32 maxBytes);

I reordered some of the function declarations so that they match the
order of the function definitions better.

@@ +614,2 @@
>  static SECStatus
> +ssl3_SelectAppProtocol(sslSocket *ss, PRUint16 ex_type, SECItem *data)

I renamed this function from ssl3_PerformAppProtocolSelection to ssl3_SelectAppProtocol.

I removed the "PRBool isNPN" parameter because we can just test
ex_type == ssl_app_layer_protocol_xtn.

@@ +635,5 @@
> +    }
> +
> +    if (ex_type == ssl_app_layer_protocol_xtn &&
> +        ss->ssl3.nextProtoState != SSL_NEXT_PROTO_NEGOTIATED) {
> +        /* The callback might say OK, but then it's picked a default.

This comment used to say "The handler". I changed that to "The callback".

@@ +656,5 @@
> +{
> +    int count;
> +    SECStatus rv;
> +
> +    if (ss->firstHsDone || data->len == 0) {

Please confirm that you want to disallow ALPN in renegotiations. (I seem
to remember the ALPN spec allows that.) This is fine by me. May be worth
a comment.

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8432918 [details] [diff] [review]
0001-Bug-996250-Server-side-support-for-ALPN.patch by Martin Thomson, v2

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

These look good.

::: lib/ssl/SSLerrs.h
@@ +416,5 @@
> +ER3(SSL_ERROR_NEXT_PROTOCOL_NO_CALLBACK, (SSL_ERROR_BASE + 129),
> +"The application cleared the next protocol callback between the time we sent the ClientHello and the time we received the ServerHello.")
> +
> +ER3(SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL, (SSL_ERROR_BASE + 130),
> +"The next protocol callback picked a default protocol for ALPN.")

I'm not sure that these are particularly good ones.

Maybe: 
"The next protocol negotiation extension was enabled, but the callback was cleared prior to being needed."

The second one is as good as I can think of.

The only other thing I can think of to do here is merge the two into a single code, NPN_CALLBACK_ERROR, which could encompass all classes of error related to the callback: bad configuration, bad responses, etc...

::: lib/ssl/ssl3ext.c
@@ +614,2 @@
>  static SECStatus
> +ssl3_SelectAppProtocol(sslSocket *ss, PRUint16 ex_type, SECItem *data)

Both changes are good.

@@ +656,5 @@
> +{
> +    int count;
> +    SECStatus rv;
> +
> +    if (ss->firstHsDone || data->len == 0) {

Yes, I should have commented that.  As you might have guessed by my somewhat strong views on renegotiation, I don't think that we should be allowing this to change.

The spec does explicitly permit it, so a comment is a good idea, if only to make it clear that the choice was at least conscious:

/* We expressly don't want to allow ALPN on renegotiation,
 * despite it being permitted by the spec. */

Comment 9

[Wan-Teh Chang]

Attachment: 0001-Bug-996250-Server-side-support-for-ALPN.patch by Martin Thomson, v3

I made the two changes Martin suggested and checked this in.

https://hg.mozilla.org/projects/nss/rev/2cb5c73ac307

Comment 10

[Wan-Teh Chang]

I assume this bug still depends on bug 753136. Does the fatal
no_application_protocol alert cause the peer to abort the
handshake?

Comment 11

[Wan-Teh Chang]

Attachment: Improve the error message for SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL

I noticed that SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL is the
error code we set when we send the no_application_protocol
alert, so I copied the description of no_application_protocol
in the Internet-Draft as the error message.

Comment 12

[Martin Thomson [:mt:]]

Comment on attachment 8433457 [details] [diff] [review]
Improve the error message for SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL

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

This is OK.  It does only happen with ALPN.  

In the case that you are using the default callback function, it's definitely right.  Should we be concerned about a custom callback function that ignores the rules and triggers this error?  If we assume that the same callback might also be used for NPN at the same time, then I think it's good.

Comment 13

[Wan-Teh Chang]

Comment on attachment 8433457 [details] [diff] [review]
Improve the error message for SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL

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

(In reply to Martin Thomson [:mt] from comment #12)
> 
> In the case that you are using the default callback function, it's
> definitely right.  Should we be concerned about a custom callback function
> that ignores the rules and triggers this error?

Could you elaborate on your concern? I'm afraid that I don't understand
this question.

> If we assume that the same callback might also be used for NPN at the
> same time, then I think it's good.

Yes, under the current design, the same callback is also used for NPN
if NPN is enabled.

Comment 14

[Martin Thomson [:mt:]]

(In reply to Wan-Teh Chang from comment #13)
> > In the case that you are using the default callback function, it's
> > definitely right.  Should we be concerned about a custom callback function
> > that ignores the rules and triggers this error?
> 
> Could you elaborate on your concern? I'm afraid that I don't understand
> this question.

That's just me thinking out loud.  I think that I answered my own question with the below.  There are valid cases for a callback to produce this error code.

> > If we assume that the same callback might also be used for NPN at the
> > same time, then I think it's good.
> 
> Yes, under the current design, the same callback is also used for NPN
> if NPN is enabled.

Comment 15

[Wan-Teh Chang]

Comment on attachment 8433457 [details] [diff] [review]
Improve the error message for SSL_ERROR_NEXT_PROTOCOL_NO_PROTOCOL

Patch checked in: https://hg.mozilla.org/projects/nss/rev/544eb0f50d40

Comment 16

[Martin Thomson [:mt:]]

In m-c: http://hg.mozilla.org/mozilla-central/rev/957b31f09f9e