[webcryto] Validate key usages during key creation.

[webcryto] Validate key usages during key creation.

 (1) Key creation (whether by importKey(), unwrapKey(), generateKey() now fails if the requested key usages are not applicable (for instance asking for 'sign' on an AES-CBC key).
 (2) When generating a key pair, the public/private key get the intersection of supported usages and requested ones.
 (3) The exceptions thrown during the import phase of unwrapKey() are now surfaced to the caller (bug 372944)

BUG=372040, 372944, 245025
R=rsleevi@chromium.org

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

[Ryan Sleevi, 2014-05-15 06:29:40]

Blergh. This CL conflicts with my CL is just about every place imaginable.

Thoughts below from a quick scan.

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:481: // http://crubg.com/372040
crbug.com

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:607: case
blink::WebCryptoKeyTypePublic:
Structurally, I'm wondering whether it makes more sense to divide this first by
key type, then by algorithm. Thoughts?

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:654: *private_key_usages =
combined_usage_mask & all_private_key_usages;
Seems like it should be invalid to generate a keypair where the private key has
no usages.

Thoughts? Spec bug?

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:810: format, key_data, algorithm,
extractable, usage_mask, key);
Shouldn't we be able to fast-fail if |algorithm| and |usage_mask| are invalid?
The only time we might reduce the set of usages is for JWK import, and we can do
that check when considering the intersection.

Ditto unwrap

[eroman, 2014-05-16 00:04:37]

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:607: case
blink::WebCryptoKeyTypePublic:
On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> Structurally, I'm wondering whether it makes more sense to divide this first
by
> key type, then by algorithm. Thoughts?

I can paint the bikeshed either colour, don't particularly care.

My arguments for doing algorithm first are:

  * Fewer default cases in switches. When chromium/blink are eventually merged I
want to get rid of all the "default" switches on "switch (algorithm)". Breaking
down on keytype first means there will be 3 incomplete switches.
  
  * Aesthetically I prefer grouping algorithms together.

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:654: *private_key_usages =
combined_usage_mask & all_private_key_usages;
On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> Seems like it should be invalid to generate a keypair where the private key
has
> no usages.
> 
> Thoughts? Spec bug?

My interpretation of the spec is that keys with no usages is valid, and hence it
is allowed by Chromium.

I personally don't see a reason to disallow it. I am not sure how useful a key
without usages is, but it can still be used for exporting the key material so
not completely useless.

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:810: format, key_data, algorithm,
extractable, usage_mask, key);
On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> Shouldn't we be able to fast-fail if |algorithm| and |usage_mask| are invalid?
> The only time we might reduce the set of usages is for JWK import, and we can
do
> that check when considering the intersection.
> 
> Ditto unwrap

I opted to do the key usages test *after* key creation to avoid unnecessary code
complexity, and protect the code from breaking in the future.

Deciding exactly what to fail-fast on is unclear. For instance when unwrapping
JWK for HMAC we could fail fast on key usages (since key type is known), however
when unwrapping JWK for RSA-SSA we cannot (since key type is not known yet).

To illustrate how this can get complicated if you want consistent fail-fast here
are some of the cases that need care:

-------------
Implementation cases
-------------

Import:

 * In ImportKey(), for usage of raw,spki,pkcs8 do usage check upfront.
 * If usage == JWK, do the usage check as part of ImportKeyJwk(), once the key
type has been determined.

Unwrapping:
 * If unwrapping a raw/spki/pkcs8 format, do the usage check upfront. There are
landmines here: unwrapping of raw keys is currently special cased and doesn't go
through ImportKey() at all. So doing the pre-check is critical. However for the
other formats ImportKey() will subsequently be called and do the check as well
(used for JWK next)

 * If unwrapping a jwk format you _could_ do a fail-fast for symmetric
algorithms since the key type is already known. Otherwise the check can be left
to ImportKey() as part of importing JWK.

Hooking multiple places with key checks is just asking for things to break.
Ideally I would only have 2 places to hook: ImportKey() and GenerateKey(), but
UnwrapKey() doesn't consistently call ImportKey() so I added it there too.

-------------
Performance
-------------

The operation we are optimizing with a fail-fast is mainly developer error. With
the exception of JWK formats (import/unwrap) giving the wrong key usages is
quite simply an invalid usage, and whether we fail it fast or slow the end
result is likely still failure.

For these reasons I think code simplicity are the more important metric. WDYT?

