Issue 276360043: TLS 1.3 draft-10 1-rtt first review

ekr-rietveld

MT, Can you please take a look at this. Here's what I know is missing. ...

10 years, 2 months ago (2015-12-07 04:48:38 UTC) #2

ekr-rietveld

Wan-Teh, here is a first cut. See upthread for caveats.

10 years, 2 months ago (2015-12-07 20:23:20 UTC) #3

mt

This looks pretty good on the whole. I have a few nits. I'm not really ...

10 years, 2 months ago (2015-12-08 03:22:27 UTC) #5

ekr-rietveld

MT, PTAL at patch #3. https://codereview.appspot.com/276360043/diff/1/cmd/platrules.mk File cmd/platrules.mk (right): https://codereview.appspot.com/276360043/diff/1/cmd/platrules.mk#newcode23 cmd/platrules.mk:23: ifdef NSS_ENABLE_TLS_1_3 On 2015/12/08 ...

10 years, 2 months ago (2015-12-15 19:16:33 UTC) #6

mt

LGTM https://codereview.appspot.com/276360043/diff/1/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/276360043/diff/1/lib/ssl/ssl3con.c#newcode6388 lib/ssl/ssl3con.c:6388: rv = ssl3_ComputeHandshakeHashes(ss, ss->ssl3.pwSpec, &hashes, 0); On 2015/12/15 ...

10 years, 2 months ago (2015-12-16 05:27:39 UTC) #7

LGTM

https://codereview.appspot.com/276360043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/276360043/diff/1/lib/ssl/ssl3con.c#newcode6388
lib/ssl/ssl3con.c:6388: rv = ssl3_ComputeHandshakeHashes(ss, ss->ssl3.pwSpec,
&hashes, 0);
On 2015/12/15 19:16:31, ekr-webrtc wrote:
> On 2015/12/08 03:22:25, mt wrote:
> > I'd rather just switch the spec in the if statement.  The difference between
> > cwSpec and pwSpec is easy to miss.
> 
> Can you explain what you mean here?

type* spec;
if (isTLS13) {
    /* In TLS 1.3, we are already encrypted. */
    spec = ss->ssl3.cwSpec;
} else {
    spec = ss->ssl3.pwSpec;
}
rv = ssl3_ComputeHandshakeHashes(ss, spec, &hashes, 0);

