Bug 1268745 - Limit use of the same symmetric key

Bug 1268745 - Limit use of the same symmetric key

[ekr-rietveld, 2016-08-18 23:30:57]

MT,

Where did you get these limits? I note that TLS 1.3 recommends 2^24.5 not
2^28...

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/libs...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/libs...
external_tests/ssl_gtest/libssl_internals.c:237: return SECSuccess;
You don't actually need to acquire these locks because we are never
multithreaded in these tests.

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc (right):

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:226: limit--;
Good catch.

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:234: // different error
code.  Note that the message that the client sends would not
What error code does it trigger? Just that it looks like a replay? If so, why
not just inject a bogus packet on the wire directly?

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:237:
TEST_P(TlsCipherLimitTest, ReadLimit) {
Would it be better to have two superclasses one for stream and one parametrized
rather than the early return here.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/dtlscon.c#newcode1201
lib/ssl/dtlscon.c:1201: dtls_seq_num = cText->seq_num & 0xffffffUL;
IMPORTANT: Isn't this too short? You need to mask off the low-order 48, not 32
bits.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode293
lib/ssl/ssl3con.c:293: /* Indexed by SSL3BulkCipher. For an analysis of
max_record values, see bug 12... */
NOTE TO SELF: Need to check these limits.

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode304
lib/ssl/ssl3con.c:304: SEC_OID_NULL_CIPHER, "NULL", (1ULL<<48) - 1},
This seems kind of spurious. How did you choose this one?

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode2892
lib/ssl/ssl3con.c:2892: mask = isDTLS ? 0xffffffffffffULL : PR_UINT64_MAX;
Given that all your limits are <= 2^48, why not just make them 2^48-1 and then
you can unconditionally mask off the top bits.

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode3070
lib/ssl/ssl3con.c:3070: wrBuf->buf[10] = (unsigned char)(cwSpec->write_seq_num
>> 0);
Maybe we can use ssl3_EncodeUintX (though it would have to take PRUint64)

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3gthr.c#newcode472
lib/ssl/ssl3gthr.c:472: cText.seq_num |= ss->gs.hdr[3 + i];
maybe a function here.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/sslimpl.h#newcode458
lib/ssl/sslimpl.h:458: typedef PRUint64 SSL3SequenceNumber;
Please get superreview from wtc for this change.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/tls13con.c#newcode3237
lib/ssl/tls13con.c:3237: ptr = ssl_EncodeUintX(seqNum & 0xffffffffUL, 4, ptr);
And a place where a 64-bit version would help

https://codereview.appspot.com/309080043/diff/1/lib/ssl/tls13con.c#newcode3261
lib/ssl/tls13con.c:3261: mask = IS_DTLS(ss) ? 0xffffffffffffULL : PR_UINT64_MAX;
See previous comments about mask

[mt, 2016-08-19 03:56:23]

More and more test coverage

[mt, 2016-08-19 03:57:11]

Patch updated.  I'll ping wtc regarding the change to the sequence number
struct.  I decided to just remove the typedef in the end.  It was barely used
and not consistently.

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc (right):

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:226: limit--;
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> Good catch.

Acknowledged.

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:234: // different error
code.  Note that the message that the client sends would not
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> What error code does it trigger? Just that it looks like a replay? If so, why
> not just inject a bogus packet on the wire directly?

Done.

https://codereview.appspot.com/309080043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:237:
TEST_P(TlsCipherLimitTest, ReadLimit) {
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> Would it be better to have two superclasses one for stream and one
parametrized
> rather than the early return here.

I've decided to just do as you suggested and write directly to the read buffer
for the overflow record.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/dtlscon.c#newcode1201
lib/ssl/dtlscon.c:1201: dtls_seq_num = cText->seq_num & 0xffffffUL;
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> IMPORTANT: Isn't this too short? You need to mask off the low-order 48, not 32
> bits.

Yes, my new tests picked that up.  (This was only 24 bits, a mistake I had made
elsewhere.)

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode293
lib/ssl/ssl3con.c:293: /* Indexed by SSL3BulkCipher. For an analysis of
max_record values, see bug 12... */
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> NOTE TO SELF: Need to check these limits.

As discussed, I've added a comment explaining why the limits are what they are.

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode304
lib/ssl/ssl3con.c:304: SEC_OID_NULL_CIPHER, "NULL", (1ULL<<48) - 1},
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> This seems kind of spurious. How did you choose this one?

