[webcrypto] Fix a bug with JWK private key import.

[webcrypto] Fix a bug with JWK private key import.

The bug would return the oldest live private key imported via JWK rather than the expected one.

BUG=378315, 245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275870

[eroman, 2014-05-29 03:35:32]

Sending the test out for review first, since heading out for the night and
actual fix isn't ready yet.

I likely won't commit this in isolation, but will fold it into the bugfix.

[eroman, 2014-05-30 03:28:29]

PTAL, ready for review.

[eroman, 2014-06-02 18:59:05]

ping

[eroman, 2014-06-02 18:59:12]

ping

[Ryan Sleevi, 2014-06-02 22:57:04]

I'm still not convinced by this approach.

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1735: //      retrieve a private
key of there choice.
1) Please update this comment, as of NSS 3.16.2 it does.
2) there -> their
3) It's entirely possible to check this, so your problem as stated does not
argue the necessity of your approach.

You could just as well do checking yourself by retrieving (PRIME_1, PRIME_2,
EXPONENT_1, EXPONENT_2, CKA_COEFF) and seeing that they all equal the supplied
(input) values if they were present. Likewise, even if they're absent, you can
see if the private exponent and public exponent match what was supplied.

Note that we can detect this programatically on *all* versions of NSS.

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1740: //      values for its
public data.
Please add a note that, as explained, this is not an issue in practice.

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1745: //      the wrong one could
lead to problems.
I still fail to see why (3) is an issue at all. The NSS flags should be
irrelevant for our operation here. If they aren't, it's a bug in our
implementation.

[eroman, 2014-06-02 23:40:18]

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1735: //      retrieve a private
key of there choice.
On 2014/06/02 22:57:05, Ryan Sleevi wrote:
> 1) Please update this comment, as of NSS 3.16.2 it does.
> 2) there -> their

Done.

> 3) It's entirely possible to check this, so your problem as stated does not
> argue the necessity of your approach.
> 
> You could just as well do checking yourself by retrieving (PRIME_1, PRIME_2,
> EXPONENT_1, EXPONENT_2, CKA_COEFF) and seeing that they all equal the supplied
> (input) values if they were present. Likewise, even if they're absent, you can
> see if the private exponent and public exponent match what was supplied.
> 
> Note that we can detect this programatically on *all* versions of NSS.

Checking the parameters (without validating their meaning) has a different set
of problems:

 - Person A could import a bogus key via JWK, with modulus X. We verified that
the parameters of the created key are what was passed in.

 - Person B tries to import a valid key with modulus X, and gets back the one
created by person A. They are prevented from being able to import their key.

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1740: //      values for its
public data.
On 2014/06/02 22:57:05, Ryan Sleevi wrote:
> Please add a note that, as explained, this is not an issue in practice.

Added a comment.

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1745: //      the wrong one could
lead to problems.
On 2014/06/02 22:57:05, Ryan Sleevi wrote:
> I still fail to see why (3) is an issue at all. The NSS flags should be
> irrelevant for our operation here. If they aren't, it's a bug in our
> implementation.

See https://codereview.chromium.org/213423007 and
https://codereview.chromium.org/269313004 for examples of where this was an
issue.

[Ryan Sleevi, 2014-06-02 23:55:57]

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1735: //      retrieve a private
key of there choice.
On 2014/06/02 23:40:18, eroman wrote:
> On 2014/06/02 22:57:05, Ryan Sleevi wrote:
> > 1) Please update this comment, as of NSS 3.16.2 it does.
> > 2) there -> their
> 
> Done.
> 
> > 3) It's entirely possible to check this, so your problem as stated does not
> > argue the necessity of your approach.
> > 
> > You could just as well do checking yourself by retrieving (PRIME_1, PRIME_2,
> > EXPONENT_1, EXPONENT_2, CKA_COEFF) and seeing that they all equal the
supplied
> > (input) values if they were present. Likewise, even if they're absent, you
can
> > see if the private exponent and public exponent match what was supplied.
> > 
> > Note that we can detect this programatically on *all* versions of NSS.
> 
> Checking the parameters (without validating their meaning) has a different set
> of problems:
> 
>  - Person A could import a bogus key via JWK, with modulus X. We verified that
> the parameters of the created key are what was passed in.

This won't succeed on Windows/Mac/ChromeOS. And we can ensure it won't succeed
on Linux (mod NSS 3.16.2).


> 
>  - Person B tries to import a valid key with modulus X, and gets back the one
> created by person A. They are prevented from being able to import their key.

