net: implement CBC processing in constant-time in libssl.

net: implement CBC processing in constant-time in libssl.

This change contains the libssl changes needed to process CBC records in
constant time. This has no effect unless the underlying NSS library has also
been updated.

BUG=172658
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180864

[wtc, 2013-02-04 20:03:25]

Patch set 1 LGTM. It is best to wait for rsleevi's review
because I didn't review the upstream NSS patch.

I suggest two small coding style/comment changes. You can
make them in your NSS upstream patch directly. Thanks.

https://codereview.chromium.org/12193010/diff/2002/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/12193010/diff/2002/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:2042: #endif

I reviewed this block of code and confirmed it is correct.

https://codereview.chromium.org/12193010/diff/2002/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:10172: * them with something else on odd CPUs.
*/

I researched this signed right-shift issue when working on
your P-256 curve. I found that the behavior is up to the
compiler (CPU support merely makes it easier to implement
arithmetic shift) and in practice all compilers today do an
arithmetic shift.

So in this comment "CPUs" should be replaced by "compilers".

I am sure there is a lot of code that simply assumes an
arithmetic shift.
This also implies a new compiler will have to do an
arithmetic shift in order to compile existing code correctly.

https://codereview.chromium.org/12193010/diff/2002/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:10178: static unsigned
SECStatusToMask(SECStatus rv) {

In NSS style, the opening curly brace for a function should be
on its own line. This is best done in the NSS upstream.

[agl, 2013-02-04 20:54:59]

https://codereview.chromium.org/12193010/diff/2002/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/12193010/diff/2002/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:10172: * them with something else on odd CPUs.
*/
On 2013/02/04 20:03:25, wtc wrote:
> So in this comment "CPUs" should be replaced by "compilers".

Done.

https://codereview.chromium.org/12193010/diff/2002/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:10178: static unsigned
SECStatusToMask(SECStatus rv) {
On 2013/02/04 20:03:25, wtc wrote:
> 
> In NSS style, the opening curly brace for a function should be
> on its own line. This is best done in the NSS upstream.

Done. Have uploaded new upstream patches and updated here.

[Ryan Sleevi, 2013-02-04 23:01:58]

I mentioned to wtc, this LGTM, on the basis of the upstream review. I took a
cursory glance (where "cursory" == 30 minutes) to map everything back in, and
there were no latent surprises.

We'll also end up hitting the fallback path on Linux, until distros upgrade, so
we could consider bumping the NSS min revision.

ChromeOS changes are pending. I broke the tree last week due to NSS-in-sandbox
failures (used by renderer & media) and have been working with jorgelo@ to get
them straightened out.

[commit-bot: I haz the power, 2013-02-04 23:29:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/12193010/17003

[wtc, 2013-02-05 16:45:48]

Patch set 3 LGTM. I suggest one mandatory fix for DTLS, and
some (optional) PKCS #11 naming changes. Thanks.

I will be out of the office the rest of your work day today.
You don't need to wait for me to review your new patch set.

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2040: CK_ULONG ulHeaderLength;    /* in */

I'm going to suggest three naming changes here to make them
more consistent with the naming convention of PKCS #11,
in the hope that these will be eventually adopted by
PKCS #11.

1. CKM_NSS_SSLV3 => CKM_NSS_SSL3

This is because the existing SSLv3 PKCS #11 mechanisms
are all named CKM_SSL3_xxx:

http://mxr.mozilla.org/security/search?string=CKM_.*SSL.*&regexp=1&case=1&fin...

2. CK_NSS_MACConstantTimeParams => CK_NSS_MAC_CONSTANT_TIME_PARAMS

This is the naming convention of mechanism parameter structs
in PKCS #11.

http://mxr.mozilla.org/security/search?string=CK_.*PARAMS&regexp=1&case=1&fin...

3. ulBodyTotalLength => ulBodyTotalLen
   ulHeaderLength => ulHeaderLen

This is because the CK_ULONG length fields are all named
ulFooLen, except two:

http://mxr.mozilla.org/security/search?string=ul.*Len%3B&regexp=1&case=1&find...

vs.

http://mxr.mozilla.org/security/search?string=ul.*Length%3B&regexp=1&case=1&f...

If you agree with these coding style changes, please go
ahead and make the changes in this CL. Thanks.

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2106: header[10] = version;

IMPORTANT: please port my isDTLS change from
https://bugzilla.mozilla.org/attachment.cgi?oldid=710008&action=interdiff&new...

It would be nice to ask juberti or ronghuawu to test
this patch with WebRTC, if the WebRTC code is already
enabled in Chromium.

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2126: /* Older versions of NSS may not support
constant-time MAC. */

I think this code is also appropriate for NSS upstream, because
sometimes we want to test with an older version of the
NSS softoken.

It seems that we should call PK11_CreateContextBySymKey
earlier in this function so that if we do need to fall
back, we can skip the code to set up |params|.

In the upstream NSS patch, you are using the new
PK11_SignWithSymKey function. Why don't you use that
function here?

[agl, 2013-02-05 21:44:18]

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2040: CK_ULONG ulHeaderLength;    /* in */
On 2013/02/05 16:45:48, wtc wrote:
> I'm going to suggest three naming changes

I have applied all your suggested naming changes.

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2106: header[10] = version;
On 2013/02/05 16:45:48, wtc wrote:
> IMPORTANT: please port my isDTLS change

Done.

> It would be nice to ask juberti or ronghuawu to test
> this patch with WebRTC, if the WebRTC code is already
> enabled in Chromium.

I've emailed them to ask whether there are tests in Chrome already.

https://codereview.chromium.org/12193010/diff/17003/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3con.c:2126: /* Older versions of NSS may not support
constant-time MAC. */
On 2013/02/05 16:45:48, wtc wrote:
> It seems that we should call PK11_CreateContextBySymKey
> earlier in this function so that if we do need to fall
> back, we can skip the code to set up |params|.

That's true, although it's only local memory access and so probably on the order
of a few dozen cycles. I haven't made this change here yet, although you could
upstream if you wish and we would eventually sync up.

> In the upstream NSS patch, you are using the new
> PK11_SignWithSymKey function. Why don't you use that
> function here?

PK11_SignWithSymKey is a new function, therefore we would have to link against
an NSS that provided it. By expanding PK11_SignWithSymKey here, we can continue
to link against older versions of NSS.

[commit-bot: I haz the power, 2013-02-05 21:56:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/12193010/24001

[commit-bot: I haz the power, 2013-02-06 02:14:15]

Change committed as 180864