1284412 - Provide usable ALPN support

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Franziskus Kiefer [:franziskus]]

NSS offers the possibility to register a callback to handle ALPN negotiations (SSL_SetNextProtoCallback). But the callback has no possibility to set |ss->ssl3.nextProtoState| and thus is not usable as ssl3_SelectAppProtocol fails if |nextProtoState != SSL_NEXT_PROTO_NEGOTIATED|.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: alpn-callback.patch

If we don't want to change the callback, we have to omit the check for |nextProtoState| here. We have to trust that the callback sets rv and result correct.
If we don't want to do that, we could check again that the result we get here is in |data|. But I don't think that's necessary.

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8767866 [details] [diff] [review]
alpn-callback.patch

wait, this doesn't work...

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: alpn-callback.patch

so, this time also disabling the default in ssl_NextProtoNegoCallback.

Comment 4

[Franziskus Kiefer [:franziskus]]

Attachment: alpn-callback.patch

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8767873 [details] [diff] [review]
alpn-callback.patch

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

I'm not sure the semantics here are quite right. Can you please write up what the callback's responsibilities are and how they are handled by libssl.

::: lib/ssl/ssl3ext.c
@@ +754,5 @@
>      SECITEM_FreeItem(&ss->ssl3.nextProto, PR_FALSE);
>  
> +    if (ex_type == ssl_app_layer_protocol_xtn && (result.len < 1 || !result.data)) {
> +        /* We do NOT check nextProtoState here because nextProtoCallback can't
> +         * set it. But we have to check that we actually got a result. */

You should double-check that result is valid, I think. You could use tls13_AlpnTagAllowed(), maybe refactoring this into here.

::: lib/ssl/sslsock.c
@@ +1929,5 @@
>      /* The other side supports the extension, and either doesn't have any
>       * protocols configured, or none of its options match ours. In this case we
>       * request our favoured protocol. */
>      /* This will be treated as a failure for ALPN. */
>      ss->ssl3.nextProtoState = SSL_NEXT_PROTO_NO_OVERLAP;

Why are you setting the state here? I thought we agreed it wasn't permitted.

@@ +1939,5 @@
>      }
> +    if (result) {
> +        memcpy(protoOut, result + 1, result[0]);
> +        *protoOutLen = result[0];
> +    }

Nested seems like it would be better here.

Comment 6

[Franziskus Kiefer [:franziskus]]

(In reply to Eric Rescorla (:ekr) from comment #5)
> Comment on attachment 8767873 [details] [diff] [review]
> alpn-callback.patch

The callback has to return the |result| SECItem holding the negotiated protocol that gets copied to |ss->ssl3.nextProto|. We currently expect the callback also to set nextProtoState, which is not possible if the callback is not in NSS but external.
In order make this callback work without changes to the API we can interpret the result as follow:
If result is empty or the callback return an error, we assume that no protocol could be negotiated (we could set nextProtoState to SSL_NEXT_PROTO_NO_OVERLAP here). 
If the result contains a protocol and the callback doesn't return an error, we take the result as nextProto and set nextProtoState to SSL_NEXT_PROTO_NEGOTIATED (as required later on).
This breaks the case where NPN chooses a default value that was not negotiated.

> ::: lib/ssl/ssl3ext.c
> @@ +754,5 @@
> >      SECITEM_FreeItem(&ss->ssl3.nextProto, PR_FALSE);
> >  
> > +    if (ex_type == ssl_app_layer_protocol_xtn && (result.len < 1 || !result.data)) {
> > +        /* We do NOT check nextProtoState here because nextProtoCallback can't
> > +         * set it. But we have to check that we actually got a result. */
> 
> You should double-check that result is valid, I think. You could use
> tls13_AlpnTagAllowed(), maybe refactoring this into here.

ok, will do.

> ::: lib/ssl/sslsock.c
> @@ +1929,5 @@
> >      /* The other side supports the extension, and either doesn't have any
> >       * protocols configured, or none of its options match ours. In this case we
> >       * request our favoured protocol. */
> >      /* This will be treated as a failure for ALPN. */
> >      ss->ssl3.nextProtoState = SSL_NEXT_PROTO_NO_OVERLAP;
> 
> Why are you setting the state here? I thought we agreed it wasn't permitted.

This shouldn't really make a difference because result will be empty in this case as well. So we could remove it, definitely the comment.

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8767873 [details] [diff] [review]
alpn-callback.patch

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

I'll await a second round.  BTW, rietveld please.

Comment 8

[Franziskus Kiefer [:franziskus]]

Attachment: alpn-callback.patch

also at https://codereview.appspot.com/303000043

I added a check for ssl3_AlpnTagAllowed, did some clean up, and added a quick test for the callback.

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8769806 [details] [diff] [review]
alpn-callback.patch

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

Let's see if we can't remove this stuff instead.

Comment 10

[Franziskus Kiefer [:franziskus]]

Fixed in bug 1402738