Implement RFC 5764 (DTLS-SRTP).

net/third_party/nss/ssl/ssl3ext.c

Index: net/third_party/nss/ssl/ssl3ext.c
===================================================================
--- net/third_party/nss/ssl/ssl3ext.c	(revision 130750)
+++ net/third_party/nss/ssl/ssl3ext.c	(working copy)
@@ -90,6 +90,10 @@
     PRUint16 ex_type, SECItem *data);
 static PRInt32 ssl3_SendEncryptedClientCertsXtn(sslSocket *ss,
     PRBool append, PRUint32 maxBytes);
+static PRInt32 ssl3_SendUseSRTPXtn(sslSocket *ss, PRBool append,
+    PRUint32 maxBytes);
+static SECStatus ssl3_HandleUseSRTPXtn(sslSocket * ss, PRUint16 ex_type,
+    SECItem *data);
 
 /*
  * Write bytes.  Using this function means the SECItem structure
@@ -250,6 +254,7 @@
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
     { ssl_next_proto_nego_xtn,    &ssl3_ServerHandleNextProtoNegoXtn },
     { ssl_ob_cert_xtn,            &ssl3_ServerHandleOBCertXtn },
+    { ssl_use_srtp_xtn,           &ssl3_HandleUseSRTPXtn },
     { -1, NULL }
 };
 
@@ -264,6 +269,7 @@
     { ssl_next_proto_nego_xtn,    &ssl3_ClientHandleNextProtoNegoXtn },
     { ssl_cert_status_xtn,        &ssl3_ClientHandleStatusRequestXtn },
     { ssl_ob_cert_xtn,            &ssl3_ClientHandleOBCertXtn },
+    { ssl_use_srtp_xtn,           &ssl3_HandleUseSRTPXtn},
     { -1, NULL }
 };
 
@@ -290,7 +296,8 @@
     { ssl_encrypted_client_certs, &ssl3_SendEncryptedClientCertsXtn },
     { ssl_next_proto_nego_xtn,    &ssl3_ClientSendNextProtoNegoXtn },
     { ssl_cert_status_xtn,        &ssl3_ClientSendStatusRequestXtn },
-    { ssl_ob_cert_xtn,            &ssl3_SendOBCertXtn }
+    { ssl_ob_cert_xtn,            &ssl3_SendOBCertXtn },
+    { ssl_use_srtp_xtn,           &ssl3_SendUseSRTPXtn }
     /* any extra entries will appear as { 0, NULL }    */
 };
 
@@ -1868,3 +1875,189 @@
 
     return SECSuccess;
 }
