Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12417005:
Split RC4-encrypted records. (Closed)
|
Created:
7 years, 9 months ago by wtc Modified:
7 years, 4 months ago CC:
cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionSplit RC4-encrypted records.
This idea is proposed by Adam Langley.
I repurpose the ss->opt.cbcRandomIV field to also control this.
I will rename the option "enableRecordSplitting".
R=agl@chromium.org,rsleevi@chromium.org
Patch Set 1 #
Total comments: 6
Patch Set 2 : Feature complete #
Total comments: 15
Patch Set 3 : Move rc4EncryptedBytes to cwSpec/pwSpec. #
Total comments: 2
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... File net/third_party/nss/ssl/ssl3con.c (left): https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... net/third_party/nss/ssl/ssl3con.c:2508: } I generalized this code to be able to split the application data into any number of records. https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... net/third_party/nss/ssl/ssl3con.c:2474: (BIASED_RC4_BYTES + MIN_RECORD_SIZE - 1) / MIN_RECORD_SIZE Here I calculate a crude estimate of how many one-byte records we need. This needs to be fine tuned.
https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... net/third_party/nss/ssl/ssl3con.c:2466: if (nIn > 1 && ss->opt.cbcRandomIV && Unlike the CBC case, we only need to split RC4 records at the beginning of the connection, but I think this code will split all writes currently. Also, do we wish to handle the case where an application makes multiple, short writes at the beginning of the connection? (e.g. "GET ", followed by the URL, followed by " HTTP/1.1") In that case, we might want to keep track of how many keystream bytes we have burnt over several writes. Since this is additional bookkeeping in the connection struct, it may not be worthwhile. Lastly, and most obscurely, this is technically a problem after a renegotiation because the cipher gets rekeyed. However, imagining Javascript that's triggering a renegotiation is tough and I feel that worrying about that might be too much unless it's very easy to account for. https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... net/third_party/nss/ssl/ssl3con.c:2474: (BIASED_RC4_BYTES + MIN_RECORD_SIZE - 1) / MIN_RECORD_SIZE On 2013/03/15 03:58:18, wtc wrote: > Here I calculate a crude estimate of how many one-byte > records we need. This needs to be fine tuned. A couple of ideas for tuning: Obviously the MAC may be larger for a given connection. Additionally, we get a free handshake protocol header, verify_data and MAC from the Finished record.
agl: thank you for your comments. Please review patch set 2. I consider patch set 2 "feature complete", although it still could be polished. https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... net/third_party/nss/ssl/ssl3con.c:2466: if (nIn > 1 && ss->opt.cbcRandomIV && On 2013/03/15 12:35:21, agl wrote: > Unlike the CBC case, we only need to split RC4 records at the beginning of the > connection, but I think this code will split all writes currently. This code only splits the first RC4 application data record. I use a bodge to do that, by setting ss->opt.cbcRandomIV to false, on line 2482 below. > Also, do we wish to handle the case where an application makes multiple, short > writes at the beginning of the connection? (e.g. "GET ", followed by the URL, > followed by " HTTP/1.1") I wasn't planning to go out of my way to do that. But I realized it isn't hard to do that, so patch set 2 handles that correctly. > In that case, we might want to keep track of how many keystream bytes we have > burnt over several writes. Since this is additional bookkeeping in the > connection struct, it may not be worthwhile. This is exactly the approach I take in patch set 2. I also handle renegotiations. https://codereview.chromium.org/12417005/diff/1/net/third_party/nss/ssl/ssl3c... net/third_party/nss/ssl/ssl3con.c:2474: (BIASED_RC4_BYTES + MIN_RECORD_SIZE - 1) / MIN_RECORD_SIZE On 2013/03/15 12:35:21, agl wrote: > > A couple of ideas for tuning: > > Obviously the MAC may be larger for a given connection. Additionally, we get a > free handshake protocol header, verify_data and MAC from the Finished record. Done. With Google sites we also get NextProtocol and EncryptedExtensions. With Twitter and Facebook sites we also get NextProtocol (because they do SPDY/NPN). https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2341: wrBuf->len = cipherBytes + headerLen; Here ssl3_CompressMACEncryptRecord sets wrBuf->len to cipherBytes + headerLen before returning SECSuccess. This fact will be used by ssl3_SendRecord below to calculate the RC4 keystream bytes consumed by encrypted handshake records. https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2474: ss->ssl3.rc4EncryptedBytes < SSL3_BIASED_RC4_BYTES) { Reminder to self: add a comment to explain how we are splitting the application data. https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2483: ss->ssl3.rc4EncryptedBytes += 1 + ss->ssl3.cwSpec->mac_size; It is non-ideal that we are incrementing ss->ssl3.rc4EncryptedBytes for application data records here and for handshake records on lines 2552-2556 below. But this avoids repeating the calculation. Also, I am too lazy to come up with a branch-free way to calculate numRecords. https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2555: wrBuf->len - SSL3_RECORD_HEADER_LENGTH; I could add an output argument to ssl3_CompressMACEncryptRecord to return the ciphertext length. Here I just subtract SSL3_RECORD_HEADER_LENGTH from wrBuf->len. I guess the TLS record header length did not change in TLS 1.2? https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:4221: ss->ssl3.rc4EncryptedBytes = 0; I found the places where we need to reset ss->ssl3.rc4EncryptedBytes to 0 by searching for "PORT_Memset(&ss->xtnData" and the "We might be starting a session renegotiation ..." comment. I believe we only need to reset ss->ssl3.rc4EncryptedBytes to 0 in ssl3_SendClientHello and ssl3_HandleClientHello. But to be safe I reset it whenever we clear ss->xtnData.
LGTM. Although, as discussed, of unclear utility at this point :( https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2470: } This could be an else if to make it clear that the cases are mutually exclusive. https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2555: wrBuf->len - SSL3_RECORD_HEADER_LENGTH; > I guess > the TLS record header length did not change in TLS 1.2? It doesn't. (I don't think it ever could if TLS is to be able to negotiate a common version with previous implementations.) https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:4221: ss->ssl3.rc4EncryptedBytes = 0; On 2013/03/16 01:15:38, wtc wrote: > I believe we only need to reset > ss->ssl3.rc4EncryptedBytes to 0 in ssl3_SendClientHello > and ssl3_HandleClientHello. But to be safe I reset it > whenever we clear ss->xtnData. I think this would be taken care of naturally if the encrypted byte count was part of cwSpec/crSpec. However, I don't think that it's a big difference.
https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2500: rv = sslBuffer_Grow(wrBuf, spaceNeeded); so sslBuffer_Grow caps the growth at MAX_FRAGMENT_LENGTH + 2048 , or 18K. Have you checked with an SSL write of 12-16K? At 256 (258? > 258?) bytes of bias, with 20 bytes of SHA1 MAC (note: not counting the record size, which also factors in on line 2518), that's almost 5K worth of additional record overhead. With a TLSPlaintext size of 2^14, aren't we going to blow past the buffer here, and fail to grow?
https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2500: rv = sslBuffer_Grow(wrBuf, spaceNeeded); On 2013/03/19 17:24:28, Ryan Sleevi wrote: > At 256 (258? > 258?) bytes of > bias, with 20 bytes of SHA1 MAC (note: not counting the record size, which also > factors in on line 2518), that's almost 5K worth of additional record overhead. The MACs are also encrypted so each record is burning 21 bytes of keystream. There should only be ~256 bytes of extra overhead from the splitting.
LGTM, but are we sure this will be sufficient? https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2500: rv = sslBuffer_Grow(wrBuf, spaceNeeded); On 2013/03/19 17:26:44, agl wrote: > On 2013/03/19 17:24:28, Ryan Sleevi wrote: > > At 256 (258? > 258?) bytes of > > bias, with 20 bytes of SHA1 MAC (note: not counting the record size, which > also > > factors in on line 2518), that's almost 5K worth of additional record > overhead. > > The MACs are also encrypted so each record is burning 21 bytes of keystream. > There should only be ~256 bytes of extra overhead from the splitting. Doh! Right, forgot that important detail.
Thank you for the review comments. I won't check this in until we know it is useful. I will create an upstream NSS patch for the cleanup changes in this CL. https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2683: toSend = PR_MIN(len - totalSent, MAX_FRAGMENT_LENGTH); This PR_MIN call caps the |nIn| argument for ssl3_SendRecord at MAX_FRAGMENT_LENGTH. This should keep us below the buffer size limit of MAX_FRAGMENT_LENGTH + 2048 in sslBuffer_Grow. https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:4221: ss->ssl3.rc4EncryptedBytes = 0; On 2013/03/19 16:41:00, agl wrote: > > I think this would be taken care of naturally if the encrypted byte count was > part of cwSpec/crSpec. That seems like a good idea because rc4EncryptedBytes is closely related to the write_seq_num field in cwSpec. This will also allow me to increment rc4EncryptedBytes in ssl3_CompressMACEncryptRecord. Originally I wanted to do that, but ssl3_CompressMACEncryptRecord only has access to 'cwSpec' but not to 'ss', so I can't do that.
https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2470: } On 2013/03/19 16:41:00, agl wrote: > This could be an else if to make it clear that the cases are mutually exclusive. Done. https://codereview.chromium.org/12417005/diff/4001/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:4221: ss->ssl3.rc4EncryptedBytes = 0; On 2013/03/19 16:41:00, agl wrote: > > I think this would be taken care of naturally if the encrypted byte count was > part of cwSpec/crSpec. However, I don't think that it's a big difference. Done.
LGTM. My opinion on the benefit of landing this is a weak no, but I won't object if you feel that it's the right thing to do. https://codereview.chromium.org/12417005/diff/6003/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/6003/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2493: ss->ssl3.cwSpec->rc4EncryptedBytes += I doesn't really matter, but these bytes will be accounted for twice in rc4EncryptedBytes, no? Once here, and then again in ssl3_CompressMACEncryptRecord.
Thanks for the review. I'm not planning to check this in. I am just making it available as one option we can consider. https://codereview.chromium.org/12417005/diff/6003/net/third_party/nss/ssl/ss... File net/third_party/nss/ssl/ssl3con.c (right): https://codereview.chromium.org/12417005/diff/6003/net/third_party/nss/ssl/ss... net/third_party/nss/ssl/ssl3con.c:2493: ss->ssl3.cwSpec->rc4EncryptedBytes += On 2013/03/22 13:44:46, agl wrote: > I doesn't really matter, but these bytes will be accounted for twice in > rc4EncryptedBytes, no? Once here, and then again in > ssl3_CompressMACEncryptRecord. No. In ssl3_CompressMACEncryptRecord, there is a type == content_handshake check so it will only apply to handshake records. I am unhappy about this aspect of the CL because application data records and handshake records are counted in different places. I will try to fix this.
Any objections to closing this review, considering we don't plan to land it?
On 2013/07/25 22:12:39, Ryan Sleevi wrote: > Any objections to closing this review, considering we don't plan to land it? Ping on closing - so that it's out of my queue? :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