This also is with respect to multiple origins sharing the same renderer process,
which is something we try to avoid as a matter of practice (on Desktop/CrOS).
This would present as a DoS bug, and the temporary risk seems low.

https://codereview.chromium.org/308523003/diff/60001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1745: //      the wrong one could
lead to problems.
On 2014/06/02 23:40:18, eroman wrote:
> On 2014/06/02 22:57:05, Ryan Sleevi wrote:
> > I still fail to see why (3) is an issue at all. The NSS flags should be
> > irrelevant for our operation here. If they aren't, it's a bug in our
> > implementation.
> 
> See https://codereview.chromium.org/213423007 and
> https://codereview.chromium.org/269313004 for examples of where this was an
> issue.

I'm still having trouble with this.

All WebCrypto keys within the renderer should have the same flags. Both of those
bugs seem to be issues where we weren't setting the *right* NSS flags, but your
description is where flags(key-a) != flags (key-b)

https://codereview.chromium.org/308523003/diff/80001/content/child/webcrypto/...
File content/child/webcrypto/platform_crypto_nss.cc (right):

https://codereview.chromium.org/308523003/diff/80001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1742: //      Ryan says this
isn't possible in practice, because matching values for
We don't do this in comments :)


But I'm glad Ryan Hamilton approves ;)


// This is not an issue in practice, since it requires generating a valid RSA
public key that is also a valid DH/EC key.

[eroman, 2014-06-03 00:02:20]

I an not sure I follow your comments. Are you unhappy with the approach, or the
justification of it?

Is your proposal to use the ID based on public key? I can do that if it means
making progress on this issue, however seems like more work with more issues
which "shouldn't happen" than using a random ID, whose probability of failure is
much easier to understand and bound.

[Ryan Sleevi, 2014-06-03 00:05:53]

On 2014/06/03 00:02:20, eroman wrote:
> I an not sure I follow your comments. Are you unhappy with the approach, or
the
> justification of it?
> 
> Is your proposal to use the ID based on public key? I can do that if it means
> making progress on this issue, however seems like more work with more issues
> which "shouldn't happen" than using a random ID, whose probability of failure
is
> much easier to understand and bound.

I'm trying to compare/contrast with using the 'standard' NSS approach of
PK11_MakeIDFromPubKey, combined with checking that NSS did not change any of the
parameters under you the !(n, e, d) case (eg: full CRT). In the (n, e, d) case,
it's supposed to implicitly validate (n, e, d) as otherwise it won't be able to
derive p/q if n/e/d aren't consistent.

[eroman, 2014-06-03 21:45:48]

PTAL, I switched to PK11_MakeIDFromPubKey() as requested.

One of the accompanying tests will have to be enabled once the NSS fix rolls in.

[Ryan Sleevi, 2014-06-05 23:20:24]

LGTM, but please wait until https://codereview.chromium.org/317803004/ lands so
that you can remove the tests that check it succeeds.

[Ryan Sleevi, 2014-06-05 23:21:25]

https://codereview.chromium.org/308523003/diff/100001/content/child/webcrypto...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/308523003/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:2261: // If NSS did
validations of the input, this would fail. Failing would be the
I'm not a fan of this test, even DISABLED_, since OpenSSL will likely behave
differently.

Rather, changing this test to make sure the second import *doesn't* succeed
seems much better.

[eroman, 2014-06-05 23:43:35]

https://codereview.chromium.org/308523003/diff/100001/content/child/webcrypto...
File content/child/webcrypto/shared_crypto_unittest.cc (right):

https://codereview.chromium.org/308523003/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:2261: // If NSS did
validations of the input, this would fail. Failing would be the
On 2014/06/05 23:21:25, Ryan Sleevi wrote:
> I'm not a fan of this test, even DISABLED_, since OpenSSL will likely behave
> differently.
> 
> Rather, changing this test to make sure the second import *doesn't* succeed
> seems much better.

Once the NSS side rolls in, presumably I can change the import to assert a
failure (for builds using bundled NSS, and skip it on the others).

[eroman, 2014-06-06 19:35:59]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-06-06 19:38:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/308523003/180001

[commit-bot: I haz the power, 2014-06-07 01:19:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-06-07 03:13:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-07 03:13:59]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[eroman, 2014-06-09 17:26:26]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-06-09 17:27:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eroman@chromium.org/308523003/180001

[commit-bot: I haz the power, 2014-06-09 22:53:02]

Change committed as 275870