[Ryan Sleevi, 2014-05-19 23:06:29]

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:607: case
blink::WebCryptoKeyTypePublic:
On 2014/05/16 00:04:37, eroman wrote:
> On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> > Structurally, I'm wondering whether it makes more sense to divide this first
> by
> > key type, then by algorithm. Thoughts?
> 
> I can paint the bikeshed either colour, don't particularly care.
> 
> My arguments for doing algorithm first are:
> 
>   * Fewer default cases in switches. When chromium/blink are eventually merged
I
> want to get rid of all the "default" switches on "switch (algorithm)".
Breaking
> down on keytype first means there will be 3 incomplete switches.
>   
>   * Aesthetically I prefer grouping algorithms together.

Part of the comment was motivated by things like line 601-602 / 613-614, which
by default return 0 if the key_type is not consistent with algorithm - eg: if
'secret' is supplied as type.

However, for the symmetric keys, you don't have that equivalent behaviour - if
the type is public/private (which is invalid), it still returns 'valid' key
usages.

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:654: *private_key_usages =
combined_usage_mask & all_private_key_usages;
On 2014/05/16 00:04:37, eroman wrote:
> On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> > Seems like it should be invalid to generate a keypair where the private key
> has
> > no usages.
> > 
> > Thoughts? Spec bug?
> 
> My interpretation of the spec is that keys with no usages is valid, and hence
it
> is allowed by Chromium.
> 
> I personally don't see a reason to disallow it. I am not sure how useful a key
> without usages is, but it can still be used for exporting the key material so
> not completely useless.

Fair point, though I wonder if we shouldn't correct the spec, since it's
possible you attempt to generate for usages that are not supported, end up
hitting the empty array - but still perform the expensive operation of
generating the key.

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:810: format, key_data, algorithm,
extractable, usage_mask, key);
On 2014/05/16 00:04:37, eroman wrote:
> On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> > Shouldn't we be able to fast-fail if |algorithm| and |usage_mask| are
invalid?
> > The only time we might reduce the set of usages is for JWK import, and we
can
> do
> > that check when considering the intersection.
> > 
> > Ditto unwrap
> 
> I opted to do the key usages test *after* key creation to avoid unnecessary
code
> complexity, and protect the code from breaking in the future.
> 
> Deciding exactly what to fail-fast on is unclear. For instance when unwrapping
> JWK for HMAC we could fail fast on key usages (since key type is known),
however
> when unwrapping JWK for RSA-SSA we cannot (since key type is not known yet).
> 
> To illustrate how this can get complicated if you want consistent fail-fast
here
> are some of the cases that need care:
> 
> -------------
> Implementation cases
> -------------
> 
> Import:
> 
>  * In ImportKey(), for usage of raw,spki,pkcs8 do usage check upfront.
>  * If usage == JWK, do the usage check as part of ImportKeyJwk(), once the key
> type has been determined.
> 
> Unwrapping:
>  * If unwrapping a raw/spki/pkcs8 format, do the usage check upfront. There
are
> landmines here: unwrapping of raw keys is currently special cased and doesn't
go
> through ImportKey() at all. So doing the pre-check is critical. However for
the
> other formats ImportKey() will subsequently be called and do the check as well
> (used for JWK next)
> 
>  * If unwrapping a jwk format you _could_ do a fail-fast for symmetric
> algorithms since the key type is already known. Otherwise the check can be
left
> to ImportKey() as part of importing JWK.
> 
> Hooking multiple places with key checks is just asking for things to break.
> Ideally I would only have 2 places to hook: ImportKey() and GenerateKey(), but
> UnwrapKey() doesn't consistently call ImportKey() so I added it there too.
> 
> -------------
> Performance
> -------------
> 
> The operation we are optimizing with a fail-fast is mainly developer error.
With
> the exception of JWK formats (import/unwrap) giving the wrong key usages is
> quite simply an invalid usage, and whether we fail it fast or slow the end
> result is likely still failure.
> 
> For these reasons I think code simplicity are the more important metric. WDYT?

For the similar reasons as the generate case above - where you intend to use a
feature that is not yet in the implementation - it seems vastly more beneficial
to fail as quickly as possible.

I'm still inclined to suggest a double check, and may try to put it in the spec
as a pre-condition, as part of the overall feature-detection/fast-failure.

[eroman, 2014-05-20 00:32:19]

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:654: *private_key_usages =
combined_usage_mask & all_private_key_usages;
On 2014/05/19 23:06:29, Ryan Sleevi wrote:
> On 2014/05/16 00:04:37, eroman wrote:
> > On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> > > Seems like it should be invalid to generate a keypair where the private
key
> > has
> > > no usages.
> > > 
> > > Thoughts? Spec bug?
> > 
> > My interpretation of the spec is that keys with no usages is valid, and
hence
> it
> > is allowed by Chromium.
> > 
> > I personally don't see a reason to disallow it. I am not sure how useful a
key
> > without usages is, but it can still be used for exporting the key material
so
> > not completely useless.
> 
> Fair point, though I wonder if we shouldn't correct the spec, since it's
> possible you attempt to generate for usages that are not supported, end up
> hitting the empty array - but still perform the expensive operation of
> generating the key.