https://codereview.appspot.com/276360043/diff/1/lib/ssl/ssl3con.c#newcode7258
lib/ssl/ssl3con.c:7258: ssl3_ParseCertificateRequestCAs(sslSocket *ss,SSL3Opaque
**b, PRUint32 *length,
On 2015/12/15 19:16:31, ekr-webrtc wrote:
> On 2015/12/08 03:22:25, mt wrote:
> > I can't wait until we get clang-format.  This is a bit of a mess.  I can't
see
> > any actual changes though.
> 
> Not sure clang-format would help. I think the formatting hasn't changed and
> Rietveld is just sad about the big code block being moved.

It's not the moving, it's the whitespace, which is a bit of a shambles.

Reviewboard might actually be able to handle this.

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode208
lib/ssl/tls13con.c:208: tls13_GetHash(sslSocket *ss)
On 2015/12/15 19:16:32, ekr-webrtc wrote:
> On 2015/12/08 03:22:26, mt wrote:
> > I suggested this elsewhere, but you need to call this more consistently. 
You
> > can probably remove a whole lump of PORT_Assert(h == ssl_hash_sha256) at the
> > same time.
> 
> I only saw like one place. Please point out others.

Ahh, it is a combination of this and the CKM_..._SHA256 variant.  This is less
common.

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode251
lib/ssl/tls13con.c:251: return SECFailure;  /* Error code set below */
On 2015/12/15 19:16:32, ekr-webrtc wrote:
> On 2015/12/08 03:22:26, mt wrote:
> > s/below/by caller/
> 
> Below here means in the function below. That's the convention here.

Hmm, while I agree that your arrangement makes sense, I can't guarantee that
someone won't add functions between these and muck up your comments.

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode1320
lib/ssl/tls13con.c:1320: * be subtleties so retain the restriction for now.
On 2015/12/15 19:16:33, ekr-webrtc wrote:
> On 2015/12/08 03:22:26, mt wrote:
> > I can't imagine there being a problem here: the signature_algorithms
extension
> > is no more or less vulnerable to tampering than its set of offered cipher
> > suites.
> > 
> > The unfortunate part of your choice here is that it's hard to fix up.  I
think
> > that we should just do it right from the outset.  That simply means
> maintaining
> > a handshake hash for every hash that we offer in signature_algorithms. 
Then,
> > look up the peer's choice and use that.  It will fail if the peer selects a
> > signature algorithm you haven't configured.
> 
> It's not a matter of tampering, just that getting the code right here is
tricky.
> I don't think that this is particularly hard to fix up later as opposed to
now.

OK, not sure that I agree, but let's try to land this ASAP.

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode1360
lib/ssl/tls13con.c:1360: /* TODO(ekr@rtfm.com): verify that the key & kea match
*/
On 2015/12/15 19:16:32, ekr-webrtc wrote:
> On 2015/12/08 03:22:26, mt wrote:
> > You should do this.  However, most of that checking is already done in
> > ssl3_CheckSignatureAndHashAlgorithmConsistency.  You could just add a check
to
> > ssl3_CheckSignatureAndHashAlgorithmConsistency for - on a client only -
> checking
> > that kea_def->signType matches sigAndHash.signAlg.
> 
> Hmm... I'm not quite following. Can you provide a code snippet?

Add to ssl3_CheckSignatureAndHashAlgorithmConsistency, right at the top:

if (ss->sec.isServer && 
    ss->ssl3.kea_def->signType != sigAndHash->sigAlg) {
    PORT_SetError(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM);
    return SECFailure;
}

I think that's enough & correct.

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode1414
lib/ssl/tls13con.c:1414: rv = PK11_DigestOp(hmacCtx, (unsigned char *)label,
strlen(label) + 1); /* Include the \0 */
On 2015/12/15 19:16:32, ekr-webrtc wrote:
> On 2015/12/08 03:22:26, mt wrote:
> > You should be able to reduce this to just the second cast.
> 
> Casting off const

Yes, but you do two casts.  Change label to unsigned char *label

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/Makefile
File lib/ssl/Makefile (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/Makefile#newcode26
lib/ssl/Makefile:26: # (4) Include "local" platform-dependent assignments
(OPTIONAL).      #q
"q" oops

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/dtlscon.c
File lib/ssl/dtlscon.c (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1168: if (crSpec->epoch != epoch) {
Check indent here.

Sign in to reply to this message.

wtc1

Review comments on patch set 3: I have reviewed the "easy" files in lib/ssl. I ...

10 years, 2 months ago (2015-12-23 00:37:01 UTC) #8

ekr-rietveld

Thanks for the thorough review. I will try to update these files tomorrow to match ...

10 years, 2 months ago (2015-12-23 01:00:29 UTC) #9

Thanks for the thorough review. I will try to update these files tomorrow to
match this review, but I wanted to respond right away to several of the points
in case it is relevant.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/SSLerrs.h#newcode451
lib/ssl/SSLerrs.h:451: "SSL received a malformed ECDHE key share handshake
message.")
On 2015/12/23 00:36:58, wtc1 wrote:
> Is ECDHE key share a "handshake message" or "extension"? Same for line 454.


It is an extension. I will fix this.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/SSLerrs.h#newcode453
lib/ssl/SSLerrs.h:453: ER3(SSL_ERROR_RX_MALFORMED_DHE_KEY_SHARE, (SSL_ERROR_BASE
+ 141),
On 2015/12/23 00:36:58, wtc1 wrote:
> These two error code are named SSL_ERROR_RX_MALFORMED_ECDHE_SHARE and
> SSL_ERROR_RX_MALFORMED_DHE_KEY_SHARE, without the "_KEY".

Oops. Thanks.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode472
lib/ssl/ssl3ecc.c:472: SECKEYPublicKey **peerKeyP)
On 2015/12/23 00:36:59, wtc1 wrote:
> The final "P" in "peerKeyP" is uncommon in NSS. I suggest something like
> "outPeerKey" or "peerKeyPtr".
> 
> This also applies to the "sharedP" parameter of tls13_ComputeECDHSharedKey().
> 
> UPDATE: this function can just return SECKEYPublicKey *. A null pointer return
> value indicates failure. Similarly, tls13_ComputeECDHSharedKey() can just
return
> PK11SymKey *.

I can do that. My usual convention is to always return values, but I am happy to
do it this way.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode530
lib/ssl/ssl3ecc.c:530: PORT_FreeArena(arena, PR_FALSE);
On 2015/12/23 00:36:58, wtc1 wrote:
> This should be:
>     if (arena) {
>         PORT_FreeArena(arena, PR_FALSE);
>     }

Is this needed? Looking at
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secport....,
it appears that if arena == NULL, then PORT_FreeArena() is a no-op. Are you
concerned about the cost of the subroutine call?

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1974: handshakeMessage)) {
On 2015/12/23 00:36:59, wtc1 wrote:
> 1. IMPORTANT: move this check after the duplicate extension check so that the
> duplicate extension check is performed. I.e., this check should be right
before
> the for loop that invokes the handlers.

Willdo.


> 2. Whether an extension is allowed in TLS 1.3 or not can also be checked by
the
> individual hello extension handlers. I assume you considered that design and
> concluded it is easier to do that check centrally?

Yes, I'm generally trying to consolidate logic where I can, and also I prefer
tables to logic.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1979: tls13_FatalError(ss, SSL_ERROR_RX_MALFORMED_HANDSHAKE,
On 2015/12/23 00:37:00, wtc1 wrote:
> Do we have a better error code than SSL_ERROR_RX_MALFORMED_HANDSHAKE, such as
> SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION? It may be good to add one
> (SSL_ERROR_EXTENSION_DISALLOWED_FOR_VERSION).

Willdo.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2020: if (tls13_ExtensionAllowed(ex_type, server_hello)) {
On 2015/12/23 00:37:00, wtc1 wrote:
> Just wanted to check my understanding: if an extension is allowed in both
> server_hello and encrypted_extension, we must send the extension in
> server_hello. Is this correct?

This shouldn't happen. Extensions should be allowed in one or the other but not
both. Perhaps an assert?

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2757: /* This currently only works for ECC keys */
On 2015/12/23 00:37:00, wtc1 wrote:
> IMPORTANT: return SECFailure if pair->pubKey->keyType != ecKey.

Good point. Though this should have failed elsewhere, I believe. Maybe an
asserT?

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2863: * array for future use.*/
On 2015/12/23 00:36:59, wtc1 wrote:
> Nit: what does "array" refer to?
> 
> UPDATE: I believe "and copy to an array for future use" is only correct on the
> server side.

It is actually correct on either side, I believe. But it's not an array, it's a
linked list. Will fix.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2894: tls13_ServerHandleKeyShareXtn(sslSocket *ss, PRUint16
ex_type, SECItem *data)
On 2015/12/23 00:37:00, wtc1 wrote:
> IMPORTANT: why can't we call tls13_HandleClientKeyShare immediately in this
> function?

This code is invoked prior to negotiating the cipher suites.

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...


https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...


> If it is necessary to handle the client key share in two phases, please add a
> comment to point out this function only merely saves the client key shares and
> they are later handled by tls13_HandleClientKeyShare.

Willdo.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2907: /* Redundant length because of TLS encoding */
On 2015/12/23 00:36:59, wtc1 wrote:
> Nit: the "redundant" in this comment is a little confusing. I think this
length
> is the vector length, right?
> 
>                 case client:
>                   KeyShareEntry client_shares<4..2^16-1>;

Correct.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2943: * Can't happen. */
On 2015/12/23 00:37:00, wtc1 wrote:
> IMPORTANT: "Can't happen" and the PORT_Assert(0) are only correct when we test
> an NSS client against an NSS server. Other TLS 1.3 client implementations may
> support the finite field DHE key shares.

I claim that this is not true because we have an error check in the code that
stages the keys (in tls13con.c) for the cipher suite.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h#newcode214
lib/ssl/sslerr.h:214: SSL_ERROR_RX_MALFORMED_DHE_SHARE = (SSL_ERROR_BASE + 141),
On 2015/12/23 00:37:00, wtc1 wrote:
> SSL_ERROR_RX_MALFORMED_DHE_SHARE is not used. Do we need it?

It's a placeholder for later.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h#newcode214
lib/ssl/sslerr.h:214: SSL_ERROR_RX_MALFORMED_DHE_SHARE = (SSL_ERROR_BASE + 141),
On 2015/12/23 00:37:00, wtc1 wrote:
> SSL_ERROR_RX_MALFORMED_DHE_SHARE is not used. Do we need it?

This is a placeholder for when we add FFDHE

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h#newcode216
lib/ssl/sslerr.h:216: SSL_ERROR_RX_UNEXPECTED_ENCRYPTED_EXTENSIONS =
(SSL_ERROR_BASE + 142),
On 2015/12/23 00:37:00, wtc1 wrote:
> Should we also add SSL_ERROR_RX_UNEXPECTED_HELLO_RETRY_REQUEST?

Yes. We will need that later.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h#newcode219
lib/ssl/sslerr.h:219: SSL_ERROR_KEY_EXCHANGE_FAILURE = (SSL_ERROR_BASE + 144),
On 2015/12/23 00:37:00, wtc1 wrote:
> Key exchange failure is not new in TLS 1.3. What error code are we using for
key
> exchange failure right now?

I will investigate.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode966
lib/ssl/sslimpl.h:966: PRBool                dontHashMessage; /* Suppress
hashing on output. Hack */
On 2015/12/23 00:37:01, wtc1 wrote:
> 1. Nit: in general it can be confusing to name a boolean in a negative sense.
> Even something like "disableMessageHash" or "skipMessageHash" could avoid the
> potential confusion.
> 
> 2. Why is this a hack? Please elaborate.

It's just gross to set some state variable and use that to disable whether we
hash stuff we would otherwise hash.

I believe that this code will no longer be needed in draft-11 since we actually
hash Finished.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode...
lib/ssl/sslimpl.h:1770: SECStatus tls13_EncodeECDHEKeyShare(sslSocket *ss,
ssl3KeyPair *pair);
On 2015/12/23 00:37:01, wtc1 wrote:
> Why aren't these tls13_xxx functions declared in tls13con.h? It doesn't bother
> me. I'm just wondering if this is an oversight.

Because they are implemented in ssl3ecc.c and the convention seems to be to
declare those functions in sslimpl.h.

Do you think we should pull the ssl3ecc.c fxns out into a (new) ssl3ecc.h?

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslt.h
File lib/ssl/sslt.h (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslt.h#newcode240
lib/ssl/sslt.h:240: ssl_tls13_key_share_xtn          = 40,      /* unofficial
TODO(ekr@rtfm.com) */
On 2015/12/23 00:37:01, wtc1 wrote:
> 1. ssl_tls13_key_share_xtn => ssl_key_share_xtn ?

This was MT's suggestion. I could go either way.


> 
> ssl_signature_algorithms_xtn isn't named ssl_tls12_signature_algorithms_xtn,
> even though it can only be used in TLS 1.2 or higher.
> 
> 2. Nit: this line seems longer than 80 characters. You can shorten it by using
> just "ekr" in the TODO.

Yes.


> 3. Draft 10 added three new extensions (not counting supported_groups). Does
> this mean you haven't implemented early_data and pre_shared_key?

Correct.

Sign in to reply to this message.

ekr-rietveld

New version. https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode251 lib/ssl/tls13con.c:251: return SECFailure; /* Error code set below ...

10 years, 2 months ago (2015-12-23 22:14:21 UTC) #10

New version.

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode251
lib/ssl/tls13con.c:251: return SECFailure;  /* Error code set below */
On 2015/12/16 05:27:38, mt wrote:
> On 2015/12/15 19:16:32, ekr-webrtc wrote:
> > On 2015/12/08 03:22:26, mt wrote:
> > > s/below/by caller/
> > 
> > Below here means in the function below. That's the convention here.
> 
> Hmm, while I agree that your arrangement makes sense, I can't guarantee that
> someone won't add functions between these and muck up your comments.

I don't love this either, but it's the convention all through ssl3con.c

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode1360
lib/ssl/tls13con.c:1360: /* TODO(ekr@rtfm.com): verify that the key & kea match
*/
On 2015/12/16 05:27:38, mt wrote:
> On 2015/12/15 19:16:32, ekr-webrtc wrote:
> > On 2015/12/08 03:22:26, mt wrote:
> > > You should do this.  However, most of that checking is already done in
> > > ssl3_CheckSignatureAndHashAlgorithmConsistency.  You could just add a
check
> to
> > > ssl3_CheckSignatureAndHashAlgorithmConsistency for - on a client only -
> > checking
> > > that kea_def->signType matches sigAndHash.signAlg.
> > 
> > Hmm... I'm not quite following. Can you provide a code snippet?
> 
> Add to ssl3_CheckSignatureAndHashAlgorithmConsistency, right at the top:
> 
> if (ss->sec.isServer && 
>     ss->ssl3.kea_def->signType != sigAndHash->sigAlg) {
>     PORT_SetError(SSL_ERROR_INCORRECT_SIGNATURE_ALGORITHM);
>     return SECFailure;
> }
> 
> I think that's enough & correct.
> 

This code wasn't quite right. See my new code.

https://codereview.appspot.com/276360043/diff/1/lib/ssl/tls13con.c#newcode1414
lib/ssl/tls13con.c:1414: rv = PK11_DigestOp(hmacCtx, (unsigned char *)label,
strlen(label) + 1); /* Include the \0 */
On 2015/12/16 05:27:38, mt wrote:
> On 2015/12/15 19:16:32, ekr-webrtc wrote:
> > On 2015/12/08 03:22:26, mt wrote:
> > > You should be able to reduce this to just the second cast.
> > 
> > Casting off const
> 
> Yes, but you do two casts.  Change label to unsigned char *label

Otherwise you are strlening an unsigned char *.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/Makefile
File lib/ssl/Makefile (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/Makefile#newcode26
lib/ssl/Makefile:26: # (4) Include "local" platform-dependent assignments
(OPTIONAL).      #q
On 2015/12/23 00:36:58, wtc1 wrote:
> Please remove this file from the changeset.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/SSLerrs.h
File lib/ssl/SSLerrs.h (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/SSLerrs.h#newcode453
lib/ssl/SSLerrs.h:453: ER3(SSL_ERROR_RX_MALFORMED_DHE_KEY_SHARE, (SSL_ERROR_BASE
+ 141),
On 2015/12/23 01:00:28, ekr-webrtc wrote:
> On 2015/12/23 00:36:58, wtc1 wrote:
> > These two error code are named SSL_ERROR_RX_MALFORMED_ECDHE_SHARE and
> > SSL_ERROR_RX_MALFORMED_DHE_KEY_SHARE, without the "_KEY".
> 
> Oops. Thanks.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/dtlscon.c
File lib/ssl/dtlscon.c (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1154: * DTLS relevance checks:
On 2015/12/23 00:36:58, wtc1 wrote:
> Please document the return value and the output argument seqNum

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1165: DTLSEpoch epoch = (cText->seq_num.high >> 16) & 0xffff;
On 2015/12/23 00:36:58, wtc1 wrote:
> Nit: the & 0xffff is not necessary.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1168: if (crSpec->epoch != epoch) {
On 2015/12/16 05:27:39, mt wrote:
> Check indent here.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1169: SSL_DBG(("%d: SSL3[%d]: HandleRecord, received packet "
On 2015/12/23 00:36:58, wtc1 wrote:
> This debug message is copied from ssl3_HandleRecord. It should be updated to
say
> "dtls_IsRelevant" instead of "HandleRecord".
> 
> Also on line 1178.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:12753: PK11_FreeSymKey(ss->ssl3.hs.finishedSecret);
On 2015/12/23 00:36:58, wtc1 wrote:
> 1. It is not clear whether we need to free the symmetric keys.
> However, we should initialize them to NULL.

I moved this to ssl3_DestroySSL3Info.


> 2. We should call PR_INIT_CLIST(&ss->ssl3.hs.remoteKeyShares) and initialize
> ss->ssl3.hs.dontHashMessage to PR_FALSE.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode375
lib/ssl/ssl3ecc.c:375: tls13_SizeofECDHEKeyShare(ssl3KeyPair *pair)
On 2015/12/23 00:36:59, wtc1 wrote:
> The function name should suggest that this is referring to
> the key_exchange field of key_exchange field of a KeyShareEntry
> in a KeyShare structure, which for ECDHE is:
> 
>    opaque point <1..2^8-1>;
> 
> The current function name implies this is referring to a KeyShare
> structure.
> 
> At least, this should be documented with a comment.
> 
> This issue also applies to the tls13_EncodeECDHEKeyShare function.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode393
lib/ssl/ssl3ecc.c:393: ecdhePub->u.ec.publicValue.len, 1);
On 2015/12/23 00:36:59, wtc1 wrote:
> We can just return ssl3_AppendHandshakeVariable().

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode472
lib/ssl/ssl3ecc.c:472: SECKEYPublicKey **peerKeyP)
On 2015/12/23 01:00:28, ekr-webrtc wrote:
> On 2015/12/23 00:36:59, wtc1 wrote:
> > The final "P" in "peerKeyP" is uncommon in NSS. I suggest something like
> > "outPeerKey" or "peerKeyPtr".
> > 
> > This also applies to the "sharedP" parameter of
tls13_ComputeECDHSharedKey().
> > 
> > UPDATE: this function can just return SECKEYPublicKey *. A null pointer
return
> > value indicates failure. Similarly, tls13_ComputeECDHSharedKey() can just
> return
> > PK11SymKey *.
> 
> I can do that. My usual convention is to always return values, but I am happy
to
> do it this way.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode488
lib/ssl/ssl3ecc.c:488: if (length || ec_point.len < 1) {
On 2015/12/23 00:36:59, wtc1 wrote:
> Nit: ec_point.len < 1  =>  ec_point.len == 0
> 
> ec_point.len is an unsigned int.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode515
lib/ssl/ssl3ecc.c:515: if (rv != SECSuccess)
On 2015/12/23 00:36:58, wtc1 wrote:
> Nit: use curly braces

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode519
lib/ssl/ssl3ecc.c:519: if (SECITEM_CopyItem(arena, &peerKey->u.ec.publicValue,
&ec_point)) {
On 2015/12/23 00:36:58, wtc1 wrote:
> Test SECITEM_CopyItem() != SECSuccess

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ecc.c#newcode545
lib/ssl/ssl3ecc.c:545: target = CKM_NSS_TLS_MASTER_KEY_DERIVE_DH_SHA256;
On 2015/12/23 00:36:59, wtc1 wrote:
> IMPORTANT: we should use the official PKCS #11 mechanisms for TLS 1.2.

Don't we actually want CKM_NSS_HKDF_SHA256?

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode288
lib/ssl/ssl3ext.c:288: { ssl_tls13_key_share_xtn,       
&tls13_ClientHandleKeyShareXtn },
On 2015/12/23 00:37:00, wtc1 wrote:
> Nit: the second field (&tls13_ClientHandleKeyShareXtn) should be aligned with
> line 286 rather than line 287. Or we don't align it at all.

Aligned, but this may change with clang-format later.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1922: * in TLS 1.3, the client checks that extensions appear
in the
On 2015/12/23 00:37:00, wtc1 wrote:
> Nit: in TLS 1.3 => In TLS 1.3

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1941: PORT_Assert(handshakeMessage == server_hello);
On 2015/12/23 00:36:59, wtc1 wrote:
> Nit: it would be better to do it like this:
> 
>     case encrypted_extensions:
>         PORT_Assert(ss->version >= SSL_LIBRARY_VERSION_TLS_1_3);
>         /* fall through */
>     case server_hello:
>         if (ss->version > SSL_LIBRARY_VERSION_3_0) {
>             handlers = serverHelloHandlersTLS;
>         } else {
>             handlers = serverHelloHandlersSSL3;
>         }
>         break;

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1946: PORT_Assert(0);
On 2015/12/23 00:36:59, wtc1 wrote:
> IMPORTANT: add PORT_SetError(SEC_ERROR_LIBRARY_FAILURE). We should never
return
> SECFailure without setting an error code.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1967: if (!ssl3_ClientExtensionAdvertised(ss, extension_type))
{
On 2015/12/23 00:37:00, wtc1 wrote:
> We can replace the nested if statements with &&, like the original code. Also,
> did you mean to delete the comment in the original code?

No, pilot error.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1974: handshakeMessage)) {
On 2015/12/23 01:00:28, ekr-webrtc wrote:
> On 2015/12/23 00:36:59, wtc1 wrote:
> > 1. IMPORTANT: move this check after the duplicate extension check so that
the
> > duplicate extension check is performed. I.e., this check should be right
> before
> > the for loop that invokes the handlers.
> 
> Willdo.
> 
> 
> > 2. Whether an extension is allowed in TLS 1.3 or not can also be checked by
> the
> > individual hello extension handlers. I assume you considered that design and
> > concluded it is easier to do that check centrally?
> 
> Yes, I'm generally trying to consolidate logic where I can, and also I prefer
> tables to logic.

I moved the check but can you explain why it's better It seems that we do check
for duplicates either way.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2020: if (tls13_ExtensionAllowed(ex_type, server_hello)) {
On 2015/12/23 01:00:29, ekr-webrtc wrote:
> On 2015/12/23 00:37:00, wtc1 wrote:
> > Just wanted to check my understanding: if an extension is allowed in both
> > server_hello and encrypted_extension, we must send the extension in
> > server_hello. Is this correct?
> 
> This shouldn't happen. Extensions should be allowed in one or the other but
not
> both. Perhaps an assert?
> 

Assert added.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2729: *     Section 6.3.2.3.
On 2015/12/23 00:37:00, wtc1 wrote:
> Add internet draft name or at least "TLS 1.3".

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2739: *                 KeyShareEntry
client_shares<4..2^16-1>;
On 2015/12/23 00:37:00, wtc1 wrote:
> Nit: the minimum length of 4 implies |key_exchange| is zero-length, right?

This does seem like a potential bug in the specification, though I suppose you
might have a group with an empty value somehow.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2747: tls13_SizeofKeyShareEntry(ssl3KeyPair *pair)
On 2015/12/23 00:37:00, wtc1 wrote:
> Nit: capitalize "Of"?

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2757: /* This currently only works for ECC keys */
On 2015/12/23 01:00:28, ekr-webrtc wrote:
> On 2015/12/23 00:37:00, wtc1 wrote:
> > IMPORTANT: return SECFailure if pair->pubKey->keyType != ecKey.
> 
> Good point. Though this should have failed elsewhere, I believe. Maybe an
> asserT?

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2791: extension_length = 2 + 2 + 2 + entry_length; /* Type +
length + vector_length + entry */
On 2015/12/23 00:36:59, wtc1 wrote:
> Please fold all the lines longer than 80 characters in the rest of this file.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2816: PORT_Assert(extension_length > 0);
On 2015/12/23 00:36:59, wtc1 wrote:
> This assertion is not useful because extension_length = 2 + 2 + ..., which is
> clearly > 0.
> 
> This also applies to the assertion on line 2970.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2824: 
On 2015/12/23 00:37:00, wtc1 wrote:
> Nit: just one blank line between functions.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2844: ks = PORT_ZAlloc(sizeof(TLS13KeyShareEntry));
On 2015/12/23 00:36:59, wtc1 wrote:
> Use PORT_ZNew(TLS13KeyShareEntry)

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2863: * array for future use.*/
On 2015/12/23 01:00:29, ekr-webrtc wrote:
> On 2015/12/23 00:36:59, wtc1 wrote:
> > Nit: what does "array" refer to?
> > 
> > UPDATE: I believe "and copy to an array for future use" is only correct on
the
> > server side.
> 
> It is actually correct on either side, I believe. But it's not an array, it's
a
> linked list. Will fix.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2871: return SECFailure;
On 2015/12/23 00:36:59, wtc1 wrote:
> Set an error code. See also my comment for line 1979.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2892: an array for future use.*/
On 2015/12/23 00:36:59, wtc1 wrote:
> 1. I believe "an array" refers to ss->ssl3.hs.remoteKeyShares, which is really
a
> (circular) linked list. Perhaps we should just say "copy to
> ss->ssl3.hs.remoteKeyShares for future use".
> 
> 2. Nit: start the second line of comment with a '*'.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2913: goto loser;
On 2015/12/23 00:37:00, wtc1 wrote:
> IMPORTANT: set an error code (SSL_ERROR_RX_MALFORMED_KEY_SHARE).

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h#newcode216
lib/ssl/sslerr.h:216: SSL_ERROR_RX_UNEXPECTED_ENCRYPTED_EXTENSIONS =
(SSL_ERROR_BASE + 142),
On 2015/12/23 01:00:29, ekr-webrtc wrote:
> On 2015/12/23 00:37:00, wtc1 wrote:
> > Should we also add SSL_ERROR_RX_UNEXPECTED_HELLO_RETRY_REQUEST?
> 
> Yes. We will need that later, but maybe hold off?

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslerr.h#newcode219
lib/ssl/sslerr.h:219: SSL_ERROR_KEY_EXCHANGE_FAILURE = (SSL_ERROR_BASE + 144),
On 2015/12/23 01:00:29, ekr-webrtc wrote:
> On 2015/12/23 00:37:00, wtc1 wrote:
> > Key exchange failure is not new in TLS 1.3. What error code are we using for
> key
> > exchange failure right now?
> 
> I will investigate.

In the previous code we had SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE and
SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE, but this is shared code. Also, the
semantics of those errors are already confusing. For instance,
CLIENT_KEY_EXCHANGE_FAILURE is used here in processing a CKE:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...
and here in generating one:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3ecc.c...

Thoughts?

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode791
lib/ssl/sslimpl.h:791: wait_invalid
On 2015/12/23 00:37:01, wtc1 wrote:
> Since there is no handshake message named "Invalid", the wait_invalid state
> should be documented.

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode840
lib/ssl/sslimpl.h:840: SECItem share;     /* The share itself */
On 2015/12/23 00:37:01, wtc1 wrote:
> I suggest we name this field |key_exchange| to match the definition in the
> internet draft:
> 
>       struct {
>           NamedGroup group;
>           opaque key_exchange<1..2^16-1>;
>       } KeyShareEntry;

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode856
lib/ssl/sslimpl.h:856: SSL3WaitState         ws;  /* May also contain
SSL3WaitState | 0x80 for TLS 1.3 */
On 2015/12/23 00:37:01, wtc1 wrote:
> Nit: it may be better to use a boolean (unsigned char?) instead of OR'ing 0x80
> with an SSL3WaitState enum value.

The idea here is to fit this into the same slot so that the SSLv3 and TLS 1.3
state machines can't cross. I.e., if you do ws = <foo> it won't be in an
acceptable state for the TLS 1.3 state checks even if the message <foo> is
received.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode966
lib/ssl/sslimpl.h:966: PRBool                dontHashMessage; /* Suppress
hashing on output. Hack */
On 2015/12/23 01:00:29, ekr-webrtc wrote:
> On 2015/12/23 00:37:01, wtc1 wrote:
> > 1. Nit: in general it can be confusing to name a boolean in a negative
sense.
> > Even something like "disableMessageHash" or "skipMessageHash" could avoid
the
> > potential confusion.
> > 
> > 2. Why is this a hack? Please elaborate.
> 
> It's just gross to set some state variable and use that to disable whether we
> hash stuff we would otherwise hash.
> 
> I believe that this code will no longer be needed in draft-11 since we
actually
> hash Finished.
> 

Depending on the timing, I may actually update this patch to do -11 in which
case this will go away.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode...
lib/ssl/sslimpl.h:1713: ECName ssl3_Params2ecName(SECKEYECParams *params);
On 2015/12/23 00:37:01, wtc1 wrote:
> Capitalize "ECName".

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode...
lib/ssl/sslimpl.h:1769: unsigned int tls13_SizeofECDHEKeyShare(ssl3KeyPair
*pair);
On 2015/12/23 00:37:01, wtc1 wrote:
> Nit: capitalize "Of"?

Done.

https://codereview.appspot.com/276360043/diff/40001/lib/ssl/sslimpl.h#newcode...
lib/ssl/sslimpl.h:1981: SECStatus
ssl3_CompleteHandleCertificateRequest(sslSocket *ss, SECItem *algorithms,
On 2015/12/23 00:37:01, wtc1 wrote:
> Nit: this line seems longer than 80 characters.

Done.

Sign in to reply to this message.

ekr-rietveld

https://codereview.appspot.com/276360043/diff/80001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/276360043/diff/80001/lib/ssl/ssl3con.c#newcode2935 lib/ssl/ssl3con.c:2935: PORT_Assert(ss->ssl3.cwSpec->version < SSL_LIBRARY_VERSION_TLS_1_3); Do we need this? DTLS calls ...

10 years, 1 month ago (2015-12-31 20:11:37 UTC) #11

ekr-rietveld

WTC, MT. I think this is ready for review of draft-11. In parallel I will ...

10 years, 1 month ago (2015-12-31 20:20:59 UTC) #12

mt

We're reaching the point of diminishing returns. Landing this would be a good idea in ...

10 years, 1 month ago (2016-01-04 07:42:23 UTC) #13

ekr-rietveld

MT, I did some of these but haven't pushed them yet. See below for some ...

10 years, 1 month ago (2016-01-05 00:54:41 UTC) #15

mt

LGTM https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newcode299 lib/ssl/tls13con.c:299: /* TODO(ekr@rtfm.com): Would it be better to check ...

10 years, 1 month ago (2016-01-05 01:58:16 UTC) #16

LGTM

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:299: /* TODO(ekr@rtfm.com): Would it be better to check all
the states here? */
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > Probably.  You would need a mapping table from state to message, but that
> would
> > be less error-prone.
> 
> Maybe a TODO for later.

Acknowledged.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:361: expectedGroup = ssl3_GetCurveNameForServerSocket(ss);
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > Looking at this again, I don't think that this is the right design for this
> > method.  This wouldn't allow for P-256 and P-384 on the same server socket,
> for
> > example.
> > 
> > I think that what you want to do is go through your locally configured
groups.
> 
> > Filter out those that don't match the cipher suite.  Then pick the first one
> > that you find a remote share for.  Similar logic is used for generating
> > signatures:
> >
>
https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f...
> > 
> > This might be OK for the current hard-coded P-256, but you could more easily
> > generate throwaway code for that.
> 
> I think there are two attitudes here.
> 
> 1. We should select the client's share that we like best (if one is
available).
> 2. Pick the group we like best and insist on it.
> 
> We've talked about both in the WG. This enacts #2, so I think it's OK

Yes, I agree.

Just to confirm, I just read through ssl3_GetCurveNameForServerSocket and I
don't like it very much.  It tries to match the curve to the "strength" of the
cert.  That has some ill effects, particularly if you use an "extra-strong"
cert.

I would rather have a list of curves that we prefer and to pick the first from
that list.  I'll open a bug.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:451: kCertRequestContextLen);
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > I think that this would be better as a simple, one octet counter.  We don't
> even
> > have to increment or store it until we decide to support multiple
> post-handshake
> > client authentications.
> 
> Given that I have to have a slot anyway for the client, what's the cost here?

Cycles, bytes, more code.  You could just as easily populate the slot with a
zero byte.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:612: }
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > I think that you will want to move the key setup stuff here (the last two
> blobs)
> > into a separate function.
> 
> Can you clarify which chunks you mean?

Everything here on lines 597-612 and below on lines 716-729 is the same.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:688:
&ss->ephemeralECDHKeyPair->pubKey->u.ec.DEREncodedParams);
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > This is often referred to as a train wreck.  Is there any way to make this
> less
> > crap?  SECKEY_* functions that allow you to make some query of the key
> perhaps?
> 
> I am not sure. Perhaps Wan-Teh could help?

SECKEY_GetECCOid() doesn't help.  Though it should have been used in
ssl3_Params2ECName().  Simplest thing would be to change the signature of
ssl3_Params2ECName() to accept the public key.  That would simplify all the call
sites for that as well (as far as I can see).

Sign in to reply to this message.

ttaubert

Sorry, took me a while. LGTM, didn't find anything other than a few nits. https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest/ssl_loopback_unittest.cc ...

10 years, 1 month ago (2016-01-05 20:35:54 UTC) #17

ekr-rietveld

Note to self: MUST add Karthik's anti-downgrade stuff https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newcode451 lib/ssl/tls13con.c:451: kCertRequestContextLen); ...

10 years, 1 month ago (2016-01-05 23:25:00 UTC) #18

wtc1

Review comments on patch set 6: I only reviewed ssl3con.c this time. You can check ...

10 years, 1 month ago (2016-01-06 04:53:57 UTC) #19

ekr-rietveld

Wan-Teh Thank you for your thorough review. Upon a quick skim I think I agree ...

10 years, 1 month ago (2016-01-06 05:02:51 UTC) #20

wtc1

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcode12337 lib/ssl/ssl3con.c:12337: SSL3_SendAlert(ss, alert_fatal, record_overflow); IMPORTANT: if the comment "must not ...

10 years, 1 month ago (2016-01-06 06:37:42 UTC) #21

wtc1

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcode9118 lib/ssl/ssl3con.c:9118: /* Random already generated in ssl3_HandleClientHello */ On 2016/01/06 ...

10 years, 1 month ago (2016-01-06 15:17:51 UTC) #22

ekr-rietveld

MT, WTC, TTaubert, please see updated patch. https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest/ssl_loopback_unittest.cc File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right): https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest/ssl_loopback_unittest.cc#newcode375 external_tests/ssl_gtest/ssl_loopback_unittest.cc:375: // Disabled ...

10 years, 1 month ago (2016-01-18 22:57:54 UTC) #23

MT, WTC, TTaubert, please see updated patch.

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:375: // Disabled will
EncryptedExtensions is done.
On 2016/01/05 20:35:53, ttaubert wrote:
> "Disabled until ..."

This actually works now.

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_agent.cc:424: SECStatus rv = SSL_OptionSet(ssl_fd_,
SSL_ENABLE_DEFLATE, PR_TRUE);
On 2016/01/05 00:54:40, ekr-webrtc wrote:
> On 2016/01/04 07:42:22, mt wrote:
> > Similar functions have a call to EnsureTlsSetup() here, why the change?
> 
> I think it must just have been called at a different time? Added for
> completeness

Done.

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_agent.h:108: return info_.compressionMethod !=
ssl_compression_null; }
On 2016/01/05 20:35:53, ttaubert wrote:
> Nit: Put closing bracket on the next line?

Done.

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_connect.cc:202: ASSERT_EQ(TlsAgent::STATE_ERROR,
server_->state());
On 2016/01/05 20:35:53, ttaubert wrote:
> Nit: indentation seems off

Done.

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/tls_hkdf_unittest.cc (right):

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_hkdf_unittest.cc:51: class ScopedKey {
On 2016/01/05 20:35:54, ttaubert wrote:
> You should be able to use ScopedPK11SymKey if you include
> external_tests/common/scoped_ptrs.h

Done.

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_hkdf_unittest.cc:95: <<
static_cast<unsigned>(buf[i]);
On 2016/01/04 07:42:22, mt wrote:
> DataBuffer has a print statement.  The only downside being that it needs a
copy
> of the data, but that's not a big deal.

Done.

https://codereview.appspot.com/276360043/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_hkdf_unittest.cc:119: if (slot_) {
PK11_FreeSlot(slot_); }
On 2016/01/05 20:35:53, ttaubert wrote:
> ScopedPK11SlotInfo

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/dtlscon.c
File lib/ssl/dtlscon.c (right):

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/dtlscon.c#newcod...
lib/ssl/dtlscon.c:1178: ((PRUint64)cText->seq_num.low);
On 2016/01/05 20:35:54, ttaubert wrote:
> nit: indentation

This is the indentation that Emacs wants, but I did something

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/dtlscon.c#newcod...
lib/ssl/dtlscon.c:1183: /* Silently drop the packet */
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Delete this comment. This comment describes the action of the caller, and the
> calleralready has this comment.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcode63
lib/ssl/ssl3con.c:63: static SECStatus
ssl3_CompleteHandleHandshakeMessage(sslSocket *ss,
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> ssl3_CompleteHandleHandshakeMessage is a confusing name. At first I thought it
> was analogous to ssl3_AuthCertificateComplete and performed the remaining work
> of ssl3_HandleHandshakeMessage if we couldn't complete the work immediately.
But
> it turns out to handle all the handshake messages except client_hello and
> server_hello.

Renamed "ssl3_HandlePostHelloHandshakeMessage"

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:556: CK_MECHANISM_TYPE ssl3_alg2Mech(SSLCipherAlgorithm calg)
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> 1. IMPORTANT: are you sure ssl3_alg2Mech() only needs to be defined when TRACE
> is defined?
> 
> 2. Nit: capitalize "alg": ssl3_Alg2Mech

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2895: PORT_Assert(ss->ssl3.cwSpec->version <
SSL_LIBRARY_VERSION_TLS_1_3);
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: this assertion doesn't seem useful because this condition is ensured by
the
> ss->ssl3.cwSpec->version < SSL_LIBRARY_VERSION_TLS_1_1 test on line 2868.
> 
> UPDATE: I see, this assertion mirrors the if statement on line 2923.

Nevertheless I think you're right. Removed.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2926: PR_FALSE,
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nice catch!


MT caught this.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2935: rv = dtls_CompressMACEncryptRecord(ss, epoch,
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: it is unfortunate that the corresponding if-else for the DTLS case is
> hidden inside dtls_CompressMACEncryptRecord(). I don't have a good suggestion.

OK. I think I will leave as-is but add ac omment.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:4595: ss->ssl3.hs.kea_def->signKeyType != sigAndHash->sigAlg)
{
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> I think a comment for this check would be useful. I can't figure out why this
> check is added, and why this check doesn't apply to the server side.

MT added this code. MT?

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:5584: * ssl3 Hello Request.
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Please reformat lines 5583-5584. Line 5583 seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:5607: (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3)) {
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> IMPORTANT: ss->version >= SSL_LIBRARY_VERSION_TLS_1_3 is never true. This
> function can assert ss->version < SSL_LIBRARY_VERSION_TLS_1_3.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6377: goto done;  /* rv already == SECFailure */
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> 1. BUG: we cannot go to |done| without ssl_ReleaseSpecReadLock(ss). We should
> just fall through and rely on the code on lines 6402-6404.
> 
> 2. |rv| is the return value of ssl3_ComputeBackupHandshakeHashes(). Does this
> comment imply ssl3_ComputeBackupHandshakeHashes() always fails for TLS >= 1.3?
> 
> I wonder if you meant to write this:
> 
>     if (isTLS13) {
>         /* rv already == SECFailure */
>         PORT_SetError(...);
>     } else {
>         rv = ssl3_ComputeBackupHandshakeHashes(ss, &hashes);
>         PORT_Assert(!ss->ssl3.hs.backupHash);
>     }

Note: this branch should never appear with TLS 1.3 (see the assert above). This
is just cleanup when the assert doesn't engage. Made the change you suggested.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6388: */
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Nit: use the NSS multi-line comment format:
> 
>   /* line 1
>    * line 2
>    * line 3
>    */

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6426: /* TODO(ekr@rtfm.com): Rename these or conditionalize?
*/
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> I would just add a check for |ss| being a client socket.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6562: goto loser;        /* alert has been sent */
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> I suggest we use just one or two spaces before the comment.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6571: sidBytes.len = 0;
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Since |sidBytes| is initialized to {siBuffer, NULL, 0}, it is not necessary to
> set sidBytes.len to 0 or sidBytes.data to NULL.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6862: rv = tls13_HandleServerKeyShare(ss);
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Would it be useful to add a TODO comment to note this is where we would handle
> server early_data for a zero-RTT exchange?

I'm not sure this is actually where we would.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6864: goto alert_loser;
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Just wanted to confirm: if the server key_share is invalid (i.e.,
> tls13_HandleServerKeyShare fails), you want to respond with an
> "illegal_parameter" alert, correct?

Yes.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6888: /* Called from ssl3_CompleteHandleHandshakeMessage()
when it has deciphered a complete
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> This line seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7266: ssl3_ParseCertificateRequestCAs(sslSocket *ss,SSL3Opaque
**b, PRUint32 *length,
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> 1. Please document this function.
> 
> 2. Nit: add a space after "sslSocket *ss,"

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7300: (*b) += len;
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: it's not necessary to parenthesize |*b| in these two lines.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7331: (void)SSL3_SendAlert(ss, alert_fatal, decode_error);
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> The original code sends an illegal_parameter alert for SSL 3.0. I think we
need
> to preserve that.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7392: goto loser;             /* alert sent below */
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> 1. The code under |loser| will overwrite the error with
> SSL_ERROR_RX_MALFORMED_CERT_REQUEST. I think we should go to |done| instead.
> 
> 2. "alert sent below" seems wrong. Did you mean to say the alert was sent
inside
> ssl3_ParseCertificateRequestCAs()?

Done

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7402: if (rv != SECSuccess) {
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> BUG?: I think this if statement should be deleted, because |rv| may be
> SECWouldBlock. Alternatively, this if statement should test |rv ==
SECFailure|.

Set to == SECFailurel.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7496: return SECSuccess;
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> BUG?: I think this should be |return rv| may return SECWouldBlock if |rv| is
> SECWouldBlock.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7571: /* Called from ssl3_CompleteHandleHandshakeMessage()
when it has deciphered a complete
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> This line seems longer than 80 characters. Please search for
> "ssl3_CompleteHandleHandshakeMessage" in this file and check for > 80 chars
> lines.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8010: desc = no_renegotiation;
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> IMPORTANT: TLS 1.3 doesn't use the no_renegotiation alert (it becomes
> no_renegotiation_RESERVED). So we need to use a different value of |desc| for
> TLS 1.3. Perhaps unexpected_message? Note that unexpected_message is always
> fatal, so it'll need to sent with |level == alert_fatal|, which is already
> initialized that way.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8068: * when we process the ClientKeyShare in TLS 1.3 */
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Since the ssl3_InitHandshakeHashes() call on line 8069 doesn't need the server
> random, I suggest you move this code right before the related "grab the client
> random data" code at line 8082.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8072: return rv;
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> IMPORTANT: the code inside the if block should be:
>     errCode = SSL_ERROR_GENERATE_RANDOM_FAILURE;
>     goto loser;
> 
> You copied the code verbatim from ssl3_SendServerHello. I believe the copied
> code is also correct here, because we haven't acquired any locks, but it is
> better to not have to worry about releasing locks.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8783: sid = ssl3_NewSessionID(ss, PR_TRUE);
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> It doesn't really matter, but would this be a better place to handle the
> ClientKeyShare extension in TLS 1.3, so that we handle all the extensions
before
> creating |sid|?

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8801: goto alert_loser;
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Please confirm that it is correct to send a fatal illegal_parameter alert in
> this case. (It seems correct to me.)

Yes, I believe this is correct.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8806: if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: this conditional expression is used twice. It may be good to cache the
> result in a PR_BOOL isTLS13 local variable.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9089: &ss->xtnData.serverHelloSenders[0]);
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9118: /* Random already generated in ssl3_HandleClientHello */
On 2016/01/06 15:17:51, wtc1 wrote:
> 
> On 2016/01/06 04:53:55, wtc1 wrote:
> > 
> > I think this comment can be omitted because it seems only useful to people
who
> > know the original code.
> 
> On second thought, I think this comment is a good idea because it
> helps the reader verify that server_random has been generated before
> we get here.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9131: rv = ssl3_AppendHandshakeNumber(ss, 0, 1);
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Please add curly braces to this if-else statement.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9141: /* TLS 1.3 doesn't include compression in the
ServerHello. */
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> 1. I think these two comments (line 9125 and here) should be moved up, to
lines
> 9093-9101.
> 
> 2. Nit: compression => compression_method ?
> 
> The latter is the name of the field in the TLS spec.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9556: void ssl3_GetCertificateRequestCAs(sslSocket *ss, int
*calenp, SECItem **namesp,
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: this should be formatted as follows:
> 
> void
> ssl3_GetCertificateRequestCAs(sslSocket *ss, int *calenp, SECItem **namesp,

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9561: SECItem *names = NULL;
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Since this function never fails (it returns void), we can just use the three
> output arguments directly and don't need these three local variables. If we do
> that, then the output parameters don't need the trailing |p| in their names.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9586: SECStatus
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: the |static| in the original code can be preserved. I leave this up to
you.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9597: int i;
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Nit: |i| should be aligned with the other local variables.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10262: len = ss->ssl3.hs.certReqContextLen + 1;
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> 1. Should we check that ss->ssl3.hs.certReqContextLen <= 255?

I suggest we assert it, because it's a can't happen.

> 
> 2. Typo in draft-11: in this paragraph:
> 
>    certificate_request_context:
>       If this message is in response to a CertificateRequest, the value
>       if certificate_request_context in that message.
> 
> "if" should be changed to "is".

I think "of" actually.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10408: int                  len 		= 0;
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> 1. Nit: |len| should be renamed |certLen| or |certChainLen|.

Done.

> 2. Nit: it is a little confusing that |len| doesn't include the three-byte
> length but |contextLen| includes the one-byte length. It is OK to not change
> this.

Will leave.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10460: contextLen += ss->ssl3.hs.certReqContextLen;
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Should we check that ss->ssl3.hs.certReqContextLen <= 255?

Done with an assert.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10477: rv = ssl3_AppendHandshakeHeader(ss, certificate, len +
contextLen + 3);
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Nit: len + contextLen + 3 => contextLen + len + 3

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10596: /* Called from ssl3_CompleteHandleHandshakeMessage()
when it has deciphered a complete
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10617: ssl3_CompleteHandleCertificateStatus(sslSocket *ss,
SSL3Opaque *b, PRUint32 length)
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10661: 
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Nit: no need to add this blank line.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10662: /* Called from ssl3_CompleteHandleHandshakeMessage()
when it has deciphered a complete
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10688: 
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Delete this blank line.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10967: /* Ephemeral suites require ServerKeyExchange. Export
cipher suites
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: there seems to be spaces at the end of this line.

I think Rietveld is confused.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10976: ss->ssl3.hs.ws = wait_cert_request; /* disallow
server_key_exchange */
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: line 10974 and this line seem longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:11440: /* Called from ssl3_CompleteHandleHandshakeMessage()
when it has deciphered a complete
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:11717: computeHashes = TLS13_IN_HS_STATE(ss,
wait_cert_request, wait_cert_verify);
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> IMPORTANT: why do we also allow the wait_cert_request state?

The order used to be Certificate/CertificateRequest/CertificateVerify. Fixed.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:11719: if (type == finished) {
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Nit: this can be "else if"

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:11720: computeHashes = TLS13_IN_HS_STATE(ss,
wait_cert_request, wait_finished);
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: lines 11712, 11717, and this line seem longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:11722: }
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Nit: the block of code that sets computeHashes doesn't need to be inside the
> ssl_GetSpecReadLock(ss) critical section.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:11842: ssl3_CompleteHandleHandshakeMessage(sslSocket *ss,
SSL3Opaque *b, PRUint32 length,
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> This line looks longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12250: ssl3_UnprotectRecord(sslSocket* ss, SSL3Ciphertext
*cText, sslBuffer *plaintext)
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: sslSocket* ss => sslSocket *ss

Right. C++ versus C

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12300: ssl_ReleaseSpecReadLock(ss);
On 2016/01/06 15:17:51, wtc1 wrote:
> 
> Two more comments about this function.
> 
> 1. Please check the related tls13_UnprotectRecord() function for similar
> bugs.
> 
> 2. Please document that the caller of this function holds the spec read
> lock, and point out that this function must not call SSL3_SendAlert
> because of that.

Here's what I did:

1. Have the Unprotect functions take an outparam that indicates the alert to be
sent.
2. Require the Unprotect functions to call PORT_SetError()

Then the calling code:
1. Sends an appropriate alert (for TLS).
2. Returns SECFailure.

Note: I changed several pieces of behavior.

1. In TLS < 1.3, we now send internal_error and set SEC_ERROR_LIBRARY_FAILURE
when cipher_def->iv_size is bogus, since
this is an internal failure (though the attempt to send the alert
will probably fail too).

2. I provided more detailed errors and alerts for TLS 1.3 because anything
that's not an AEAD failure isn't sensitive.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12337: SSL3_SendAlert(ss, alert_fatal, record_overflow);
On 2016/01/06 06:37:42, wtc1 wrote:
> 
> IMPORTANT: if the comment "must not hold spec lock when calling
SSL3_SendAlert"
> is correct, then we also need to remove all SSL3_SendAlert() calls from this
> function, and let the caller call SSL3_SendAlert() based on the error code.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12444: return SECFailure;
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> Add PORT_SetError(SSL_ERROR_DECRYPTION_FAILURE) before returning SECFailure.
> 
> Note that sslerr.h says we should not use SSL_ERROR_DECRYPTION_FAILURE. We may
> want to update that comment, or add a comment here to point out that the
caller
> will immediately replace the error code with SSL_ERROR_BAD_MAC_READ.


This doesn't seem to be used anywhere. I think my refactor does the right thing.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12546: /* always log mac error, in case attacker can read
server logs. */
On 2016/01/06 04:53:55, wtc1 wrote:
> 
> IMPORTANT: resurrect the PORT_SetError(SSL_ERROR_BAD_MAC_READ) call.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12553: }
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> IMPORTANT: this whole block of code should read:
> 
>     if (rv != SECSuccess) {
>         /* must not hold spec lock when calling SSL3_SendAlert. */ <= I think
> this comment can be deleted.
>         ssl_ReleaseSpecReadLock(ss);
> 
>         if (PORT_GetError() != SSL_ERROR_DECRYPTION_FAILURE) {
>             return rv;
>         }
> 
>         SSL_DBG(("%d: SSL3[%d]: decryption failed", SSL_GETPID(), ss->fd));
> 
>         if (!IS_DTLS(ss)) {
>             SSL3_SendAlert(ss, alert_fatal, bad_record_mac);
>             /* always log mac error, in case attacker can read server logs. */
>             PORT_SetError(SSL_ERROR_BAD_MAC_READ);
>             return SECFailure;
>         } else {
>             /* Silently drop the packet */
>             databuf->len = 0; /* Needed to ensure data not left around */
>             // XXX should we call PORT_SetError(0) here, to leave
>             // absolutely no trace of decryption failure? It doesn't
>             // seem necessary, because one is not supported to call
>             // PORT_GetError() after an NSS function returns
>             // successfully.
>             return SECSuccess;
>         }
>     } 

I did something different here. Specifically, I made every unprotect failure
generate an alert, thus avoiding the need to check the error.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12562: isTLS = (PRBool)(crSpec->version >
SSL_LIBRARY_VERSION_3_0);
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: it looks nicer to do this immediately after line 12511 (where |crSpec| is
> assigned).

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12789: PR_INIT_CLIST(&ss->ssl3.hs.remoteKeyShares);
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> 1. Also initialize certReqContextLen to 0?
> 
> 2. Nit: I suggest you initialize these fields in the same order as they are
> declared in sslimpl.h:
> 
> +    PRCList               remoteKeyShares; /* The other side's public keys */
> +    PK11SymKey            *xSS;            /* Extracted static secret */
> +    PK11SymKey            *xES;            /* Extracted ephemeral secret */
> +    PK11SymKey            *trafficSecret;  /* The source key to use to
encrypt
> traffic */
> +    PK11SymKey            *clientFinishedSecret; /* Used for client Finished
*/
> +    PK11SymKey            *serverFinishedSecret; /* Used for server Finished
*/
> +    unsigned char         certReqContext[255]; /* Ties CertificateRequest
> +                                                * to Certificate */
> +    unsigned char         certReqContextLen;   /* Length of the context */

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:13216: PK11_FreeSymKey(ss->ssl3.hs.xSS);
On 2016/01/06 04:53:56, wtc1 wrote:
> 
> Nit: reverse the destruction of xES and xSS, to match their declaration order
in
> sslimpl.h.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/sslimpl.h#newcod...
lib/ssl/sslimpl.h:969: unsigned char         certReqContextLen;   /* Length of
the context */
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: unsigned char => PRUint8

Note: this makes the asserts we added for length useless, so I took
them back out.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/sslimpl.h#newcod...
lib/ssl/sslimpl.h:1967: /***** Exported functions for TLS 1.3 ********/
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: this comment can be confusing and can easily become out of date. I
suggest
> removing this comment.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:612: }
On 2016/01/05 01:58:16, mt wrote:
> On 2016/01/05 00:54:41, ekr-webrtc wrote:
> > On 2016/01/04 07:42:23, mt wrote:
> > > I think that you will want to move the key setup stuff here (the last two
> > blobs)
> > > into a separate function.
> > 
> > Can you clarify which chunks you mean?
> 
> Everything here on lines 597-612 and below on lines 716-729 is the same.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:688:
&ss->ephemeralECDHKeyPair->pubKey->u.ec.DEREncodedParams);
On 2016/01/05 01:58:16, mt wrote:
> On 2016/01/05 00:54:41, ekr-webrtc wrote:
> > On 2016/01/04 07:42:23, mt wrote:
> > > This is often referred to as a train wreck.  Is there any way to make this
> > less
> > > crap?  SECKEY_* functions that allow you to make some query of the key
> > perhaps?
> > 
> > I am not sure. Perhaps Wan-Teh could help?
> 
> SECKEY_GetECCOid() doesn't help.  Though it should have been used in
> ssl3_Params2ECName().  Simplest thing would be to change the signature of
> ssl3_Params2ECName() to accept the public key.  That would simplify all the
call
> sites for that as well (as far as I can see).

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:729: }
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > You have exactly the same block here as above, just in a different order. 
> Make
> > a function of it?
> 
> I think that will make things more complicated because there are other places
> where we init and then don't increment.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:957: } while(0)
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > I'm almost surprised that you didn't use a macro to construct the string. 
> It's
> > only a few more lines of code.
> 
> Honestly, I didn't think it through. Good idea!

Turns out it doesn't work as well.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:1083: pwSpec->epoch << 16;
On 2016/01/04 07:42:22, mt wrote:
> Every time I see this nasty stuff I wonder if it isn't worth the effort to
> change the sequence numbers to PRUint64.

File a bug.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:1100: if (install & InstallCipherSpecRead ||
On 2016/01/05 00:54:41, ekr-webrtc wrote:
> On 2016/01/04 07:42:23, mt wrote:
> > You didn't use &-operator elsewhere, so this is probably a mistake.
> 
> Oh good catch!

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:1266: static const int   tagSize = 16;
On 2016/01/04 07:42:23, mt wrote:
> gratuitous ws

Done. Also moved it to above the assert.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:1393: rv = TLS13_CHECK_HS_STATE(ss,
SSL_ERROR_RX_UNEXPECTED_CERT_VERIFY, wait_cert_verify);
On 2016/01/04 07:42:23, mt wrote:
> Excessively long line.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:1402: }
On 2016/01/04 07:42:23, mt wrote:
> Both branches of this statement seem to be identical.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:1633: if (!ss->sec.isServer) {
On 2016/01/04 07:42:23, mt wrote:
> Invert the logic, no need to negate.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:1749: idle_handshake);
On 2016/01/04 07:42:23, mt wrote:
> indent

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.h
File lib/ssl/tls13con.h (right):

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.h#newco...
lib/ssl/tls13con.h:26: PRBool tls13_InHsState(sslSocket *ss, ...);
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Please document tls13_InHsState and TLS13_IN_HS_STATE

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.h#newco...
lib/ssl/tls13con.h:30: void tls13_FatalError(sslSocket *ss, PRErrorCode prError,
SSL3AlertDescription desc);
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: lines 24, 27 and this line seem longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/tls13con.h#newco...
lib/ssl/tls13con.h:51: #endif
On 2016/01/06 04:53:57, wtc1 wrote:
> 
> Nit: add a /* __tls13con_h_ */ comment and delete the next two blank lines.

Done.

Sign in to reply to this message.

ekr-rietveld

Wan-Teh, I didn't do quite what you suggested with the unprotect code, so some study ...

10 years, 1 month ago (2016-01-18 22:58:47 UTC) #24

mt

https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/276360043/diff/100001/lib/ssl/ssl3con.c#newcode4595 lib/ssl/ssl3con.c:4595: ss->ssl3.hs.kea_def->signKeyType != sigAndHash->sigAlg) { On 2016/01/18 22:57:52, ekr-webrtc wrote: ...

10 years, 1 month ago (2016-01-18 23:05:08 UTC) #25

mt

Are you going to land this? I see a lot of stuff here that needs ...

10 years, 1 month ago (2016-01-21 01:18:25 UTC) #26

ekr-rietveld

Given WTC's extensive comments, I'd like to have his LGTM on ssl3con.c. Other than that, ...

10 years, 1 month ago (2016-01-21 02:30:28 UTC) #27

ttaubert

LGTM https://codereview.appspot.com/276360043/diff/120001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/276360043/diff/120001/lib/ssl/ssl3con.c#newcode6387 lib/ssl/ssl3con.c:6387: if (!isTLS13) { nit: maybe invert the logic ...

10 years, 1 month ago (2016-01-21 22:39:35 UTC) #28

ekr-rietveld

https://codereview.appspot.com/276360043/diff/120001/external_tests/ssl_gtest/ssl_loopback_unittest.cc File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right): https://codereview.appspot.com/276360043/diff/120001/external_tests/ssl_gtest/ssl_loopback_unittest.cc#newcode331 external_tests/ssl_gtest/ssl_loopback_unittest.cc:331: DisableDheCiphers(); // We don't support FFDHE for TLS 1.3 ...

10 years, 1 month ago (2016-01-22 00:30:30 UTC) #29

wtc1

Patch set 8 LGTM. 1. I didn't review the tests and the four new tls13* ...

10 years, 1 month ago (2016-01-24 19:49:49 UTC) #30

ekr-rietveld

This addresses WTC's review comments. MT, can you please review this diff prior to commit. ...

10 years, 1 month ago (2016-01-24 22:39:35 UTC) #31

This addresses WTC's review comments. MT, can you please review this diff prior
to commit.

Special notes:

WTC, MT: Can you please review my handling of the cText buffer expansion in
ssl3_HandleRecord rather than ssl3_UnprotectRecord?

MT: Can you please review the setting of the key bits.

MT: I found a bug in the case where we talk to a TLS 1.2 server. the new tests
in this version (from patch 8) test that and verify the fix, which also slightly
simplifies tls13con.c. Can you please double-check that it is OK to have
ssl3_UnprotectRecord handle the null case even in TLS 1.3

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/dtlscon.c
File lib/ssl/dtlscon.c (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/dtlscon.c#newcod...
lib/ssl/dtlscon.c:1172: SSL_DBG(("%d: SSL3[%d]: HandleRecord, received packet "
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: HandleRecord => dtls_IsRelevant

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcode66
lib/ssl/ssl3con.c:66: SSL3Hashes *hashesPtr);
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: these three parameters seem under-indented.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:2933: /* TLS 1.2 and TLS 1.3 cases are both handled in
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: "TLS 1.2" is a little too specific. We should say
> something like "TLS <= 1.2" or "pre-TLS 1.3" ...

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:4595: * the cipher suite. */
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: this comment probably should say "the signature algorithm matches the
> cipher suite" or "the signature algorithm matches the signing key type of
> the cipher suite".

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6390: rv = ssl3_ComputeBackupHandshakeHashes(ss, &hashes);
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> BUG: this branch (when isTLS13 is true) should be the error case.
> We can only use the backup hash when isTLS13 is false.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6424: if (rv == SECSuccess) {
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: maybe we can combine the nested if statements into one:
> 
>   if (rv == SECSuccess && !ss->sec.isServer) {

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6569: goto loser;        /* alert has been sent */
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: I suggest we use just one or two spaces before the comment.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6577: 
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: delete the spaces in this line.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:7343: ss->version <= SSL_LIBRARY_VERSION_TLS_1_0 ?
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> BUG: the condition should use "<" instead of "<=" because
> decode_error should be used for TLS 1.0 or higher.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8022: if (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) {
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: set the isTLS13 earlier, so that you can just check
> isTLS13 here.

Unfortunately, this doesn't work because ss->version will be reset when we do
ssl3_NegotiateVErsion on line 8076, so we need to distinguish the before and
after cases. I am reluctant to overload isTLS13 like that. Would you prefer I
did so?

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:8026: } else if (ss->opt.enableRenegotiation ==
SSL_RENEGOTIATE_NEVER) {
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: this "else" can be omitted because of the goto in the
> previous line. I leave this up to you.

i agree.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:9582: int i;
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> IMPORTANT: we need to initialize all the output arguments:
> 
>     *calen = 0;  // This replaces line 9595.
>     *names = NULL;
>     *nnames = 0;

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10274: PRBool isTLS13     = PR_FALSE;
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: align the equal signs?

I opted to remove the linear whitespace.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10924: /* set the server authentication types and sizes from
the value
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: I think "types" and "sizes" should now be singular
> "type" and "size", right?

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10986: /* require server_key_exchange */
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: this line seems over-indented.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10989: /* disallow server_key_exchange */
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: this line seems over-indented.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10991: /* This is static RSA so set the key bits to auth bits.
*/
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: add "exchange" after "key"

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:10992: ss->sec.keaKeyBits = ss->sec.authKeyBits;
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> This is a new change. Please make sure you or MT reviewed
> this change because I didn't check it.

Thank you. I will ask MT.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:11847: rv = tls13_HandlePostHelloHandshakeMessage(ss, b,
length, hashesPtr);
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: is this line longer than 80 characters?

Yes. 81. Fixed.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12266: * Therefore, we MUST not call SSL3_SendAlert().
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> Nit: I suggest we make this paragraph the last paragraph,
> after we describe the failure behavior of this function.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12269: * 1. Set |*alert| to the alert to be sent.
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> IMPORTANT: what if we don't need to send an alert for some error? What does
this
> function do in that case?

I think in this case we would overload alert and use 0 (close_notify) to mean
"no alert". However, we do not need to address that now.

> UPDATE: it seems that the only case where we don't need to send an alert
> is when cipher_def->iv_size is bogus. In that case, you chose to send an
> internal_error alert. I think that is fine.

Correct. This is a new alert, but I believe that it is better to send an alert
here in any case.

correCorreCo

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12270: * 2. Call PORT_SetError() witn an appropriate code.
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: witn => with

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12557: }
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> This code (which grows the |plaintext| buffer if necessary)
> was in ssl3_UnprotectRecord() in patch set 6. Please
> confirm that it should be here.

This is intentional. I made the change because of the concern about sending an
alert that you raised above. However, upon reflection I
realize that this is not necessary in TLS 1.3 mode because TLS 1.3
decryption never results in a larger block than the input record (because
compression is forbidden). So I wonder if it would be better to move this into
ssl3_UnprotectRecord() and make use of the close_notify hack that I  suggested
above.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12575: return SECFailure;
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: in many places in NSS, we take care to set the error
> code right before we return failure. The reason is that
> the error cleanup actions (such as freeing resources, logging an
> error message, or sending an alert) may change the error
> code. It would be nice to do that here (which would
> require adding an |errCode| outparam to the
> UnprotectRecord function). I'm not sure if it's worth the
> trouble though.

I believe that you have found a bug here. SSL3_SendAlert() may reset the error
code as a side effect. I have updated the patch to address this by extracting
the error code and then resetting it after PORT_SetError().

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:12583: * successfully. */
On 2016/01/24 19:49:47, wtc1 wrote:
> 
> This "XXX" comment was a question for you :-) Since you
> didn't add a PORT_SetError(0) call, let's just delete the
> comment.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ecc.c#newcod...
lib/ssl/ssl3ecc.c:193: ssl3_Pubkey2ECName(SECKEYPublicKey* pubKey)
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> 1. Nit: Pubkey => PubKey
> 
> This is how the abbreviation is spelled in NSS function
> names.

Fixed

> 2. Nit: placement of *

Fixed.


> 3. This change seems unnecessary. It made the function less general.

MT pointed out that throughout the code we were calling
this function with pubKey->u.ec.DEREncodedParams and so
this simplified the rest of the code.


> Did you plan to check the key type in this function?
> 
>     SECKEYECParams *params;
> 
>     if (pubKey->keyType != ecKey) {
>         return ec_noName;
>     }
> 
>     params = &pubKey->u.ec.DEREncodedParams;

It's an invariant but I added a check and an assert as well.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ecc.c#newcod...
lib/ssl/ssl3ecc.c:376: * the KeyShare structure. */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: KeyShare => KeyShareEntry

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ecc.c#newcod...
lib/ssl/ssl3ecc.c:385: * the KeyShare structure. */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: KeyShare => KeyShareEntry

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ecc.c#newcod...
lib/ssl/ssl3ecc.c:397: ecdhePub->u.ec.publicValue.len, 1);
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: I suggest using a different local variable:
> 
>     const SECItem *publicValue;
> 
>     ... the two assertions ...
> 
>     publicValue = &pair->pubKey->u.ec.publicValue;
>     return ssl3_AppendHandshakeVariable(ss, publicValue->data,
>                                         publicValue->len, 1);

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ecc.c#newcod...
lib/ssl/ssl3ecc.c:532: tls13_ComputeECDHSharedKey(sslSocket* ss,
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> This function can just return PK11SymKey *. A null pointer
> return value indicates failure.

Done

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:1970: if (!ss->sec.isServer &&
!ssl3_ClientExtensionAdvertised(ss, extension_type)) {
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: this line seems longer than 80 characters.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:2766: /* This currently only works for ECC keys */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Move this comment before the assertion.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:2879: PORT_SetError(SEC_ERROR_LIBRARY_FAILURE); /* Ignored
above. */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> This comment is too terse. I believe you meant to explain why this
> condition implies an NSS bug ("library failure").
> 
> What does "Ignored above" mean? Ignored by the caller?

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3ext.c#newcod...
lib/ssl/ssl3ext.c:2985: PORT_Assert(extension_length > 0);
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Please remove this assertion. It is clearly true because
> extension_length is 2 + 2 + ... > 0.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslerr.h
File lib/ssl/sslerr.h (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslerr.h#newcode214
lib/ssl/sslerr.h:214: SSL_ERROR_RX_MALFORMED_DHE_SHARE = (SSL_ERROR_BASE + 141),
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> it: unless you think it will make the error codes too long,
> I think we should use _KEY_SHARE consistently.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslimpl.h#newcod...
lib/ssl/sslimpl.h:791: wait_invalid /* Invalid value. There is no message
"invalid". */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: message => handshake message

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslimpl.h#newcod...
lib/ssl/sslimpl.h:964: PK11SymKey            *trafficSecret;  /* The source key
to use to encrypt traffic */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> 1. This line seems longer than 80 characters.
> 
> 2. What does "source key" mean? Did you mean "symmetric key"?

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslimpl.h#newcod...
lib/ssl/sslimpl.h:969: PRUint8               certReqContextLen;   /* Length of
the context */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: would be nice to point out that this is a PRUint8 because
> certReqContextLen <= 255.

Done.

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslinfo.c
File lib/ssl/sslinfo.c (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/sslinfo.c#newcode72
lib/ssl/sslinfo.c:72: PR_TRUE : sid->u.ssl3.keys.extendedMasterSecretUsed;
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: this can be simply
>   (ss->version >= SSL_LIBRARY_VERSION_TLS_1_3) ||
> sid->u.ssl3.keys.extendedMasterSecretUsed

Changed. Note that I kept the explicit use of PR_TRUE and
PR_FALSE since that seems to be the convention.


> Does this mean TLS 1.3 doesn't use sid->u.ssl3.keys?

No, it uses it. It merely doesn't need to reference extendedMasterSecretUsed
because TLS 1.3 always derives
keys from a longer transcript

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/tls13con.h
File lib/ssl/tls13con.h (right):

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/tls13con.h#newco...
lib/ssl/tls13con.h:32: #define TLS13_IN_HS_STATE(ss, ...) tls13_InHsState(ss,
__VA_ARGS__, wait_invalid)
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: is this line longer than 80 characters?

Good catch. 81

https://codereview.appspot.com/276360043/diff/140001/lib/ssl/tls13con.h#newco...
lib/ssl/tls13con.h:56: #endif /* __tls13con_h_ */
On 2016/01/24 19:49:48, wtc1 wrote:
> 
> Nit: add a blank line before this.

Done.

Sign in to reply to this message.

mt

Just a reply to the question, I'll get into the review soon. https://codereview.appspot.com/276360043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c ...

10 years, 1 month ago (2016-01-25 02:17:34 UTC) #32

ekr-rietveld

So it's your view I simply shouldn't set this on the client? That's easy enough.

10 years, 1 month ago (2016-01-25 03:29:25 UTC) #33

ekr-rietveld

So it's your view I simply shouldn't set this on the client? That's easy enough.

10 years, 1 month ago (2016-01-25 03:29:30 UTC) #34

mt

On 2016/01/25 03:29:30, ekr-webrtc wrote: > So it's your view I simply shouldn't set this ...

10 years, 1 month ago (2016-01-25 03:42:21 UTC) #35

ekr-rietveld

Oh, I see what you are saying here. I misunderstood. If it's already the case ...

10 years, 1 month ago (2016-01-25 03:45:48 UTC) #36

mt

On 2016/01/25 03:45:48, ekr-webrtc wrote: > Oh, I see what you are saying here. I ...

10 years, 1 month ago (2016-01-25 03:49:30 UTC) #37

mt

LGTM Only one bug and that's easy to fix. The AAD should be empty. I ...

10 years, 1 month ago (2016-01-26 01:33:01 UTC) #38

ekr-rietveld

One immediate response. https://codereview.appspot.com/276360043/diff/200001/lib/ssl/tls13con.c File lib/ssl/tls13con.c (right): https://codereview.appspot.com/276360043/diff/200001/lib/ssl/tls13con.c#newcode1865 lib/ssl/tls13con.c:1865: SSL3SequenceNumber seqNum) On 2016/01/26 01:33:03, mt ...

10 years, 1 month ago (2016-01-26 02:07:38 UTC) #39

ekr-rietveld

https://codereview.appspot.com/276360043/diff/200001/cmd/platrules.mk File cmd/platrules.mk (right): https://codereview.appspot.com/276360043/diff/200001/cmd/platrules.mk#newcode28 cmd/platrules.mk:28: endif On 2016/01/26 01:33:02, mt wrote: > Hmm, I ...

10 years, 1 month ago (2016-01-26 23:07:53 UTC) #40

mt

I think that the keaKeyBits change will break things more this way, since (I think) ...

10 years, 1 month ago (2016-01-27 05:53:39 UTC) #41

ekr-rietveld

Per agreement on bugs and IM, the keaKeyBits change is safe. Fixed the whitespace. https://codereview.appspot.com/276360043/diff/220001/lib/ssl/ssl3con.c ...

10 years, 1 month ago (2016-01-28 15:31:32 UTC) #42

Land it.

Sign in to reply to this message.

Issue 276360043: TLS 1.3 draft-10 1-rtt first review (Closed)

Description

Patch Set 1 #

Patch Set 2 : Updated per MT's comments #

Patch Set 3 : Updated per MT comments #

Patch Set 4 : Updated per MT/WTC comments #

Patch Set 5 : Draft-11 (for self-review) #

Patch Set 6 : Revised from self-review #

Patch Set 7 : Revised per comments #

Patch Set 8 : Updated per MT/TTaubert comments #

Patch Set 9 : Revised per WTC's comments #

Patch Set 10 : Minor editorial changes #

Patch Set 11 : One more tweak #

Patch Set 12 : Revised per MT #

Patch Set 13 : Fix whitespace #

Patch Set 14 : Rebased to master #

Messages