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

net/third_party/nss/ssl/ssl3ext.c

 /*
  * SSL3 Protocol
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* TLS extension code moved here from ssl3ecc.c */
 
 #include "nssrenam.h"
@@ skipping 706 matching lines @@
     return extension_length;
 
 loser:
     return -1;
 }
 
 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) {
         return 0;
     }
 
     extension_length = 2 /* extension type */ + 2 /* extension length */ +
                        2 /* protocol name list length */ +
                        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++] =
                 ssl_app_layer_protocol_xtn;
     } else if (maxBytes < extension_length) {
         return 0;
     }
 
     return extension_length;
 
 loser:
+    if (alpn_protos)
+        PORT_Free(alpn_protos);
     return -1;
 }
 
 static SECStatus
 ssl3_ClientHandleChannelIDXtn(sslSocket *ss, PRUint16 ex_type,
                              SECItem *data)
 {
     PORT_Assert(ss->getChannelID != NULL);
 
     if (data->len) {
@@ skipping 1496 matching lines @@
     } else if (maxBytes < extension_length) {
         PORT_Assert(0);
         return 0;
     }
 
     return extension_length;
 
 loser:
     return -1;
 }