Not sure I follow.

GenerateKey() does a pre-check -- if any of the requested usages are not
applicable to either public keys or private keys, it fails.

Only then does it take the intersection. For instance this can be seen in the
RSA-ES part of the spec which starts with:

If usages contains an entry which is not "encrypt", "decrypt", "wrapKey" or
"unwrapKey", then return an error named DataError.



So you can't simply end up with a key with empty usages because it contained
something unexpected. If the requested usages contained something unexpected and
totally bogus you end up with a DataError.

Now as far as whether empty usages should be allowed at all, I raised that as
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25820, however I see this as an
orthogonal issue.

[eroman, 2014-05-20 01:27:57]

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
File content/child/webcrypto/shared_crypto.cc (right):

https://codereview.chromium.org/282133002/diff/30001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:810: format, key_data, algorithm,
extractable, usage_mask, key);
On 2014/05/19 23:06:29, Ryan Sleevi wrote:
> On 2014/05/16 00:04:37, eroman wrote:
> > On 2014/05/15 06:29:40, Ryan Sleevi wrote:
> > > Shouldn't we be able to fast-fail if |algorithm| and |usage_mask| are
> invalid?
> > > The only time we might reduce the set of usages is for JWK import, and we
> can
> > do
> > > that check when considering the intersection.
> > > 
> > > Ditto unwrap
> > 
> > I opted to do the key usages test *after* key creation to avoid unnecessary
> code
> > complexity, and protect the code from breaking in the future.
> > 
> > Deciding exactly what to fail-fast on is unclear. For instance when
unwrapping
> > JWK for HMAC we could fail fast on key usages (since key type is known),
> however
> > when unwrapping JWK for RSA-SSA we cannot (since key type is not known yet).
> > 
> > To illustrate how this can get complicated if you want consistent fail-fast
> here
> > are some of the cases that need care:
> > 
> > -------------
> > Implementation cases
> > -------------
> > 
> > Import:
> > 
> >  * In ImportKey(), for usage of raw,spki,pkcs8 do usage check upfront.
> >  * If usage == JWK, do the usage check as part of ImportKeyJwk(), once the
key
> > type has been determined.
> > 
> > Unwrapping:
> >  * If unwrapping a raw/spki/pkcs8 format, do the usage check upfront. There
> are
> > landmines here: unwrapping of raw keys is currently special cased and
doesn't
> go
> > through ImportKey() at all. So doing the pre-check is critical. However for
> the
> > other formats ImportKey() will subsequently be called and do the check as
well
> > (used for JWK next)
> > 
> >  * If unwrapping a jwk format you _could_ do a fail-fast for symmetric
> > algorithms since the key type is already known. Otherwise the check can be
> left
> > to ImportKey() as part of importing JWK.
> > 
> > Hooking multiple places with key checks is just asking for things to break.
> > Ideally I would only have 2 places to hook: ImportKey() and GenerateKey(),
but
> > UnwrapKey() doesn't consistently call ImportKey() so I added it there too.
> > 
> > -------------
> > Performance
> > -------------
> > 
> > The operation we are optimizing with a fail-fast is mainly developer error.
> With
> > the exception of JWK formats (import/unwrap) giving the wrong key usages is
> > quite simply an invalid usage, and whether we fail it fast or slow the end
> > result is likely still failure.
> > 
> > For these reasons I think code simplicity are the more important metric.
WDYT?
> 
> For the similar reasons as the generate case above - where you intend to use a
> feature that is not yet in the implementation - it seems vastly more
beneficial
> to fail as quickly as possible.
> 
> I'm still inclined to suggest a double check, and may try to put it in the
spec
> as a pre-condition, as part of the overall feature-detection/fast-failure.

FYI: I am updating the CL to do fail-fast wherever possible, will ping when
ready for re-review.

[eroman, 2014-05-20 22:37:10]

PTAL, I have switched to fail-fast, I agree that approach is better.

I also ended up rebasing on the (hopefully soon to be landed) JWK import/export
changes to avoid some conflicts.

[eroman, 2014-05-22 21:38:15]

ping

[Ryan Sleevi, 2014-05-22 21:52:12]

lgtm

[eroman, 2014-05-23 19:23:00]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-05-23 19:24:28]

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

[eroman, 2014-05-23 23:13:54]

Committed patchset #6 manually as r272630 (presubmit successful).