Support TLS Channel IDs in NSS

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 b9fd6e7f7ef1e439349f36d4a0f02e309dd3680a..3343b5b73c9490f283d318a4703506156b4ec32f 100644
--- a/net/third_party/nss/ssl/ssl3ext.c
+++ b/net/third_party/nss/ssl/ssl3ext.c
@@ -80,10 +80,14 @@ static SECStatus ssl3_HandleRenegotiationInfoXtn(sslSocket *ss,
     PRUint16 ex_type, SECItem *data);
 static SECStatus ssl3_ClientHandleNextProtoNegoXtn(sslSocket *ss,
 			PRUint16 ex_type, SECItem *data);
+static SECStatus ssl3_ClientHandleChannelIDXtn(sslSocket *ss,
+			PRUint16 ex_type, SECItem *data);
 static SECStatus ssl3_ServerHandleNextProtoNegoXtn(sslSocket *ss,
 			PRUint16 ex_type, SECItem *data);
 static PRInt32 ssl3_ClientSendNextProtoNegoXtn(sslSocket *ss, PRBool append,
 					       PRUint32 maxBytes);
+static PRInt32 ssl3_ClientSendChannelIDXtn(sslSocket *ss, PRBool append,
+					  PRUint32 maxBytes);

[wtc, 2012/05/24 23:10:03]

We should add a server side implementation that at least
echoes the extension and verifies the signature in the
EncryptedExtensions message.  This can be done later.

 
 /*
  * Write bytes.  Using this function means the SECItem structure
@@ -253,6 +257,7 @@ static const ssl3HelloExtensionHandler serverHelloHandlersTLS[] = {
     { ssl_session_ticket_xtn,     &ssl3_ClientHandleSessionTicketXtn },
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
     { ssl_next_proto_nego_xtn,    &ssl3_ClientHandleNextProtoNegoXtn },
+    { ssl_channel_id_xtn,          &ssl3_ClientHandleChannelIDXtn },
     { ssl_cert_status_xtn,        &ssl3_ClientHandleStatusRequestXtn },
     { -1, NULL }
 };
@@ -278,6 +283,7 @@ ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS] = {
 #endif
     { ssl_session_ticket_xtn,     &ssl3_SendSessionTicketXtn },
     { ssl_next_proto_nego_xtn,    &ssl3_ClientSendNextProtoNegoXtn },
+    { ssl_channel_id_xtn,          &ssl3_ClientSendChannelIDXtn },
     { ssl_cert_status_xtn,        &ssl3_ClientSendStatusRequestXtn }
     /* any extra entries will appear as { 0, NULL }    */
 };
@@ -635,6 +641,21 @@ ssl3_ClientHandleNextProtoNegoXtn(sslSocket *ss, PRUint16 ex_type,
     return SECITEM_CopyItem(NULL, &ss->ssl3.nextProto, &result);
 }
 
+static SECStatus
+ssl3_ClientHandleChannelIDXtn(sslSocket *ss, PRUint16 ex_type,

[wtc, 2012/05/24 23:10:03]

Please move this next to the ssl3_ClientSendChannelIDXtn
function.

[agl, 2012/05/30 15:28:33]

Done.

+			     SECItem *data)
+{
+    if (!ss->ssl3.channelID)
+	return 0;

[wtc, 2012/05/24 23:10:03]

BUG: we should return either SECSuccess or SECFailure here.
It seems that we should return SECFailure (and set an
error code) if ss->ssl3.channelID is NULL.

Actually, this cannot happen, so you can remove this check
(or replace it with a PORT_Assert).

If ss->ssl3.channelID is NULL, we won't send the extension.

If the server sends an unsolicited extension, NSS will
detect that and won't call the client extension handler.

This is why ss->ssl3.channelID cannot be NULL here.

[agl, 2012/05/30 15:28:33]

Done.

+
+    if (data->len) {
+	PORT_SetError(SSL_ERROR_BAD_CHANNEL_ID_DATA);
+	return SECFailure;
+    }
+    ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type;
+    return SECSuccess;
+}
+
 static PRInt32
 ssl3_ClientSendNextProtoNegoXtn(sslSocket * ss, PRBool append,
 				PRUint32 maxBytes)
@@ -668,6 +689,35 @@ loser:
     return -1;
 }
 
+static PRInt32
+ssl3_ClientSendChannelIDXtn(sslSocket * ss, PRBool append,
+			    PRUint32 maxBytes)
+{
+    PRInt32 extension_length = 4;
+
+    if (!ss->ssl3.channelID)

[wtc, 2012/05/24 23:10:03]

BUG: this should be
    /* Renegotiations do not send this extension. */
    if (!ss->ssl3.channelID || ss->firstHsDone)
        return 0;

[agl, 2012/05/30 15:28:33]

As noted in ssl3con.c, the semantics have changed so that this is sent on
renegotiation. So the bug was that the code *wasn't* sending it on renegotiation
in ssl3con, rather than here.

+	return 0;
+
+    if (append && maxBytes >= extension_length) {
+	SECStatus rv;
+	rv = ssl3_AppendHandshakeNumber(ss, ssl_channel_id_xtn, 2);
+	if (rv != SECSuccess)
+	    goto loser;
+	rv = ssl3_AppendHandshakeNumber(ss, 0, 2);
+	if (rv != SECSuccess)
+	    goto loser;
+	ss->xtnData.advertised[ss->xtnData.numAdvertised++] =
+		ssl_channel_id_xtn;
+    } else if (maxBytes < extension_length) {
+	return 0;
+    }
+
+    return extension_length;

[wtc, 2012/05/24 23:10:03]

Nit: I find this logic difficult to understand.  I know
you copied the logic from other functions.  I think this
is equivalent and clearer:

    if (maxBytes < extension_length) {
        PORT_Assert(0);
        return 0;
    }
    if (append) {
        SECStatus rv;
        rv = ssl3_AppendHandshakeNumber(ss, ssl_channel_id_xtn, 2);
        ...
    }
    return extension_length;

What do you think?

[agl, 2012/05/30 15:28:33]

Agreed. Done.

+
+loser:
+    return -1;
+}
+
 SECStatus
 ssl3_ClientHandleStatusRequestXtn(sslSocket *ss, PRUint16 ex_type,
 				  SECItem *data)