We have the NULL cipher suite, remember?  This limit is the maximum (well,
allowing for DTLS).  I originally had it at 1<<10, but that broke.  I figure
there is no real limit for the NULL cipher.

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode2892
lib/ssl/ssl3con.c:2892: mask = isDTLS ? 0xffffffffffffULL : PR_UINT64_MAX;
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> Given that all your limits are <= 2^48, why not just make them 2^48-1 and then
> you can unconditionally mask off the top bits.

Done.

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode3070
lib/ssl/ssl3con.c:3070: wrBuf->buf[10] = (unsigned char)(cwSpec->write_seq_num
>> 0);
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> Maybe we can use ssl3_EncodeUintX (though it would have to take PRUint64)

Done.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3gthr.c#newcode472
lib/ssl/ssl3gthr.c:472: cText.seq_num |= ss->gs.hdr[3 + i];
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> maybe a function here.

Yeah, I'll just use PR_ntohll().  This code is weird anyway.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/tls13con.c#newcode3237
lib/ssl/tls13con.c:3237: ptr = ssl_EncodeUintX(seqNum & 0xffffffffUL, 4, ptr);
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> And a place where a 64-bit version would help

Done.

https://codereview.appspot.com/309080043/diff/1/lib/ssl/tls13con.c#newcode3261
lib/ssl/tls13con.c:3261: mask = IS_DTLS(ss) ? 0xffffffffffffULL : PR_UINT64_MAX;
On 2016/08/18 23:30:57, ekr-rietveld wrote:
> See previous comments about mask

Done.

[ekr-rietveld, 2016-08-19 17:58:06]

LGTM

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/dtlscon.c#newcode1201
lib/ssl/dtlscon.c:1201: dtls_seq_num = cText->seq_num & 0xffffffUL;
On 2016/08/19 03:57:11, mt wrote:
> On 2016/08/18 23:30:56, ekr-rietveld wrote:
> > IMPORTANT: Isn't this too short? You need to mask off the low-order 48, not
32
> > bits.
> 
> Yes, my new tests picked that up.  (This was only 24 bits, a mistake I had
made
> elsewhere.)

Yeah, I didn't count, just noted it seemed too short.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode304
lib/ssl/ssl3con.c:304: SEC_OID_NULL_CIPHER, "NULL", (1ULL<<48) - 1},
On 2016/08/19 03:57:11, mt wrote:
> On 2016/08/18 23:30:56, ekr-rietveld wrote:
> > This seems kind of spurious. How did you choose this one?
> 
> We have the NULL cipher suite, remember?  This limit is the maximum (well,
> allowing for DTLS).  I originally had it at 1<<10, but that broke.  I figure
> there is no real limit for the NULL cipher.

I agree with that, but it's concerning that that limit causes problems given
that in the handshake we send like 10 messages What test failed?

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:241: spec->read_seq_num = (epoch <<
48) | to;
Not that it's a big deal, but you don't need the temporary here. I believe

spec->read_seq_num = ((spec->read_seq_num) & ~(1<<48 - 1)) | to

will work here and below.

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:251:
PORT_Memset(spec->recvdRecords.data, 0, sizeof(spec->recvdRecords.data));
You should use dtls_RecordSetRecvd(). It's already exported.

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc (right):

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:254: static const uint8_t
payload[18] = {6};
Is there a reason for 6 here?

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:267:
server_->ExpectReadWriteError();
Maybe server_->ProcessRecord()?

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1202: dtls_seq_num = cText->seq_num & 0xffffffffffffUL;
Can't you use the SEQ_NUM_MAX macro?

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode322
lib/ssl/ssl3con.c:322: SEC_OID_DES_CBC,     "DES-CBC", 1<<20},
Can you make these a macro?

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode334
lib/ssl/ssl3con.c:334: SEC_OID_SEED_CBC,    "SEED-CBC", 1ULL<<32},
I believe that it would be better to make SEED and Camellia the same as their
AES counterparts if, as I suspect, they have the same block size.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2898: PORT_Assert(cipher_def->max_records <= RECORD_SEQ_MAX);
This cmment needs revision.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4941: /* Helper function to encode an integer into a buffer.
*/
integer -> unsigned integer

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4945: PRUint64 encoded;
PR_STATIC_ASSERT(sizeof encoded == sizeof value)

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:469: PORT_Memcpy(&seq_num, &ss->gs.hdr[3], sizeof(seq_num));
Instead of memcpying here, do:

