Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9982019:
Implement RFC 5764 (DTLS-SRTP). (Closed)
|
Created:
8 years, 8 months ago by wtc Modified:
8 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplement RFC 5764 (DTLS-SRTP).
The patch is contributed by Eric Rescorla.
R=rsleevi@chromium.org,ekr@rtfm.com
BUG=120938
TEST=none (eventually covered by libjingle tests)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140535
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix coding style nits, require DTLS for the use_srtp extension #
Total comments: 10
Patch Set 3 : First full review #
Total comments: 19
Patch Set 4 : Change how we read the cipher list and make changes suggested by ekr #
Total comments: 2
Patch Set 5 : Avoid goto #
Total comments: 2
Patch Set 6 : Add NSS bug number #
Total comments: 25
Patch Set 7 : Make the changes suggested by rsleevi #Patch Set 8 : Fix the all-or-nothing behavior of SSL_SetSRTPCiphers #Patch Set 9 : Sync before checkin #
Messages
Total messages: 34 (0 generated)
Ekr: Patch set 1 is your original patch in the upstream NSS bug https://bugzilla.mozilla.org/show_bug.cgi?id=737178, excluding the tstclnt.c changes. I fixed coding style nits and added two checks to prevent the use_srtp extension from being used in stream TLS mode. I still need to review the extension sender and handler functions, so you can wait until then to review my changes. I have some questions for you. https://chromiumcodereview.appspot.com/9982019/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): https://chromiumcodereview.appspot.com/9982019/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3ext.c:1679: sender = (ss->version > SSL_LIBRARY_VERSION_3_0 || IS_DTLS(ss)) ? This is no longer necessary after we made ss->version the base TLS protocol version. https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... File net/third_party/nss/ssl/ssl3ext.c (right): https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... net/third_party/nss/ssl/ssl3ext.c:2008: /* Ignore the extension. */ If the client offers the use_srtp extension in stream TLS mode, should we ignore the extension or fail? https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... File net/third_party/nss/ssl/sslsock.c (right): https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... net/third_party/nss/ssl/sslsock.c:231: /* XXX what about SRTP_NULL_SHA1_80 and SRTP_NULL_SHA1_32? */ Should SRTP_NULL_SHA1_80 and SRTP_NULL_SHA1_32 be added to this array? You allow tstclnt.c to set these null encryption cipher suites using the letters C and D.
https://chromiumcodereview.appspot.com/9982019/diff/1/net/third_party/nss/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): https://chromiumcodereview.appspot.com/9982019/diff/1/net/third_party/nss/ssl... net/third_party/nss/ssl/ssl3ext.c:1679: sender = (ss->version > SSL_LIBRARY_VERSION_3_0 || IS_DTLS(ss)) ? On 2012/04/04 23:32:49, wtc wrote: > > This is no longer necessary after we made ss->version the > base TLS protocol version. Concur. https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... File net/third_party/nss/ssl/ssl3ext.c (right): https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... net/third_party/nss/ssl/ssl3ext.c:2008: /* Ignore the extension. */ On 2012/04/04 23:32:49, wtc wrote: > > If the client offers the use_srtp extension in stream TLS > mode, should we ignore the extension or fail? I think it's best to ignore it. In the OpenSSL code, I don't even test for DTLS, I believe. But it's pretty clear from the spec that this isn't a TLS feature. https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... File net/third_party/nss/ssl/sslsock.c (right): https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... net/third_party/nss/ssl/sslsock.c:231: /* XXX what about SRTP_NULL_SHA1_80 and SRTP_NULL_SHA1_32? */ On 2012/04/04 23:32:49, wtc wrote: > > Should SRTP_NULL_SHA1_80 and SRTP_NULL_SHA1_32 be added to > this array? You allow tstclnt.c to set these null encryption > cipher suites using the letters C and D. I think my preference is to simply not implement them, and remove them from tstclnt.c as well.
Ekr: the CL is ready for your review. Patch set 1 is your original NSS patch. Patch set 2 contains the white space and style nit fixes. Patch set 3 has the fixes I made after a full review today. Please diff between the patch sets to review my changes. I also have some questions for you below. Thanks. https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... File net/third_party/nss/ssl/ssl.h (right): https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... net/third_party/nss/ssl/ssl.h:846: unsigned int numCiphers); To enable SSL/TLS cipher suites, one has to call the SSL_CipherPrefSet function on individual cipher suites. So SSL_SetSRTPCiphers uses a different method to enable all SRTP cipher suites in one shot. I wonder if we should use a SSL_SRTPCipherPrefSet function instead to be consistent with the SSL_CipherPrefSet style. https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... File net/third_party/nss/ssl/ssl3ext.c (right): https://chromiumcodereview.appspot.com/9982019/diff/5001/net/third_party/nss/... net/third_party/nss/ssl/ssl3ext.c:1960: !ssl3_ClientExtensionAdvertised(ss, ssl_use_srtp_xtn)) { We don't need to check this here because ssl3_HandleHelloExtensions checks this (on line 1622). https://chromiumcodereview.appspot.com/9982019/diff/13001/net/third_party/nss... File net/third_party/nss/ssl/ssl3ext.c (right): https://chromiumcodereview.appspot.com/9982019/diff/13001/net/third_party/nss... net/third_party/nss/ssl/ssl3ext.c:1995: /* We didn't offer an MKI, so this must be 0 length */ 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. https://chromiumcodereview.appspot.com/9982019/diff/13001/net/third_party/nss... File net/third_party/nss/ssl/sslproto.h (right): https://chromiumcodereview.appspot.com/9982019/diff/13001/net/third_party/nss... net/third_party/nss/ssl/sslproto.h:242: #define SRTP_AES128_CM_HMAC_SHA1_80 0x0001 I hope it's fine to use the exact cipher suite names (which have _HMAC) from the RFC. By the way, these are called "protection profiles" in the RFC. Should we also use that term here?
I had a little trouble with Rietveld figuring out how to isolate each patch set, but I think I got all the changes. http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl.... net/third_party/nss/ssl/ssl.h:846: unsigned int numCiphers); On 2012/04/20 02:02:13, wtc wrote: > > To enable SSL/TLS cipher suites, one has to call the > SSL_CipherPrefSet function on individual cipher suites. > So SSL_SetSRTPCiphers uses a different method to enable > all SRTP cipher suites in one shot. > > I wonder if we should use a SSL_SRTPCipherPrefSet function > instead to be consistent with the SSL_CipherPrefSet style. That seems to be a change consistent with NSS style. I.e., "Yes" :) http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl3... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl3... net/third_party/nss/ssl/ssl3ext.c:1960: !ssl3_ClientExtensionAdvertised(ss, ssl_use_srtp_xtn)) { On 2012/04/20 02:02:13, wtc wrote: > > We don't need to check this here because > ssl3_HandleHelloExtensions checks this (on line 1622). Agreed. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1897: if (append && maxBytes >= 4 + ext_data_len) { 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. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1956: Why are these signed? Doesn't the encoding require unsigned values? http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1995: /* We didn't offer an MKI, so this must be 0 length */ On 2012/04/20 02:02:13, wtc wrote: > > 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. 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. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2008: return SECSuccess; I would change this comment to say /* Ignore the extension if we aren't doing DTLS or no DTLS-SRTP preferences have been set */ http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2038: } 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? http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslproto.h (right): http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslproto.h:242: #define SRTP_AES128_CM_HMAC_SHA1_80 0x0001 On 2012/04/20 02:02:13, wtc wrote: > > I hope it's fine to use the exact cipher suite names > (which have _HMAC) from the RFC. Yes, that seems like an improvement. > By the way, these are called "protection profiles" in the > RFC. Should we also use that term here? Yes, we should.
Ekr: thank you for reviewing my changes. I answer your questions below. http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl.... net/third_party/nss/ssl/ssl.h:846: unsigned int numCiphers); On 2012/04/26 14:33:42, ekr wrote: > > That seems to be a change consistent with NSS style. I.e., "Yes" :) I just realized that one disadvantage of the SSL_CipherPrefSet style is that the ordering of cipher suites is fixed. So, if you think the number of SRTP cipher suites will remain small, the SSL_SetSRTPCiphers function seems better: it allows us to specify all the SRTP cipher to enable, including their ordering, in one shot. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1897: if (append && maxBytes >= 4 + ext_data_len) { On 2012/04/26 14:33:42, ekr wrote: > 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. 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))) { http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1956: On 2012/04/26 14:33:42, ekr wrote: > Why are these signed? Doesn't the encoding require unsigned values? 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? http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1995: /* We didn't offer an MKI, so this must be 0 length */ On 2012/04/26 14:33:42, ekr wrote: > > 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? 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 :-( > I think it would be good to abort the handshake, > even if we can't generate an error. 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. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2038: } On 2012/04/26 14:33:42, ekr wrote: > 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? 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.
See comments. http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9982019/diff/5001/net/third_party/nss/ssl/ssl.... net/third_party/nss/ssl/ssl.h:846: unsigned int numCiphers); On 2012/04/27 01:06:08, wtc wrote: > On 2012/04/26 14:33:42, ekr wrote: > > > > That seems to be a change consistent with NSS style. I.e., "Yes" :) > > I just realized that one disadvantage of the > SSL_CipherPrefSet style is that the ordering of cipher > suites is fixed. > > So, if you think the number of SRTP cipher suites will > remain small, the SSL_SetSRTPCiphers function seems > better: it allows us to specify all the SRTP cipher to > enable, including their ordering, in one shot. I think it's likely to remain very small; there's no combinatoric explosion problem with key exchange, so we may just add AEAD (GCM) or something, but probably no more than 10 cipher suites. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1897: if (append && maxBytes >= 4 + ext_data_len) { On 2012/04/27 01:06:09, wtc wrote: > On 2012/04/26 14:33:42, ekr wrote: > > 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. > > 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))) { 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. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1956: On 2012/04/27 01:06:09, wtc wrote: > On 2012/04/26 14:33:42, ekr wrote: > > Why are these signed? Doesn't the encoding require unsigned values? > > 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? 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. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1995: /* We didn't offer an MKI, so this must be 0 length */ On 2012/04/27 01:06:09, wtc wrote: > On 2012/04/26 14:33:42, ekr wrote: > > > > 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? > > 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 :-( > > > I think it would be good to abort the handshake, > > even if we can't generate an error. > > 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. I think failing with no alert would be satisfactory for now, plus a TODO if you get around to c55. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2038: } On 2012/04/27 01:06:09, wtc wrote: > On 2012/04/26 14:33:42, ekr wrote: > > 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? > > 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. 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?
Ekr: I will send you a new patch set some time next week. See my answers to your questions below. Thanks. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1956: On 2012/04/27 03:36:34, ekr wrote: > > 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; We can also do if (x < 0) return SECFailure; This will get rid of all the negative numbers. > PRUint32 y = x & 0xffff; 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. > It's kind of bad no matter what. Yes. I will look at this more closely next week and propose a solution. I actually like your idea. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2038: } On 2012/04/27 03:36:34, ekr wrote: > > 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? 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. http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslproto.h (right): http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslproto.h:242: #define SRTP_AES128_CM_HMAC_SHA1_80 0x0001 On 2012/04/26 14:33:42, ekr wrote: > On 2012/04/20 02:02:13, wtc wrote: > > By the way, these are called "protection profiles" in the > > RFC. Should we also use that term here? > > Yes, we should. Any suggestion on how to abbreviate "ProtectionProfile" if it is too long? I actually like "cipher suites" better. "Protection profiles" have a different meaning in Common Criteria evaluations.
On Fri, Apr 27, 2012 at 10:32 AM, <wtc@chromium.org> wrote > http://codereview.chromium.org/9982019/diff/13001/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/ssl3ext.c:2038: } > > On 2012/04/27 03:36:34, ekr wrote: > >> 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? > > > 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. Willdo. I'm working on the libjingle NSS patch as we speak so testing shoul dbe no problem. > Any suggestion on how to abbreviate "ProtectionProfile" > if it is too long? > > I actually like "cipher suites" better. "Protection profiles" > have a different meaning in Common Criteria evaluations. Let's leave it as is then. I don't think it matters much. -Ekr > http://codereview.chromium.org/9982019/
Ekr: please review patch set 4. If it looks good, please also test it. Thanks! http://codereview.chromium.org/9982019/diff/24002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/24002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2049: goto cipher_found; This goto statement is necessary to exit the nested for loops. Perhaps I should try harder to avoid it?
Ekr: Please review patch set 5. I got rid of the goto.
Except for the above comment, I think this is fine. I have tested it against OpenSSL and in my test harness and things seem OK. http://codereview.chromium.org/9982019/diff/24002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/24002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2049: goto cipher_found; On 2012/05/03 02:00:15, wtc wrote: > > This goto statement is necessary to exit the nested for loops. > Perhaps I should try harder to avoid it? I think it's fine. http://codereview.chromium.org/9982019/diff/31002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/31002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2010: */ I'd still like to find a way to abort the handshake. I don't care if it generates an alert, though. Is there anything we can do?
rsleevi: please review patch set 6. This CL was based on Eric Rescorla's NSS patch, with some changes by me. ekr: I decided to avoid the "goto" statement. I answered your other question below. http://codereview.chromium.org/9982019/diff/31002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/31002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2010: */ On 2012/05/07 22:51:00, ekr wrote: > I'd still like to find a way to abort the handshake. I don't care if it > generates an alert, though. Is there anything we can do? I filed NSS bug 753136 and described how to fix this: https://bugzilla.mozilla.org/show_bug.cgi?id=753136 Basically we need to examine all the TLS hello extension handlers called by ssl3_HandleHelloExtensions and the call sites of ssl3_HandleHelloExtensions: http://mxr.mozilla.org/security/ident?i=ssl3_HandleHelloExtensions For the former, I think it is best done in a series of 1-1.5 hour "pair programming" sessions. Ekr, would you have time to do that?
On Tue, May 8, 2012 at 3:15 PM, <wtc@chromium.org> wrote: > rsleevi: please review patch set 6. This CL was based on > Eric Rescorla's NSS patch, with some changes by me. > > ekr: I decided to avoid the "goto" statement. I answered > your other question below. > > > > http://codereview.chromium.org/9982019/diff/31002/net/third_party/nss/ssl/ssl... > File net/third_party/nss/ssl/ssl3ext.c (right): > > http://codereview.chromium.org/9982019/diff/31002/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/ssl3ext.c:2010: */ > On 2012/05/07 22:51:00, ekr wrote: >> >> I'd still like to find a way to abort the handshake. I don't care if > > it >> >> generates an alert, though. Is there anything we can do? > > > I filed NSS bug 753136 and described how to fix this: > https://bugzilla.mozilla.org/show_bug.cgi?id=753136 > > Basically we need to examine all the TLS hello extension > handlers called by ssl3_HandleHelloExtensions and the call > sites of ssl3_HandleHelloExtensions: > > http://mxr.mozilla.org/security/ident?i=ssl3_HandleHelloExtensions > > For the former, I think it is best done in a series of > 1-1.5 hour "pair programming" sessions. Ekr, would you > have time to do that? I'm way ahead of you... I already offered that in the bugzilla bug :) My preference would be to have your hands on the keyboard if that's OK with you. -Ekr
wtc: Based on the ChromeOS TPM work ongoing, I'm not sure I'll have a chance to review this CL this week. You may wish to ping agl@
rsleevi: since you reviewed the previous DTLS patch, I think you are the best reviewer for this patch. You can review this patch next week. Thanks.
wtc: I only focused on the code, and did not focus on the implementation of the extension. I've assumed you've already vetted the conformance to the spec, etc. Mostly style nits, but one design question and one implementation question/maybe BUG? http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl.h:842: ** negotiated. If you have to document that it has a side-effect, would it make more sense to have a UseSRTP function (which sets some default cipher list) and then a SetSRTPCiphers? This would parallel the DTLS/TLS cipher preference setting. Also, the interaction with SSL model sockets is not clear here. Does this have a model socket / global preference parallel? http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1892: if (!IS_DTLS(ss) || !ss->ssl3.dtlsSRTPCipherCount) Seems IS_DTLS should be checked before the server side is checked, otherwise SRTP can be negotiated on a TLS socket? http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1956: IS_DTLS check http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1972: if (rv != SECSuccess) { This function seems to have inconsistent bracing throughout. 1972-1973, 1976/1977, 1990/1991, 1996-1998, 2011/2012, 2035-2037, 2039/2040, 2056-2058, 2060/2061 http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1999: /* We didn't offer an MKI, so this must be 0 length */ nit: newline following } http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2010: */ Perhaps move this up to 1964, since it applies to the SECFailure returned in (1961/1966), and at least there it provides context about the MKI. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2045: for (j = 0; j + 1 < ciphers.len; j += 2) { !found && j + 1 ... http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2049: break; I assume this is where you were talking about gotos? I note the equivalent NSS function does use goto, but that's because it's got an error handler - http://code.google.com/searchframe#OAMlx_jo-ck/src/net/third_party/nss/ssl/ss... http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1633: PORT_SetError(SEC_ERROR_INVALID_ARGS); The NSS SSL/DTLS code accepts non-existant cipher suites without issue, simply ignoring them. I think this works to our advantage. Does it make sense to do the same here? http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1677: #if 0 Does it make sense to update this function, even though it's not implemented? Seems a subtle edge if it gets enabled but breaks.
Make the changes suggested by rsleevi
rsleevi: please review patch set 7. Thank you very much for your thoughtful review. I made all the changes you suggested except for adding a SSL_UseSRTP function and global preference for SRTP ciphers. I will discuss them with Ekr. Ekr: what do you think of rsleevi's suggestion? (Search for "Ekr" in the comment below.) http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl.h:842: ** negotiated. On 2012/05/10 19:43:11, Ryan Sleevi wrote: > If you have to document that it has a side-effect, would it make more sense to > have a UseSRTP function (which sets some default cipher list) and then a > SetSRTPCiphers? > > This would parallel the DTLS/TLS cipher preference setting. I will talk to Ekr about this. Thanks for the suggestion. > Also, the interaction with SSL model sockets is not clear here. Does this have a > model socket / global preference parallel? I updated ssl_DupSocket and SSL_ReconfigFD to copy from the model socket. I will talk to Ekr about a global preference for SRTP ciphers. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1892: if (!IS_DTLS(ss) || !ss->ssl3.dtlsSRTPCipherCount) On 2012/05/10 19:43:11, Ryan Sleevi wrote: > Seems IS_DTLS should be checked before the server side is checked, otherwise > SRTP can be negotiated on a TLS socket? This is a good question. The server side checks IS_DTLS in ssl3_HandleUseSRTPXtn on line 2021. The reason client and server check IS_DTLS in different places is that the client always calls ssl3_SendUseSRTPXtn, but the server calls ssl3_SendUseSRTPXtn only if ssl3_HandleUseSRTPXtn has validated the extension and marked that ssl3_SendUseSRTPXtn should be called. Similarly, the client side does not need to check IS_DTLS in ssl3_HandleUseSRTPXtn because the client won't call ssl3_HandleUseSRTPXtn (even if the server sends the extension) unless the client has sent the extension. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2010: */ On 2012/05/10 19:43:11, Ryan Sleevi wrote: > Perhaps move this up to 1964, since it applies to the SECFailure returned in > (1961/1966), and at least there it provides context about the MKI. After studying this code, I decided to move the extension data length check (data->len != 5) on line 1964 after the MKI length check here. This way we only need to talk about MKI here. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2045: for (j = 0; j + 1 < ciphers.len; j += 2) { On 2012/05/10 19:43:11, Ryan Sleevi wrote: > !found && j + 1 ... It is not necessary to test !found here because !found is guaranteed to be true by the !found test on line 2044 and the break statement on line 2049. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2049: break; On 2012/05/10 19:43:11, Ryan Sleevi wrote: > I assume this is where you were talking about gotos? Yes, it is. Do you still prefer that we avoid goto here? http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1633: PORT_SetError(SEC_ERROR_INVALID_ARGS); On 2012/05/10 19:43:11, Ryan Sleevi wrote: > The NSS SSL/DTLS code accepts non-existant cipher suites without issue, simply > ignoring them. SSL_CipherPrefSet returns SECFailure with SSL_ERROR_UNKNOWN_CIPHER_SUITE if the cipher suite is nonexistent. Perhaps I misunderstood your comment here? http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1677: #if 0 On 2012/05/10 19:43:11, Ryan Sleevi wrote: > Does it make sense to update this function, even though it's not implemented? I updated both this function and ssl_DupSocket (both of them take a model socket as input).
http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:1892: if (!IS_DTLS(ss) || !ss->ssl3.dtlsSRTPCipherCount) On 2012/05/12 01:00:59, wtc wrote: > > On 2012/05/10 19:43:11, Ryan Sleevi wrote: > > Seems IS_DTLS should be checked before the server side is checked, otherwise > > SRTP can be negotiated on a TLS socket? > > This is a good question. > > The server side checks IS_DTLS in ssl3_HandleUseSRTPXtn on > line 2021. The reason client and server check IS_DTLS in > different places is that the client always calls > ssl3_SendUseSRTPXtn, but the server calls > ssl3_SendUseSRTPXtn > only if ssl3_HandleUseSRTPXtn has validated the extension > and marked that ssl3_SendUseSRTPXtn should be called. > > Similarly, the client side does not need to check IS_DTLS > in ssl3_HandleUseSRTPXtn because the client won't call > ssl3_HandleUseSRTPXtn (even if the server sends the > extension) unless the client has sent the extension. Fair enough, if that's the contract. I wanted to make sure it wasn't possible for a misbehaving program to end up with this called on a server socket or a malicious server to be able to send an invalid extension. Seems like this is already covered. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2010: */ On 2012/05/12 01:00:59, wtc wrote: > > On 2012/05/10 19:43:11, Ryan Sleevi wrote: > > Perhaps move this up to 1964, since it applies to the SECFailure returned in > > (1961/1966), and at least there it provides context about the MKI. > > After studying this code, I decided to move the > extension data length check (data->len != 5) on line 1964 after the MKI length > check here. This way we only need to > talk about MKI here. This means you're now relying on ssl3_ConsumeHandshakeVariable to do the length check. This is probably fine, but note that if the lengths don't match, this will force SSL3_SendAlert() to be called. Is this what you want to happen? http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2045: for (j = 0; j + 1 < ciphers.len; j += 2) { On 2012/05/12 01:00:59, wtc wrote: > > On 2012/05/10 19:43:11, Ryan Sleevi wrote: > > !found && j + 1 ... > > It is not necessary to test !found here because !found is > guaranteed to be true by the !found test on line 2044 and > the break statement on line 2049. Ah, right. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2049: break; On 2012/05/12 01:00:59, wtc wrote: > > On 2012/05/10 19:43:11, Ryan Sleevi wrote: > > I assume this is where you were talking about gotos? > > Yes, it is. Do you still prefer that we avoid goto here? Depends. Currently, the code always consumes the srtp_mki value (line 2055), even when !found (line 2064). If that's the expected behaviour, then yeah, I think goto is considered harmful. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1633: PORT_SetError(SEC_ERROR_INVALID_ARGS); On 2012/05/12 01:00:59, wtc wrote: > > On 2012/05/10 19:43:11, Ryan Sleevi wrote: > > The NSS SSL/DTLS code accepts non-existant cipher suites without issue, simply > > ignoring them. > > SSL_CipherPrefSet returns SECFailure with > SSL_ERROR_UNKNOWN_CIPHER_SUITE if the cipher suite is > nonexistent. Perhaps I misunderstood your comment here? Ah, you're right, sorry for not double-checking memory. In the SSL/TLS case, we can enable/disable individual cipher suites via SSL_CipherPrefSet, so we can safely ignore any errors with unrecognized suites ( see http://code.google.com/searchframe#OAMlx_jo-ck/src/net/socket/ssl_server_sock... ) With this code, because setting enabled suites is all-or-nothing, and because even enabling SRTP is dependent on all-or-nothing suites, we couldn't do such an approach if we wanted to. This would be an issue if a cipher suite ever needed to be removed, as it would break clients still supplying the old cipher suite. I think that would argue in favour of separating the API?
On Fri, May 11, 2012 at 6:00 PM, <wtc@chromium.org> wrote: > > http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl.h > File net/third_party/nss/ssl/ssl.h (right): > > http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... > net/third_party/nss/ssl/ssl.h:842: ** negotiated. > > On 2012/05/10 19:43:11, Ryan Sleevi wrote: >> >> If you have to document that it has a side-effect, would it make more > > sense to >> >> have a UseSRTP function (which sets some default cipher list) and then > > a >> >> SetSRTPCiphers? > > >> This would parallel the DTLS/TLS cipher preference setting. > > > I will talk to Ekr about this. Thanks for the suggestion. > > >> Also, the interaction with SSL model sockets is not clear here. Does > > this have a >> >> model socket / global preference parallel? > > > I updated ssl_DupSocket and SSL_ReconfigFD to copy from > the model socket. > > I will talk to Ekr about a global preference for > SRTP ciphers. I'm torn: I'm generally not a huge fan of global preferences, but it seems to be the way NSS currently does things. I'm not sure how to weigh these. -Ekr
http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2049: break; On 2012/05/12 01:13:06, Ryan Sleevi wrote: > On 2012/05/12 01:00:59, wtc wrote: > > > > On 2012/05/10 19:43:11, Ryan Sleevi wrote: > > > I assume this is where you were talking about gotos? > > > > Yes, it is. Do you still prefer that we avoid goto here? > > Depends. > > Currently, the code always consumes the srtp_mki value (line 2055), even when > !found (line 2064). If that's the expected behaviour, then yeah, I think goto is > considered harmful. I don't think it matters either way, since we're going to generate an error. IMO the goto is clearer but I'm not going to go to the mat over it.
http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/ssl3ext.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/ssl3ext.c:2010: */ On 2012/05/12 01:13:06, Ryan Sleevi wrote: > > This means you're now relying on ssl3_ConsumeHandshakeVariable to do the length > check. > > This is probably fine, but note that if the lengths don't match, this will force > SSL3_SendAlert() to be called. Is this what you want to happen? I didn't intend to force SSL3_SendAlert(), but I think it is fine to send a decode_error alert when an extension cannot be decoded because the length of the extension is incorrect. http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... File net/third_party/nss/ssl/sslsock.c (right): http://codereview.chromium.org/9982019/diff/37002/net/third_party/nss/ssl/ssl... net/third_party/nss/ssl/sslsock.c:1633: PORT_SetError(SEC_ERROR_INVALID_ARGS); On 2012/05/12 01:13:06, Ryan Sleevi wrote: > > This would be an issue if a cipher suite ever needed to be removed, as it would > break clients still supplying the old cipher suite. I think that would argue in > favour of separating the API? I also came to the same conclusion that we probably should enable SRTP cipher suites individually, in the same way we enable TLS cipher suites. Ekr: do you have time to implement an SSL_USE_SRTP socket option and the SSL_SRTPCipherPrefSet and SSL_SRTPCipherPrefGet functions, or do you want me to do that? I think we can omit the global preferences and just support a model socket.
lso came to the same conclusion that we probably should > enable SRTP cipher suites individually, in the same way we > enable TLS cipher suites. > > Ekr: do you have time to implement an SSL_USE_SRTP socket > option and the SSL_SRTPCipherPrefSet and SSL_SRTPCipherPrefGet > functions, or do you want me to do that? It would be great if you were able to do it. I'm juggling a bunch of libjingle CLs today. Is it too much trouble? -Ekr
Wan-Teh, are you able to to do this, or should I pick it back up? Thanks, -Ekr On Wed, May 16, 2012 at 11:51 AM, Eric Rescorla <ekr@rtfm.com> wrote: > lso came to the same conclusion that we probably should >> enable SRTP cipher suites individually, in the same way we >> enable TLS cipher suites. >> >> Ekr: do you have time to implement an SSL_USE_SRTP socket >> option and the SSL_SRTPCipherPrefSet and SSL_SRTPCipherPrefGet >> functions, or do you want me to do that? > > It would be great if you were able to do it. I'm juggling a bunch > of libjingle CLs today. Is it too much trouble? > > -Ekr
On Thu, May 24, 2012 at 11:26 AM, Eric Rescorla <ekr@rtfm.com> wrote: > Wan-Teh, are you able to to do this, or should I pick it back up? I haven't had time to do this, sorry. I think we probably should first make the current SSL_SetSRTPCiphers usable. Its only major problem is the "all or nothing" behavior. If we can change it to fail only if none of the SRTP ciphers are supported, then that would be an acceptable solution to me. Alternatively we can consider using the cipher suite array as an in/out argument, and let the caller determine if the actual enabled ciphers are acceptable. I still think doing the SSL_SetSRTPCipherPref approach might be better, but I think it is also advantageous to make the current approach work, to allow testing. Because of the backward compatibility of NSS, we will need to be careful not to leak the SSL_SetSRTPCiphers function to the DLL export file. Wan-Teh
On Thu, May 24, 2012 at 11:46 AM, Wan-Teh Chang <wtc@google.com> wrote: > On Thu, May 24, 2012 at 11:26 AM, Eric Rescorla <ekr@rtfm.com> wrote: >> Wan-Teh, are you able to to do this, or should I pick it back up? > > I haven't had time to do this, sorry. > > I think we probably should first make the current SSL_SetSRTPCiphers > usable. Its only major problem is the "all or nothing" behavior. If > we can change it to fail only if none of the SRTP ciphers are > supported, then that would be an acceptable solution to me. > Alternatively we can consider using the cipher suite array as an > in/out argument, and let the caller determine if the actual enabled > ciphers are acceptable. The first of these seems easiest. Basically, all that's required seems to be to (1) have PRBool found = PR_FALSE; at the beginning, (2) Invert the test on 1629 to read if (*srtpCipher) { found = PR_TRUE; break;} (3) change line 1638 to read if (!found || ...) ... Do you need me to make a patch for this? > I still think doing the SSL_SetSRTPCipherPref approach might be > better, but I think it is also advantageous to make the current > approach work, to allow testing. Because of the backward > compatibility of NSS, we will need to be careful not to leak the > SSL_SetSRTPCiphers function to the DLL export file. WFM -Ekr
On Thu, May 24, 2012 at 3:56 PM, Eric Rescorla <ekr@rtfm.com> wrote: > On Thu, May 24, 2012 at 11:46 AM, Wan-Teh Chang <wtc@google.com> wrote: >> >> I think we probably should first make the current SSL_SetSRTPCiphers >> usable. Its only major problem is the "all or nothing" behavior. If >> we can change it to fail only if none of the SRTP ciphers are >> supported, then that would be an acceptable solution to me. >> Alternatively we can consider using the cipher suite array as an >> in/out argument, and let the caller determine if the actual enabled >> ciphers are acceptable. > > The first of these seems easiest. Basically, all that's required seems to > be to > > (1) have PRBool found = PR_FALSE; at the beginning, > (2) Invert the test on 1629 to read if (*srtpCipher) { found = PR_TRUE; break;} > (3) change line 1638 to read if (!found || ...) ... > > Do you need me to make a patch for this? No, I can make this patch. I just wanted to run this idea by you. Basically, the issue is whether "all ciphers in the input array are acceptable" or "some of the ciphers in the input array are mandatory". I think the latter is unlikely to be true because ultimately, if we are willing to allow the peer to select any ciphers that we offer, then that means all ciphers in the input array are acceptable. Does my reasoning make sense? Wan-Teh
Ekr: please review patch set 8. You can just review the delta between patch sets 7 and 8. Thanks. I fixed the all-or-nothing behavior of SSL_SetSRTPCiphers.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/9982019/65002
Try job failure for 9982019-65002 on win for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/9982019/65002
Change committed as 140535 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
