Implement RFC 5764 (DTLS-SRTP).

net/third_party/nss/ssl/ssl3ext.c

 /*
  * SSL3 Protocol
  *
  * ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
  * 1.1 (the "License"); you may not use this file except in compliance with
  * the License. You may obtain a copy of the License at
  * http://www.mozilla.org/MPL/
@@ skipping 72 matching lines @@
 static SECStatus ssl3_ServerHandleNextProtoNegoXtn(sslSocket *ss,
                         PRUint16 ex_type, SECItem *data);
 static PRInt32 ssl3_ClientSendNextProtoNegoXtn(sslSocket *ss, PRBool append,
                                                PRUint32 maxBytes);
 static SECStatus ssl3_ServerHandleEncryptedClientCertsXtn(sslSocket *ss,
     PRUint16 ex_type, SECItem *data);
 static SECStatus ssl3_ClientHandleEncryptedClientCertsXtn(sslSocket *ss,
     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
  * cannot be freed.  The caller is expected to call this function
  * on a shallow copy of the structure.
  */
 static SECStatus
 ssl3_AppendToItem(SECItem *item, const unsigned char *buf, PRUint32 bytes)
 {
     if (bytes > item->len)
@@ skipping 140 matching lines @@
     { ssl_server_name_xtn,        &ssl3_HandleServerNameXtn },
 #ifdef NSS_ENABLE_ECC
     { ssl_elliptic_curves_xtn,    &ssl3_HandleSupportedCurvesXtn },
     { ssl_ec_point_formats_xtn,   &ssl3_HandleSupportedPointFormatsXtn },
 #endif
     { ssl_session_ticket_xtn,     &ssl3_ServerHandleSessionTicketXtn },
     { ssl_encrypted_client_certs, &ssl3_ServerHandleEncryptedClientCertsXtn },
     { 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 }
 };
 
 /* These two tables are used by the client, to handle server hello
  * extensions. */
 static const ssl3HelloExtensionHandler serverHelloHandlersTLS[] = {
     { ssl_server_name_xtn,        &ssl3_HandleServerNameXtn },
     /* TODO: add a handler for ssl_ec_point_formats_xtn */
     { ssl_session_ticket_xtn,     &ssl3_ClientHandleSessionTicketXtn },
     { ssl_encrypted_client_certs, &ssl3_ClientHandleEncryptedClientCertsXtn },
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
     { 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 }
 };
 
 static const ssl3HelloExtensionHandler serverHelloHandlersSSL3[] = {
     { ssl_renegotiation_info_xtn, &ssl3_HandleRenegotiationInfoXtn },
     { -1, NULL }
 };
 
 /* Tables of functions to format TLS hello extensions, one function per
  * extension.
  * These static tables are for the formatting of client hello extensions.
  * The server's table of hello senders is dynamic, in the socket struct,
  * and sender functions are registered there.
  */
 static const 
 ssl3HelloExtensionSender clientHelloSendersTLS[SSL_MAX_EXTENSIONS] = {
     { ssl_server_name_xtn,        &ssl3_SendServerNameXtn        },
     { ssl_renegotiation_info_xtn, &ssl3_SendRenegotiationInfoXtn },
 #ifdef NSS_ENABLE_ECC
     { ssl_elliptic_curves_xtn,    &ssl3_SendSupportedCurvesXtn },
     { ssl_ec_point_formats_xtn,   &ssl3_SendSupportedPointFormatsXtn },
 #endif
     { ssl_session_ticket_xtn,     &ssl3_SendSessionTicketXtn },
     { 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 }    */
 };
 
 static const 
 ssl3HelloExtensionSender clientHelloSendersSSL3[SSL_MAX_EXTENSIONS] = {
     { ssl_renegotiation_info_xtn, &ssl3_SendRenegotiationInfoXtn }
     /* any extra entries will appear as { 0, NULL }    */
 };
 
 static PRBool
@@ skipping 1557 matching lines @@
 
     /* The echoed extension must be empty. */
     if (data->len != 0)
         return SECFailure;
 
     /* Keep track of negotiated extensions. */
     ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type;
 
     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);
+}