PRuint64 *seq_num_p = (PRUint64*)&ss->gs.hdr[3]
cText.seq_num = PR_ntohll(*seq_num_p).

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/sslimpl.h#newcode521
lib/ssl/sslimpl.h:521: #define RECORD_SEQ_MAX 0xffffffffffffULL /* 2^48-1 */
Maybe computing this would make sense.

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3258: /* For DTLS, ignore the epoch. */
Revise comment.

[wtc1, 2016-08-19 18:04:16]

Review comments on patch set 2:

Please consider this as a supplemental review.

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4941: /* Helper function to encode an integer into a buffer.
*/
Nit: an integer => an unsigned integer

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4949: encoded = PR_htonll(value);
Note: I know you wrote the code to be independent of the size of
|encoded|. But the use of PR_htonll here already hardcodes the
requirement that |encoded| must be 8 bytes.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:469: PORT_Memcpy(&seq_num, &ss->gs.hdr[3], sizeof(seq_num));

Eric: we should use memcpy. Your suggestion may result in unaligned
memory access, which may raise a SIGBUS error on some processors.

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/sslimpl.h#newcode554
lib/ssl/sslimpl.h:554: PRUint64 write_seq_num;
Not representing the TLS sequence number as a struct is a good idea.
But I recommend using a SSL3SequenceNumber type to express the intent.
This can be either a typedef or a struct:

  typedef PRUint64 SSL3SequenceNumber;

or

  typedef struct {
      PRUint64 val;
  } SSL3SequenceNumber;

We can also express the intent by naming the variables/members
with "seq_num". But I think a type conveys more.

Note: I don't understand the comment "Do not depend upon 64 bit
arithmetic in the underlying machine." It seems to have been there
since the beginning of open-source NSS, so the concern about 64 bit
arithmetic in the underlying machine should be obsolete today.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/sslimpl.h#newcode737
lib/ssl/sslimpl.h:737: PRUint64 max_records;
Even if we use a SSL3SequenceNumber typedef, it seems better to use
PRUint64 here.

[mt, 2016-08-22 01:29:36]

I've uploaded a new version, but this can't land for the moment.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/ssl3con.c#newcode304
lib/ssl/ssl3con.c:304: SEC_OID_NULL_CIPHER, "NULL", (1ULL<<48) - 1},
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> On 2016/08/19 03:57:11, mt wrote:
> > On 2016/08/18 23:30:56, ekr-rietveld wrote:
> > > This seems kind of spurious. How did you choose this one?
> > 
> > We have the NULL cipher suite, remember?  This limit is the maximum (well,
> > allowing for DTLS).  I originally had it at 1<<10, but that broke.  I figure
> > there is no real limit for the NULL cipher.
> 
> I agree with that, but it's concerning that that limit causes problems given
> that in the handshake we send like 10 messages What test failed?

It's not the handshake.  It's the fact that we support
TLS_RSA_WITH_NULL_(SHA(256)?|MD5).  Note: these are on the list for discussion
at our f2f.

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

https://codereview.appspot.com/309080043/diff/1/lib/ssl/sslimpl.h#newcode458
lib/ssl/sslimpl.h:458: typedef PRUint64 SSL3SequenceNumber;
On 2016/08/18 23:30:56, ekr-rietveld wrote:
> Please get superreview from wtc for this change.

Done.

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/libssl_internals.c (right):

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:241: spec->read_seq_num = (epoch <<
48) | to;
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> Not that it's a big deal, but you don't need the temporary here. I believe
> 
> spec->read_seq_num = ((spec->read_seq_num) & ~(1<<48 - 1)) | to
> 
> will work here and below.

The compiler will do that for us, and the documentation value of having a
variable called epoch is non-zero.

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/libssl_internals.c:251:
PORT_Memset(spec->recvdRecords.data, 0, sizeof(spec->recvdRecords.data));
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> You should use dtls_RecordSetRecvd(). It's already exported.

Done.

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc (right):

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:254: static const uint8_t
payload[18] = {6};
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> Is there a reason for 6 here?

Nope.  It was the number on my keyboard that I hit.

https://codereview.appspot.com/309080043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc:267:
server_->ExpectReadWriteError();
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> Maybe server_->ProcessRecord()?

That's not accessible here.

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1202: dtls_seq_num = cText->seq_num & 0xffffffffffffUL;
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> Can't you use the SEQ_NUM_MAX macro?

