Issue 122370043: Remove compression and prune cipher list
DescriptionRemove compression and prune cipher list
Patch Set 1 #
Total comments: 8
Patch Set 2 : Revised to match Wan-Teh's comments #
Total comments: 1
Patch Set 3 : Rebased #Patch Set 4 : Fix parentheses nit #Patch Set 5 : Rebased #MessagesTotal messages: 8
MT, please sanity check this for me,
Sign in to reply to this message.
LGTM. Not that many valid suites in 1.3, are there... On 11 August 2014 16:50, <ekr-webrtc@rtfm.com> wrote: > Reviewers: martin.thomson_gmail.com, > > Message: > MT, please sanity check this for me, > > Description: > Remove compression and prune cipher list > > Please review this at https://codereview.appspot.com/122370043/ > > Affected files (+19, -6 lines): > M lib/ssl/ssl3con.c > > > Index: lib/ssl/ssl3con.c > =================================================================== > --- a/lib/ssl/ssl3con.c > +++ b/lib/ssl/ssl3con.c > @@ -210,17 +210,21 @@ static const int compressionMethodsCount > static PRBool > compressionEnabled(sslSocket *ss, SSLCompressionMethod compression) > { > switch (compression) { > case ssl_compression_null: > return PR_TRUE; /* Always enabled */ > #ifdef NSS_ENABLE_ZLIB > case ssl_compression_deflate: > - return ss->opt.enableDeflate; > + if (ss->version < SSL_LIBRARY_VERSION_TLS_1_3) { > + return ss->opt.enableDeflate; > + } else { > + return PR_FALSE; > + } > #endif > default: > return PR_FALSE; > } > } > > static const /*SSL3ClientCertificateType */ PRUint8 certificate_types [] = > { > ct_RSA_sign, > @@ -632,24 +636,26 @@ ssl3_CipherSuiteAllowedForVersionRange( > * TLS_DH_anon_EXPORT_WITH_RC4_40_MD5: never implemented > * TLS_DH_anon_EXPORT_WITH_DES40_CBC_SHA: never implemented > */ > return vrange->min <= SSL_LIBRARY_VERSION_TLS_1_0; > > case TLS_DHE_RSA_WITH_AES_256_CBC_SHA256: > case TLS_RSA_WITH_AES_256_CBC_SHA256: > case TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256: > - case TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256: > case TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256: > - case TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256: > case TLS_DHE_RSA_WITH_AES_128_CBC_SHA256: > - case TLS_DHE_RSA_WITH_AES_128_GCM_SHA256: > case TLS_RSA_WITH_AES_128_CBC_SHA256: > case TLS_RSA_WITH_AES_128_GCM_SHA256: > case TLS_RSA_WITH_NULL_SHA256: > + return vrange->max == SSL_LIBRARY_VERSION_TLS_1_2; > + > + case TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256: > + case TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256: > + case TLS_DHE_RSA_WITH_AES_128_GCM_SHA256: > return vrange->max >= SSL_LIBRARY_VERSION_TLS_1_2; > > /* RFC 4492: ECC cipher suites need TLS extensions to negotiate curves > and > * point formats.*/ > case TLS_ECDH_ECDSA_WITH_NULL_SHA: > case TLS_ECDH_ECDSA_WITH_RC4_128_SHA: > case TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA: > case TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA: > @@ -664,20 +670,21 @@ ssl3_CipherSuiteAllowedForVersionRange( > case TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA: > case TLS_ECDH_RSA_WITH_AES_128_CBC_SHA: > case TLS_ECDH_RSA_WITH_AES_256_CBC_SHA: > case TLS_ECDHE_RSA_WITH_NULL_SHA: > case TLS_ECDHE_RSA_WITH_RC4_128_SHA: > case TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA: > case TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA: > case TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA: > - return vrange->max >= SSL_LIBRARY_VERSION_TLS_1_0; > + return vrange->max >= SSL_LIBRARY_VERSION_TLS_1_0 && > + vrange->min < SSL_LIBRARY_VERSION_TLS_1_3; > > default: > - return PR_TRUE; > + return vrange->min < SSL_LIBRARY_VERSION_TLS_1_3; > } > } > > /* return pointer to ssl3CipherSuiteDef for suite, or NULL */ > /* XXX This does a linear search. A binary search would be better. */ > static const ssl3CipherSuiteDef * > ssl_LookupCipherSuiteDef(ssl3CipherSuite suite) > { > @@ -7712,16 +7719,22 @@ ssl3_HandleClientHello(sslSocket *ss, SS > } > > /* grab the list of compression methods. */ > rv = ssl3_ConsumeHandshakeVariable(ss, &comps, 1, &b, &length); > if (rv != SECSuccess) { > goto loser; /* malformed */ > } > > + /* TLS 1.3 requires that compression be empty */ > + if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3_DRAFT_VERSION) { > + if (comps.len != 1 || comps.data[0]) { > + goto loser; > + } > + } > desc = handshake_failure; > > /* Handle TLS hello extensions for SSL3 & TLS. We do not know if > * we are restarting a previous session until extensions have been > * parsed, since we might have received a SessionTicket extension. > * Note: we allow extensions even when negotiating SSL3 for the sake > * of interoperability (and backwards compatibility). > */ > >
Sign in to reply to this message.
Wan-Teh, Here is the second round of TLS 1.3 patches, removing compression and pruning the cipher list. PTAL. P.S. I have some unit tests for these but they are built on top of the GTest framework I built. Doug has asked me to turn that over to Camilo, so it's not ready for landing yet. Let me know if you want me to get you a temporary copy.
Sign in to reply to this message.
Patch set 1 LGTM. I have some questions and suggested changes. https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode220 lib/ssl/ssl3con.c:220: } else { Nit: Please omit "else" after a return statement. This is the common style in NSS. https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode647 lib/ssl/ssl3con.c:647: case TLS_RSA_WITH_AES_128_GCM_SHA256: BUG?: this GCM cipher suite should also be moved to the next group, right? Or is static RSA disallowed in TLS 1.3? https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode7728 lib/ssl/ssl3con.c:7728: if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3_DRAFT_VERSION) { What's the difference between SSL_LIBRARY_VERSION_TLS_1_3 and SSL_LIBRARY_VERSION_TLS_1_3_DRAFT_VERSION? https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode7729 lib/ssl/ssl3con.c:7729: if (comps.len != 1 || comps.data[0]) { Nit: comps.data[0] != ssl_compression_null would be a bit easier to understand.
Sign in to reply to this message.
https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode220 lib/ssl/ssl3con.c:220: } else { On 2014/08/12 04:47:43, wtc wrote: > > Nit: Please omit "else" after a return statement. This is the common style in > NSS. Willdo. https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode647 lib/ssl/ssl3con.c:647: case TLS_RSA_WITH_AES_128_GCM_SHA256: On 2014/08/12 04:47:42, wtc wrote: > > BUG?: this GCM cipher suite should also be moved to the next group, right? Or is > static RSA disallowed in TLS 1.3? Correct. We removed all non-ephemeral cipher suites in TLS 1.3 https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode7728 lib/ssl/ssl3con.c:7728: if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3_DRAFT_VERSION) { On 2014/08/12 04:47:42, wtc wrote: > > What's the difference between SSL_LIBRARY_VERSION_TLS_1_3 and > SSL_LIBRARY_VERSION_TLS_1_3_DRAFT_VERSION? Good catch. This is actually a bug. The former is the TLS 1.3 version number and the latter is a version indicating the version of the TLS 1.3 draft e are implementing. See https://codereview.appspot.com/119690044/ for context. https://codereview.appspot.com/122370043/diff/1/lib/ssl/ssl3con.c#newcode7729 lib/ssl/ssl3con.c:7729: if (comps.len != 1 || comps.data[0]) { On 2014/08/12 04:47:42, wtc wrote: > > Nit: comps.data[0] != ssl_compression_null would be a bit easier to understand. Willdo.
Sign in to reply to this message.
Wan-Teh, I think this is ready to go once you approve issue https://codereview.appspot.com/119690044/
Sign in to reply to this message.
Patch set 2 LGTM! https://codereview.appspot.com/122370043/diff/20001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/122370043/diff/20001/lib/ssl/ssl3con.c#newcode... lib/ssl/ssl3con.c:7728: if (comps.len != 1 || (comps.data[0] != ssl_compression_null)) { Nit: if you parenthesize the second test, please also parenthesize the first test. I think the parentheses can be omitted. Is there some style guide or compiler warning that made you add the parentheses?
Sign in to reply to this message.
On 2014/08/13 21:47:34, wtc wrote: > Patch set 2 LGTM! > > https://codereview.appspot.com/122370043/diff/20001/lib/ssl/ssl3con.c > File lib/ssl/ssl3con.c (right): > > https://codereview.appspot.com/122370043/diff/20001/lib/ssl/ssl3con.c#newcode... > lib/ssl/ssl3con.c:7728: if (comps.len != 1 || (comps.data[0] != > ssl_compression_null)) { > > Nit: if you parenthesize the second test, please also parenthesize the first > test. > > I think the parentheses can be omitted. Is there some style guide or compiler > warning that made you add the parentheses? I just tend to parenthesize, but I know that's not NSS style and I sometimes forget. I will remove.
Sign in to reply to this message.
|
