Apply the fix for NSS bug 1021102 from the NSS upstream.

Apply the fix for NSS bug 1021102 from the NSS upstream.

RSA_PrivateKeyCheck should not swap members of the input
RSAPrivateKey.

R=rsleevi@chromium.org
BUG=380925

[Ryan Sleevi, 2014-06-11 19:11:16]

Sorry I didn't catch this upstream. "yay" Splinter.

https://codereview.chromium.org/329663003/diff/1/nss/lib/freebl/rsa.c
File nss/lib/freebl/rsa.c (right):

https://codereview.chromium.org/329663003/diff/1/nss/lib/freebl/rsa.c#newcode...
nss/lib/freebl/rsa.c:1407: mp_exch(&d_p,&d_q);
This now means we're allowing invalid qInvs. I suspect this should really be
rv = SECFailure;
goto cleanup;

Otherwise, we'll allow qInv = p^-1 mod q

[wtc, 2014-06-11 19:35:24]

Ryan, please let me know whether we should require p > q. Thanks.

https://codereview.chromium.org/329663003/diff/1/nss/lib/freebl/rsa.c
File nss/lib/freebl/rsa.c (right):

https://codereview.chromium.org/329663003/diff/1/nss/lib/freebl/rsa.c#newcode930
nss/lib/freebl/rsa.c:930: SECITEM_TO_MPINT(key->exponent2,   &d_q);  /* d_q  = d
mod (q-1) */

Another option is to do the p > q check here and swap p with q and d_p with d_q
if necessary.

https://codereview.chromium.org/329663003/diff/1/nss/lib/freebl/rsa.c#newcode...
nss/lib/freebl/rsa.c:1407: mp_exch(&d_p,&d_q);

I see. We can also just delete this p > q check (lines 1403 - 1408) and rely on
the qInv check on line 1449-1451.

[Ryan Sleevi, 2014-06-11 19:41:13]

This is the discussion that's happening on the bug with Richard.

From both of us reading RFC 3447, we seem to have reached the conclusion that
"first two primes, denoted as p, q" indicate an ordering requirement, which
means that yes, p < q is required.

https://codereview.chromium.org/329663003/diff/1/nss/lib/freebl/rsa.c
File nss/lib/freebl/rsa.c (right):

https://codereview.chromium.org/329663003/diff/1/nss/lib/freebl/rsa.c#newcode...
nss/lib/freebl/rsa.c:1407: mp_exch(&d_p,&d_q);
On 2014/06/11 19:35:24, wtc wrote:
> 
> I see. We can also just delete this p > q check (lines 1403 - 1408) and rely
on
> the qInv check on line 1449-1451.

We could, but it seems like that'd be more computationally expensive for an
error condition we can short-circuit on here.

[eroman, 2014-06-11 21:50:04]

Deferring review to rsleevi

[Ryan Sleevi, 2014-06-12 19:18:29]

Following this mornings conversation, let's remove the swap handling and land
that on the Chrome side. I think it's very likely we'll do the same on the NSS
side, but I wouldn't want to hold that up.

[wtc, 2014-06-12 22:58:53]

Please review patch set 2. Thanks.

[Ryan Sleevi, 2014-06-13 18:24:27]

lgtm

[wtc, 2014-06-13 18:28:05]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-06-13 18:28:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wtc@chromium.org/329663003/20001

[commit-bot: I haz the power, 2014-06-13 18:29:19]

Change committed as 277057