Done.

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode322
lib/ssl/ssl3con.c:322: SEC_OID_DES_CBC,     "DES-CBC", 1<<20},
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> Can you make these a macro?

Done.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode334
lib/ssl/ssl3con.c:334: SEC_OID_SEED_CBC,    "SEED-CBC", 1ULL<<32},
On 2016/08/19 17:58:06, ekr-rietveld wrote:
> I believe that it would be better to make SEED and Camellia the same as their
> AES counterparts if, as I suspect, they have the same block size.

Done.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2898: PORT_Assert(cipher_def->max_records <= RECORD_SEQ_MAX);
On 2016/08/19 17:58:05, ekr-rietveld wrote:
> This cmment needs revision.

Done.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4941: /* Helper function to encode an integer into a buffer.
*/
On 2016/08/19 18:04:15, wtc1 wrote:
> Nit: an integer => an unsigned integer

Done.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4945: PRUint64 encoded;
On 2016/08/19 17:58:06, ekr-rietveld wrote:
> PR_STATIC_ASSERT(sizeof encoded == sizeof value)

You can't use PR_STATIC_ASSERT in a function, and the types are the same, so I
imagine that that test is not going to fail.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4949: encoded = PR_htonll(value);
On 2016/08/19 18:04:15, wtc1 wrote:
> Note: I know you wrote the code to be independent of the size of
> |encoded|. But the use of PR_htonll here already hardcodes the
> requirement that |encoded| must be 8 bytes.

:)

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3gthr.c
File lib/ssl/ssl3gthr.c (right):

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/ssl3gthr.c#newcod...
lib/ssl/ssl3gthr.c:469: PORT_Memcpy(&seq_num, &ss->gs.hdr[3], sizeof(seq_num));
On 2016/08/19 18:04:15, wtc1 wrote:
> 
> Eric: we should use memcpy. Your suggestion may result in unaligned
> memory access, which may raise a SIGBUS error on some processors.
> 

I almost did as ekr asked, but this was my reasoning.  This is especially
important here given that this is almost certainly aligned poorly.

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/sslimpl.h#newcode521
lib/ssl/sslimpl.h:521: #define RECORD_SEQ_MAX 0xffffffffffffULL /* 2^48-1 */
On 2016/08/19 17:58:06, ekr-rietveld wrote:
> Maybe computing this would make sense.

Done.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/sslimpl.h#newcode554
lib/ssl/sslimpl.h:554: PRUint64 write_seq_num;
On 2016/08/19 18:04:15, wtc1 wrote:
> Not representing the TLS sequence number as a struct is a good idea.
> But I recommend using a SSL3SequenceNumber type to express the intent.
> This can be either a typedef or a struct:
> 
>   typedef PRUint64 SSL3SequenceNumber;
> 
> or
> 
>   typedef struct {
>       PRUint64 val;
>   } SSL3SequenceNumber;
> 
> We can also express the intent by naming the variables/members
> with "seq_num". But I think a type conveys more.
> 
> Note: I don't understand the comment "Do not depend upon 64 bit
> arithmetic in the underlying machine." It seems to have been there
> since the beginning of open-source NSS, so the concern about 64 bit
> arithmetic in the underlying machine should be obsolete today.

Done.

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/sslimpl.h#newcode737
lib/ssl/sslimpl.h:737: PRUint64 max_records;
On 2016/08/19 18:04:15, wtc1 wrote:
> Even if we use a SSL3SequenceNumber typedef, it seems better to use
> PRUint64 here.

Acknowledged.

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

https://codereview.appspot.com/309080043/diff/20001/lib/ssl/tls13con.c#newcod...
lib/ssl/tls13con.c:3258: /* For DTLS, ignore the epoch. */
On 2016/08/19 17:58:06, ekr-rietveld wrote:
> Revise comment.

Done.

[wtc1, 2016-08-22 16:46:16]

Review comments on patch set 3:

Please consider this as a supplemental review. I only read the diffs between
patch sets 2 and 3.

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

https://codereview.appspot.com/309080043/diff/40001/lib/ssl/dtlscon.c#newcode...
lib/ssl/dtlscon.c:1082: sslSequenceNumber offset;
Nit: it looks strange to declare |offset| as sslSequenceNumber. It is true that
in some cases it is not clear whether sslSequenceNumber or a plain PRUint64 is
more appropriate.

If you change the type to PRUint64, please make the same change to the |offset|
in dtls_RecordSetRecvd.