NSS: move the first protocol to the end of the ALPN extension.

net/third_party/nss/ssl/ssl3ext.c

Index: net/third_party/nss/ssl/ssl3ext.c
diff --git a/net/third_party/nss/ssl/ssl3ext.c b/net/third_party/nss/ssl/ssl3ext.c
index 46e31c3bfb1dc8ca30c9efd0189bf79527706671..49c09f7557ed3a5565cf7786a132d025f8f860e8 100644
--- a/net/third_party/nss/ssl/ssl3ext.c
+++ b/net/third_party/nss/ssl/ssl3ext.c
@@ -724,6 +724,7 @@ static PRInt32
 ssl3_ClientSendAppProtoXtn(sslSocket * ss, PRBool append, PRUint32 maxBytes)
 {
     PRInt32 extension_length;
+    unsigned char *alpn_protos = NULL;
 
     /* Renegotiations do not send this extension. */
     if (!ss->opt.nextProtoNego.data || ss->firstHsDone) {
@@ -735,15 +736,39 @@ ssl3_ClientSendAppProtoXtn(sslSocket * ss, PRBool append, PRUint32 maxBytes)
                        ss->opt.nextProtoNego.len;
 
     if (append && maxBytes >= extension_length) {
+	/* NPN requires that the client's fallback protocol is first in the
+	 * list. However, ALPN sends protocols in preference order. So we
+	 * allocate a buffer and move the first protocol to the end of the
+	 * list. */

[wtc, 2013/08/05 22:37:35]

This policy implies the fallback protocol is the least preferred by the client.
While this is true for SPDY and HTTP/1.1, is this true in general?

[agl, 2013/08/06 17:06:43]

I can imagine a case where it's not true, but it would be odd.

Despite what the NPN technote suggests, NPN clients have, in practice, always
used the server's preference. Thus there was only a "fallback protocol" and
"everything else" for NPN.

For ALPN, that's no longer true. The NSS API at the moment is making NSS and
ALPN transparent and I think that means that we need to do this. When we come to
removing NSS support in 2015 I think we should add a new API function which
takes the ALPN protocols in ALPN order. But subtly changing the semantics of the
existing NSS call seems bad.

The alternative is to have an explicit ALPN API now, but I think that's worse
for promoting the use of ALPN, which is the goal for the moment.

 	SECStatus rv;
+	const size_t len = ss->opt.nextProtoNego.len;

[wtc, 2013/08/05 22:37:35]

Nit: |len| can be an unsigned int. I wonder if passing a size_t to
ssl3_AppendHandshakeVariable may result in a compiler warning about
value truncation if size_t is 64 bits.

[agl, 2013/08/06 17:06:43]

Changed to unsigned int (and |i|).

+	size_t i;
+
+	alpn_protos = PORT_Alloc(len);
+	if (alpn_protos == NULL) {
+	    return SECFailure;
+	}
+	if (len > 0) {
+	    /* Each protocol string is prefixed with a single byte length. */

[wtc, 2013/08/05 22:37:35]

Nit: the variable |i| can be declared inside this block.

[agl, 2013/08/06 17:06:43]

Done.

+	    i = ss->opt.nextProtoNego.data[0] + 1;
+	    if (i <= len) {
+		memcpy(alpn_protos, &ss->opt.nextProtoNego.data[i], len - i);
+		memcpy(alpn_protos + len-i, ss->opt.nextProtoNego.data, i);

[wtc, 2013/08/05 22:37:35]

Nit: len-i => len - i

Are you trying to avoid a line wrap?

[agl, 2013/08/06 17:06:43]

It was just the logical grouping in my mind. Changed.

+	    } else {
+		/* This seems to be invalid data so we'll send as-is. */
+		memcpy(alpn_protos, ss->opt.nextProtoNego.data, len);
+	    }
+	}
+
 	rv = ssl3_AppendHandshakeNumber(ss, ssl_app_layer_protocol_xtn, 2);
 	if (rv != SECSuccess)
 	    goto loser;
 	rv = ssl3_AppendHandshakeNumber(ss, extension_length - 4, 2);
 	if (rv != SECSuccess)
 	    goto loser;
-	rv = ssl3_AppendHandshakeVariable(ss, ss->opt.nextProtoNego.data,
-					  ss->opt.nextProtoNego.len, 2);
+	rv = ssl3_AppendHandshakeVariable(ss, alpn_protos, len, 2);
+	PORT_Free(alpn_protos);
+	alpn_protos = NULL;
 	if (rv != SECSuccess)
 	    goto loser;
 	ss->xtnData.advertised[ss->xtnData.numAdvertised++] =
@@ -755,6 +780,8 @@ ssl3_ClientSendAppProtoXtn(sslSocket * ss, PRBool append, PRUint32 maxBytes)
     return extension_length;
 
 loser:
+    if (alpn_protos)
+	PORT_Free(alpn_protos);
     return -1;
 }