+
+static PRInt32
+ssl3_SendUseSRTPXtn(sslSocket *ss, PRBool append, PRUint32 maxBytes)
+{
+    PRUint32 ext_data_len;
+    PRInt16 i;
+    SECStatus rv;
+
+    if (!ss)
+	return 0;
+
+    if (!ss->sec.isServer) {
+	/* Client side */
+
+	if (!IS_DTLS(ss) || !ss->ssl3.dtlsSRTPCipherCount)
+	    return 0;  /* Not relevant */
+	
+	ext_data_len = 2 + 2 * ss->ssl3.dtlsSRTPCipherCount + 1;
+	
+	if (append && maxBytes >= 4 + ext_data_len) {

[ekr, 2012/04/26 14:33:42]

As a matter of style, I tend to add parentheses wherever I don't immediately
know the precedence rules. I believe that you are correct that this is a safe
transformation, but I'm not sure I would make it. Matter of house style.

[wtc, 2012/04/27 01:06:09]

I removed these parentheses because I found the other
extender sender functions don't use them.  I seem to recall
gcc prefers adding parentheses when there is && or ||.
I can add these parentheses back like this:

  if (append && (maxBytes >= 4 + ext_data_len)) {

but I think this would be excessive:

  if (append && (maxBytes >= (4 + ext_data_len))) {

[ekr, 2012/04/27 03:36:34]

If it were me, I would do the first of these, but really, it's OK as long as
you're sure it's OK. I looked at it and it seemed good, but I had to check the
precedence table first.

+	    /* Extension type */
+	    rv = ssl3_AppendHandshakeNumber(ss, ssl_use_srtp_xtn, 2);
+	    if (rv != SECSuccess) return -1;
+	    /* Length of extension data */
+	    rv = ssl3_AppendHandshakeNumber(ss, ext_data_len, 2);
+	    if (rv != SECSuccess) return -1;
+	    /* Length of the SRTP cipher list */
+	    rv = ssl3_AppendHandshakeNumber(ss,
+					    2 * ss->ssl3.dtlsSRTPCipherCount,
+					    2);
+	    if (rv != SECSuccess) return -1;
+	    /* The SRTP ciphers */
+	    for (i = 0; i < ss->ssl3.dtlsSRTPCipherCount; i++) {
+		rv = ssl3_AppendHandshakeNumber(ss,
+						ss->ssl3.dtlsSRTPCiphers[i],
+						2);
+	    }
+	    /* Empty MKI value */
+	    ssl3_AppendHandshakeVariable(ss, NULL, 0, 1);
+
+	    ss->xtnData.advertised[ss->xtnData.numAdvertised++] =
+		ssl_use_srtp_xtn;
+	}
+
+	return 4 + ext_data_len;
+    }
+
+    /* Server side */
+    if (append && maxBytes >= 9) {
+	/* Extension type */
+	rv = ssl3_AppendHandshakeNumber(ss, ssl_use_srtp_xtn, 2);
+	if (rv != SECSuccess) return -1;
+	/* Length of extension data */
+	rv = ssl3_AppendHandshakeNumber(ss, 5, 2);
+	if (rv != SECSuccess) return -1;
+	/* Length of the SRTP cipher list */
+	rv = ssl3_AppendHandshakeNumber(ss, 2, 2);
+	if (rv != SECSuccess) return -1;
+	/* The selected cipher */
+	rv = ssl3_AppendHandshakeNumber(ss, ss->ssl3.dtlsSRTPCipherSuite, 2);
+	if (rv != SECSuccess) return -1;
+	/* Empty MKI value */
+	ssl3_AppendHandshakeVariable(ss, NULL, 0, 1);	
+    }
+
+    return 9;
+}
+
+static SECStatus
+ssl3_HandleUseSRTPXtn(sslSocket * ss, PRUint16 ex_type, SECItem *data)
+{
+    SECStatus rv;
+    PRInt16 i;
+    PRInt16 bestIndex;
+    PRBool found = PR_FALSE;
+    SECItem litem;
+    PRInt32 cipherLenBytes;
+    PRInt32 cipher;
+

[ekr, 2012/04/26 14:33:42]

Why are these signed? Doesn't the encoding require unsigned values?

[wtc, 2012/04/27 01:06:09]

These two variables are used to store the return value
of ssl3_ConsumeHandshakeNumber, which is PRInt32.  This
is why I changed them to PRInt32.

But I just discovered that these two variables are compared
with variables of unsigned types, which would cause MSVC
warnings.  So it seems that I should change these back to
PRUint32?

[ekr, 2012/04/27 03:36:34]

Ouch, that's not a good state of affairs, esp. since SECFailure = -1.

Maybe we should use PRInt32 and then cast to PRUint32. E.g.,

PRInt32 x;

x = ssl3_ConsumeHandshakeNumber()

if (x == SECFailure)
  return SECFailure;

PRUint32 y = x & 0xffff;

It's kind of bad no matter what.

[wtc, 2012/04/27 17:32:55]

We can also do
  if (x < 0)
    return SECFailure;

This will get rid of all the negative numbers.
y should be PRUint16, right?  Both cipherLenBytes and
cipher are 16-bit quantities in the use_srtp extension.
Ah, I see, the 0xffff mask you used truncates it to 16 bits.
Strictly speaking the masking is not necessary because
we ask ssl3_ConsumeHandshakeNumber to read two bytes.
Yes.  I will look at this more closely next week and
propose a solution.  I actually like your idea.

+    if (!ss->sec.isServer) {
+	/* Client side */
+	if (!data->data || !data->len) {
+            /* malformed */
+            return SECFailure;
+	}
+
+	if (data->len != 5) { /* Must always be 5 since we don't offer MKI */
+            /* malformed */
+            return SECFailure;
+	}
+	
+	/* Now check that the number of ciphers listed is 1 (len = 2) */
+	cipherLenBytes = ssl3_ConsumeHandshakeNumber(ss, 2,
+						     &data->data, &data->len);
+	if (cipherLenBytes != 2)
+	    return SECFailure;
+	
+	/* Get the selected cipher */
+	cipher = ssl3_ConsumeHandshakeNumber(ss, 2, &data->data, &data->len);
+
+	/* Now check that this is one of the ciphers we offered */
+	for (i = 0; i < ss->ssl3.dtlsSRTPCipherCount; i++) {
+	    if (cipher == ss->ssl3.dtlsSRTPCiphers[i]) {
+		found = PR_TRUE;
+		break;
+	    }
+	}
+
+	if (!found)
+	    return SECFailure;
+
+	/* Get the srtp_mki value */
+        rv = ssl3_ConsumeHandshakeVariable(ss, &litem, 1,
+					   &data->data, &data->len);
+        if (rv != SECSuccess) {
+            return SECFailure;
+        }
+	/* We didn't offer an MKI, so this must be 0 length */

[wtc, 2012/04/20 02:02:13]

The RFC says:
  If the client detects a nonzero-length MKI in the server's
  response that is different than the one the client offered,
  then the client MUST abort the handshake and SHOULD send an
  invalid_parameter alert.

Due to a limitation of the ssl3_HandleHelloExtensions
function, this is not easy to do.  (See lines 1633-1636.)
I described this problem before in
https://bugzilla.mozilla.org/show_bug.cgi?id=537356#c52
and Nelson commented on it in
https://bugzilla.mozilla.org/show_bug.cgi?id=537356#c53.

Returning SECFailure here won't abort the handshake.  It
will merely cause the use_srtp extension to be not
negotiated.

I hope this deviation from the RFC's requirement is
acceptable.

[ekr, 2012/04/26 14:33:42]

Did I misread your comment in c55 where it sounded like you
were going to change to be able to abort the handshake with
SECFailure? I think it would be good to abort the handshake,
even if we can't generate an error.

[wtc, 2012/04/27 01:06:09]

You read my comment in c55 correctly.  I did volunteer to
do that, and in c56 Nelson agreed with my plan.  But for
some reason I never got around to doing that :-(
By generating an error, you meant sending an invalid_parameter alert?

In any case, I should open a new NSS bug report for the
work I volunteered to do in c55.

[ekr, 2012/04/27 03:36:34]

I think failing with no alert would be satisfactory for now, plus a TODO if you
get around to c55.

+	if (litem.len != 0)
+	    return SECFailure;
+	
+	/* OK, this looks fine. */
+	ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ssl_use_srtp_xtn;
+	ss->ssl3.dtlsSRTPCipherSuite = cipher;
+	return SECSuccess;
+    }
+
+    /* Server side */
+    if (!IS_DTLS(ss) || !ss->ssl3.dtlsSRTPCipherCount) {
+	/* Ignore the extension. */
+	return SECSuccess;

[ekr, 2012/04/26 14:33:42]

I would change this comment to say
/* Ignore the extension if we aren't doing DTLS or no DTLS-SRTP preferences have
been set */

+    }
+
+    if (!data->data || data->len < 5) {
+	/* malformed */	
+	return SECFailure;
+    }
+
+    /* Get the length of the cipher list */
+    cipherLenBytes = ssl3_ConsumeHandshakeNumber(ss, 2,
+						 &data->data, &data->len);
+    /* Check that there is room for the ciphers and that the list is even
+     * length */
+    if ((cipherLenBytes > (data->len - 1)) || (cipherLenBytes % 2))
+	return SECFailure;
+
+    /* Walk through the offered list one at a time and pick the most preferred
+     * of our ciphers, if any */
+    while (cipherLenBytes > 0) {
+	cipher = ssl3_ConsumeHandshakeNumber(ss, 2, &data->data, &data->len);
+	cipherLenBytes -= 2;
+	
+	for (i = 0; i < ss->ssl3.dtlsSRTPCipherCount; i++) {
+	    /* Unknown ciphers just go through the entire list */
+	    if (cipher == ss->ssl3.dtlsSRTPCiphers[i]) {
+		if (!found || i < bestIndex) {
+		    bestIndex = i;
+		}
+		found = PR_TRUE;
+		break;
+	    }

[ekr, 2012/04/26 14:33:42]

This is arguably slightly less clear because it involves setting found to true
multiple times. I don't feel strongly about it. You think clearer because no
nested conditionals?

[wtc, 2012/04/27 01:06:09]

This was my attempt to do
    bestIndex = i;
in only one place.

I can change it back to your original code.  But after
looking into this, I think the best solution is to imitate
how ssl3_HandleClientHello picks a cipher suite:
- Call ssl3_ConsumeHandshakeVariable to read the entire
  list of cipher suites into a SECItem.
- Use nested for loops, with the outer loop iterating over
  ss->ssl3.dtlsSRTPCiphers[i], to pick a match in our
  order of preference.

[ekr, 2012/04/27 03:36:34]

I'm not sure I'm knowledgeable enough about SECItem to do this easily. Do you
want to try and have me take a look?

[wtc, 2012/04/27 17:32:55]

Yes, I'll be happy to do that.  I can make all the changes
that I suggested and you agreed with, but I'll need you to
test my changes.

Please wait for a new patch set from me some time next week.

+	}
+    }
+
+    /* Get the srtp_mki value */
+    rv = ssl3_ConsumeHandshakeVariable(ss, &litem, 1, &data->data, &data->len);
+    if (rv != SECSuccess) {
+	return SECFailure;
+    }
+
+    if (data->len)
+	return SECFailure; /* Malformed */
+
+    /* Now figure out what to do */
+    if (!found) {
+	/* No matching ciphers */
+	return SECSuccess;
+    }
+
+    /* OK, we have a valid cipher and we've selected it */
+    ss->ssl3.dtlsSRTPCipherSuite = ss->ssl3.dtlsSRTPCiphers[bestIndex];
+    ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ssl_use_srtp_xtn;
+
+    return ssl3_RegisterServerHelloExtensionSender(ss, ssl_use_srtp_xtn,
+						   ssl3_SendUseSRTPXtn);
+}