Closed Bug 996250 Opened 7 years ago Closed 6 years ago

Implement server-side components of ALPN (RFC 7301)

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.16.2

People

(Reporter: mt, Assigned: mt)

References

()

Details

Attachments

(2 files, 5 obsolete files)

WebRTC is going to need NSS to support ALPN on both sides of a connection, not just one.  Bug 996238 describes why this is.
Prep work.  I don't know what the whitespace conventions on NSS are, these were so inconsistent.  These files are now consistent.
Attachment #8407820 - Flags: review?(wtc)
Attachment #8407820 - Flags: review?(ekr)
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?
Attachment #8407823 - Flags: feedback?(wtc)
Attachment #8407823 - Flags: feedback?(ekr)
Component: Security: PSM → Libraries
Product: Core → NSS
Version: unspecified → trunk
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.
Attachment #8407820 - Attachment is obsolete: true
Attachment #8407820 - Attachment is patch: false
Attachment #8407820 - Flags: review?(wtc)
Attachment #8407820 - Flags: review?(ekr)
Attachment #8407823 - Attachment is obsolete: true
Attachment #8407823 - Attachment is patch: false
Attachment #8407823 - Flags: feedback?(wtc)
Attachment #8407823 - Flags: feedback?(ekr)
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.
Attachment #8412825 - Flags: review?(wtc)
Attachment #8412825 - Flags: review?(ekr)
Status: NEW → ASSIGNED
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.
Attachment #8412825 - Attachment is obsolete: true
Attachment #8412825 - Flags: review?(wtc)
Attachment #8412825 - Flags: review?(ekr)
Martin: please review the diffs between this patch and the v1 patch.
Thanks.
Attachment #8432918 - Flags: review?(martin.thomson)
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 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. */
Attachment #8432918 - Flags: review?(martin.thomson) → review+
I made the two changes Martin suggested and checked this in.

https://hg.mozilla.org/projects/nss/rev/2cb5c73ac307
Attachment #8432915 - Attachment is obsolete: true
Attachment #8432918 - Attachment is obsolete: true
Attachment #8433451 - Flags: review+
Attachment #8433451 - Flags: checked-in+
I assume this bug still depends on bug 753136. Does the fatal
no_application_protocol alert cause the peer to abort the
handshake?
Depends on: 753136
Priority: -- → P1
Target Milestone: --- → 3.16.2
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.
Attachment #8433457 - Flags: review?(martin.thomson)
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.
Attachment #8433457 - Flags: review?(martin.thomson) → review+
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.
(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 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
Attachment #8433457 - Flags: checked-in+
In m-c: http://hg.mozilla.org/mozilla-central/rev/957b31f09f9e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Implement server-side components of ALPN → Implement server-side components of ALPN (RFC 7301)
See Also: → 1284412
You need to log in before you can comment on or make changes to this bug.