[webcrypto] Add JWK symmetric key AES-KW unwrap for NSS.

[webcrypto] Add JWK symmetric key AES-KW unwrap for NSS.

Note: AES-KW wrap of formats other than 'raw' is not 
currently supported because of strict input data size
modulo requirements of RFC3394.

BUG=245025
TEST=content_unittests --gtest_filter="SharedCryptoTest*"

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

[eroman, 2014-03-13 00:17:18]

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

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily viewed as a
symmetric key to be unwrapped (decrypted).
Deferring to @rsleevi. I had the impression from Ryan that there was a mechanism
that could be used directly with decryption. So the way this would work is to do
decryption to a buffer, and then import that as a jwk key.

[padolph, 2014-03-13 01:26:33]

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

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily viewed as a
symmetric key to be unwrapped (decrypted).
On 2014/03/13 00:17:18, eroman wrote:
> Deferring to @rsleevi. I had the impression from Ryan that there was a
mechanism
> that could be used directly with decryption. So the way this would work is to
do
> decryption to a buffer, and then import that as a jwk key.

I tried that method earlier but could not make it work. NSS returns an error
from PK11_CipherOp().

The actual failure is in sftk_GetContext() in pkcs11c.c:344:
    if((context==NULL)||(context->type!=type)||(needMulti&&!(context->multi))){
        sftk_FreeSession(session);
	return CKR_OPERATION_NOT_INITIALIZED;
In this case |needMulti| is true and |context->multi| = 0.

I'm not sure what I did wrong - I followed the same method as you used for
AES-CBC, just using CKM_NSS_AES_KEY_WRAP where appropriate and adjusting the
length checks. Does the AES-KW cipher need to be enabled somehow, like OpenSSL's
AddCipher()?

After I hit this snag, I searched around to see how other people were doing
AES-KW, and found several examples that used PK11_UnwrapSymKey with generic data
as I am doing now, so I figured that was the 'canonical' way.

Ryan, any thoughts?

[Ryan Sleevi, 2014-03-13 01:44:17]

On 2014/03/13 01:26:33, padolph wrote:
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> File content/child/webcrypto/platform_crypto_nss.cc (right):
> 
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily viewed as
a
> symmetric key to be unwrapped (decrypted).
> On 2014/03/13 00:17:18, eroman wrote:
> > Deferring to @rsleevi. I had the impression from Ryan that there was a
> mechanism
> > that could be used directly with decryption. So the way this would work is
to
> do
> > decryption to a buffer, and then import that as a jwk key.
> 
> I tried that method earlier but could not make it work. NSS returns an error
> from PK11_CipherOp().
> 
> The actual failure is in sftk_GetContext() in pkcs11c.c:344:
>    
if((context==NULL)||(context->type!=type)||(needMulti&&!(context->multi))){
>         sftk_FreeSession(session);
> 	return CKR_OPERATION_NOT_INITIALIZED;
> In this case |needMulti| is true and |context->multi| = 0.
> 
> I'm not sure what I did wrong - I followed the same method as you used for
> AES-CBC, just using CKM_NSS_AES_KEY_WRAP where appropriate and adjusting the
> length checks. Does the AES-KW cipher need to be enabled somehow, like
OpenSSL's
> AddCipher()?
> 
> After I hit this snag, I searched around to see how other people were doing
> AES-KW, and found several examples that used PK11_UnwrapSymKey with generic
data
> as I am doing now, so I figured that was the 'canonical' way.
> 
> Ryan, any thoughts?

Don't use PK11_CipherOp

You need to use PK11_Encrypt / PK11_Decrypt, which was introduced when GCM
support was introduced, and does 'single-shot' (eg: it doesn't use
EncryptUpdate, which is what sets needMulti)

[padolph, 2014-03-14 01:01:09]

On 2014/03/13 01:44:17, Ryan Sleevi wrote:
> On 2014/03/13 01:26:33, padolph wrote:
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > File content/child/webcrypto/platform_crypto_nss.cc (right):
> > 
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily viewed
as
> a
> > symmetric key to be unwrapped (decrypted).
> > On 2014/03/13 00:17:18, eroman wrote:
> > > Deferring to @rsleevi. I had the impression from Ryan that there was a
> > mechanism
> > > that could be used directly with decryption. So the way this would work is
> to
> > do
> > > decryption to a buffer, and then import that as a jwk key.
> > 
> > I tried that method earlier but could not make it work. NSS returns an error
> > from PK11_CipherOp().
> > 
> > The actual failure is in sftk_GetContext() in pkcs11c.c:344:
> >    
> if((context==NULL)||(context->type!=type)||(needMulti&&!(context->multi))){
> >         sftk_FreeSession(session);
> > 	return CKR_OPERATION_NOT_INITIALIZED;
> > In this case |needMulti| is true and |context->multi| = 0.
> > 
> > I'm not sure what I did wrong - I followed the same method as you used for
> > AES-CBC, just using CKM_NSS_AES_KEY_WRAP where appropriate and adjusting the
> > length checks. Does the AES-KW cipher need to be enabled somehow, like
> OpenSSL's
> > AddCipher()?
> > 
> > After I hit this snag, I searched around to see how other people were doing
> > AES-KW, and found several examples that used PK11_UnwrapSymKey with generic
> data
> > as I am doing now, so I figured that was the 'canonical' way.
> > 
> > Ryan, any thoughts?
> 
> Don't use PK11_CipherOp
> 
> You need to use PK11_Encrypt / PK11_Decrypt, which was introduced when GCM
> support was introduced, and does 'single-shot' (eg: it doesn't use
> EncryptUpdate, which is what sets needMulti)

Ok, I confirmed that works but I have a concern.

Right now, we are using the presence / absence of the PK11_Encrypt to detect
AES-GCM support. If we use this function as part of the AES-KW implementation,
we will tie the AES-KW cipher to >=NSS 3.15 just like we had to for AES-GCM. And
so we will need the same runtime checking ugliness around AES-KW. Using the
older method avoids this issue.

[Ryan Sleevi, 2014-03-14 01:33:17]

On 2014/03/14 01:01:09, padolph wrote:
> On 2014/03/13 01:44:17, Ryan Sleevi wrote:
> > On 2014/03/13 01:26:33, padolph wrote:
> > >
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > > File content/child/webcrypto/platform_crypto_nss.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > > content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily viewed
> as
> > a
> > > symmetric key to be unwrapped (decrypted).
> > > On 2014/03/13 00:17:18, eroman wrote:
> > > > Deferring to @rsleevi. I had the impression from Ryan that there was a
> > > mechanism
> > > > that could be used directly with decryption. So the way this would work
is
> > to
> > > do
> > > > decryption to a buffer, and then import that as a jwk key.
> > > 
> > > I tried that method earlier but could not make it work. NSS returns an
error
> > > from PK11_CipherOp().
> > > 
> > > The actual failure is in sftk_GetContext() in pkcs11c.c:344:
> > >    
> > if((context==NULL)||(context->type!=type)||(needMulti&&!(context->multi))){
> > >         sftk_FreeSession(session);
> > > 	return CKR_OPERATION_NOT_INITIALIZED;
> > > In this case |needMulti| is true and |context->multi| = 0.
> > > 
> > > I'm not sure what I did wrong - I followed the same method as you used for
> > > AES-CBC, just using CKM_NSS_AES_KEY_WRAP where appropriate and adjusting
the
> > > length checks. Does the AES-KW cipher need to be enabled somehow, like
> > OpenSSL's
> > > AddCipher()?
> > > 
> > > After I hit this snag, I searched around to see how other people were
doing
> > > AES-KW, and found several examples that used PK11_UnwrapSymKey with
generic
> > data
> > > as I am doing now, so I figured that was the 'canonical' way.
> > > 
> > > Ryan, any thoughts?
> > 
> > Don't use PK11_CipherOp
> > 
> > You need to use PK11_Encrypt / PK11_Decrypt, which was introduced when GCM
> > support was introduced, and does 'single-shot' (eg: it doesn't use
> > EncryptUpdate, which is what sets needMulti)
> 
> Ok, I confirmed that works but I have a concern.
> 
> Right now, we are using the presence / absence of the PK11_Encrypt to detect
> AES-GCM support. If we use this function as part of the AES-KW implementation,
> we will tie the AES-KW cipher to >=NSS 3.15 just like we had to for AES-GCM.
And
> so we will need the same runtime checking ugliness around AES-KW. Using the
> older method avoids this issue.

Yeah, but the older methods are always multi-shot, which is why we introduced
the new method - it was the only (public API) way to get to the single-shot
methods, because AES-GCM only supports single-shot.

Up until then, NSS hadn't needed to expose it, and that AES-KW was implemented
in terms of it was an implementation detail of softoken.

FWIW, it's an NSS "bug" that AES-KW supports C_Encrypt/C_Decrypt at all - the
spec is clear it only supports C_WrapKey/C_UnwrapKey. I suspect we'll move off
NSS before this becomes an issue, but just an example of us relying on undefined
behaviour.

See 6.13 of http://www.cryptsoft.com/pkcs11doc/STANDARD/pkcs-11v2-30m1-d7.pdf .
Only CKM_AES_KEY_WRAP_PAD supports Enc/Dec.

[padolph, 2014-03-14 04:25:41]

On 2014/03/14 01:33:17, Ryan Sleevi wrote:
> On 2014/03/14 01:01:09, padolph wrote:
> > On 2014/03/13 01:44:17, Ryan Sleevi wrote:
> > > On 2014/03/13 01:26:33, padolph wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > > > File content/child/webcrypto/platform_crypto_nss.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > > > content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily
viewed
> > as
> > > a
> > > > symmetric key to be unwrapped (decrypted).
> > > > On 2014/03/13 00:17:18, eroman wrote:
> > > > > Deferring to @rsleevi. I had the impression from Ryan that there was a
> > > > mechanism
> > > > > that could be used directly with decryption. So the way this would
work
> is
> > > to
> > > > do
> > > > > decryption to a buffer, and then import that as a jwk key.
> > > > 
> > > > I tried that method earlier but could not make it work. NSS returns an
> error
> > > > from PK11_CipherOp().
> > > > 
> > > > The actual failure is in sftk_GetContext() in pkcs11c.c:344:
> > > >    
> > >
if((context==NULL)||(context->type!=type)||(needMulti&&!(context->multi))){
> > > >         sftk_FreeSession(session);
> > > > 	return CKR_OPERATION_NOT_INITIALIZED;
> > > > In this case |needMulti| is true and |context->multi| = 0.
> > > > 
> > > > I'm not sure what I did wrong - I followed the same method as you used
for
> > > > AES-CBC, just using CKM_NSS_AES_KEY_WRAP where appropriate and adjusting
> the
> > > > length checks. Does the AES-KW cipher need to be enabled somehow, like
> > > OpenSSL's
> > > > AddCipher()?
> > > > 
> > > > After I hit this snag, I searched around to see how other people were
> doing
> > > > AES-KW, and found several examples that used PK11_UnwrapSymKey with
> generic
> > > data
> > > > as I am doing now, so I figured that was the 'canonical' way.
> > > > 
> > > > Ryan, any thoughts?
> > > 
> > > Don't use PK11_CipherOp
> > > 
> > > You need to use PK11_Encrypt / PK11_Decrypt, which was introduced when GCM
> > > support was introduced, and does 'single-shot' (eg: it doesn't use
> > > EncryptUpdate, which is what sets needMulti)
> > 
> > Ok, I confirmed that works but I have a concern.
> > 
> > Right now, we are using the presence / absence of the PK11_Encrypt to detect
> > AES-GCM support. If we use this function as part of the AES-KW
implementation,
> > we will tie the AES-KW cipher to >=NSS 3.15 just like we had to for AES-GCM.
> And
> > so we will need the same runtime checking ugliness around AES-KW. Using the
> > older method avoids this issue.
> 
> Yeah, but the older methods are always multi-shot, which is why we introduced
> the new method - it was the only (public API) way to get to the single-shot
> methods, because AES-GCM only supports single-shot.
> 
> Up until then, NSS hadn't needed to expose it, and that AES-KW was implemented
> in terms of it was an implementation detail of softoken.
> 
> FWIW, it's an NSS "bug" that AES-KW supports C_Encrypt/C_Decrypt at all - the
> spec is clear it only supports C_WrapKey/C_UnwrapKey. I suspect we'll move off
> NSS before this becomes an issue, but just an example of us relying on
undefined
> behaviour.

It seems to me this makes a good case to use PK11_UnwrapSymKey /
PK11_UnwrapSymKey (wrappers over C_WrapKey/C_UnwrapKey) instead. They are fully
supported and do not require the most recent NSS version.

> See 6.13 of http://www.cryptsoft.com/pkcs11doc/STANDARD/pkcs-11v2-30m1-d7.pdf
.
> Only CKM_AES_KEY_WRAP_PAD supports Enc/Dec.

I can't parse that section. "The CKM_AES_KEY_WRAP mechanism can wrap a key of
any length. ... The CKM_AES_KEY_WRAP mechanism can only encrypt a block of data
whose size is an exact multiple of the AES Key Wrap algorithm block size." I
think there is a typo here, where the first sentence should read
CKM_AES_KEY_WRAP_PAD. Agreed?

I assume we would never want to use CKM_AES_KEY_WRAP_PAD because of the
arbitrary zero padding. It is clearly not RFC5649. This would be another reason
to stick with PK11_UnwrapSymKey / PK11_UnwrapSymKey.

[padolph, 2014-03-14 17:10:25]

On 2014/03/14 04:25:41, padolph wrote:
> On 2014/03/14 01:33:17, Ryan Sleevi wrote:
> > On 2014/03/14 01:01:09, padolph wrote:
> > > On 2014/03/13 01:44:17, Ryan Sleevi wrote:
> > > > On 2014/03/13 01:26:33, padolph wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > > > > File content/child/webcrypto/platform_crypto_nss.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
> > > > > content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily
> viewed
> > > as
> > > > a
> > > > > symmetric key to be unwrapped (decrypted).
> > > > > On 2014/03/13 00:17:18, eroman wrote:
> > > > > > Deferring to @rsleevi. I had the impression from Ryan that there was
a
> > > > > mechanism
> > > > > > that could be used directly with decryption. So the way this would
> work
> > is
> > > > to
> > > > > do
> > > > > > decryption to a buffer, and then import that as a jwk key.
> > > > > 
> > > > > I tried that method earlier but could not make it work. NSS returns an
> > error
> > > > > from PK11_CipherOp().
> > > > > 
> > > > > The actual failure is in sftk_GetContext() in pkcs11c.c:344:
> > > > >    
> > > >
> if((context==NULL)||(context->type!=type)||(needMulti&&!(context->multi))){
> > > > >         sftk_FreeSession(session);
> > > > > 	return CKR_OPERATION_NOT_INITIALIZED;
> > > > > In this case |needMulti| is true and |context->multi| = 0.
> > > > > 
> > > > > I'm not sure what I did wrong - I followed the same method as you used
> for
> > > > > AES-CBC, just using CKM_NSS_AES_KEY_WRAP where appropriate and
adjusting
> > the
> > > > > length checks. Does the AES-KW cipher need to be enabled somehow, like
> > > > OpenSSL's
> > > > > AddCipher()?
> > > > > 
> > > > > After I hit this snag, I searched around to see how other people were
> > doing
> > > > > AES-KW, and found several examples that used PK11_UnwrapSymKey with
> > generic
> > > > data
> > > > > as I am doing now, so I figured that was the 'canonical' way.
> > > > > 
> > > > > Ryan, any thoughts?
> > > > 
> > > > Don't use PK11_CipherOp
> > > > 
> > > > You need to use PK11_Encrypt / PK11_Decrypt, which was introduced when
GCM
> > > > support was introduced, and does 'single-shot' (eg: it doesn't use
> > > > EncryptUpdate, which is what sets needMulti)
> > > 
> > > Ok, I confirmed that works but I have a concern.
> > > 
> > > Right now, we are using the presence / absence of the PK11_Encrypt to
detect
> > > AES-GCM support. If we use this function as part of the AES-KW
> implementation,
> > > we will tie the AES-KW cipher to >=NSS 3.15 just like we had to for
AES-GCM.
> > And
> > > so we will need the same runtime checking ugliness around AES-KW. Using
the
> > > older method avoids this issue.
> > 
> > Yeah, but the older methods are always multi-shot, which is why we
introduced
> > the new method - it was the only (public API) way to get to the single-shot
> > methods, because AES-GCM only supports single-shot.
> > 
> > Up until then, NSS hadn't needed to expose it, and that AES-KW was
implemented
> > in terms of it was an implementation detail of softoken.
> > 
> > FWIW, it's an NSS "bug" that AES-KW supports C_Encrypt/C_Decrypt at all -
the
> > spec is clear it only supports C_WrapKey/C_UnwrapKey. I suspect we'll move
off
> > NSS before this becomes an issue, but just an example of us relying on
> undefined
> > behaviour.
> 
> It seems to me this makes a good case to use PK11_UnwrapSymKey /
> PK11_UnwrapSymKey (wrappers over C_WrapKey/C_UnwrapKey) instead. They are
fully
> supported and do not require the most recent NSS version.
> 
> > See 6.13 of
http://www.cryptsoft.com/pkcs11doc/STANDARD/pkcs-11v2-30m1-d7.pdf
> .
> > Only CKM_AES_KEY_WRAP_PAD supports Enc/Dec.
> 
> I can't parse that section. "The CKM_AES_KEY_WRAP mechanism can wrap a key of
> any length. ... The CKM_AES_KEY_WRAP mechanism can only encrypt a block of
data
> whose size is an exact multiple of the AES Key Wrap algorithm block size." I
> think there is a typo here, where the first sentence should read
> CKM_AES_KEY_WRAP_PAD. Agreed?
> 
> I assume we would never want to use CKM_AES_KEY_WRAP_PAD because of the
> arbitrary zero padding. It is clearly not RFC5649. This would be another
reason
> to stick with PK11_UnwrapSymKey / PK11_UnwrapSymKey.

So, please consider this CL as-is (uses PK11_UnwrapSymKey / PK11_UnwrapSymKey)
in light of this discussion.

[Ryan Sleevi, 2014-03-14 21:42:04]

NSS LGTM, deferring to eroman for the rest.

[eroman, 2014-03-14 22:14:52]

IMPORTANT: please answer
https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...

Other than that looks good.

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

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:493: Status
AesKwUnwrapSymKey(const CryptoData& wrapped_key_data,
I don't like this name as it is a subtle re-ordering of the existing function
UnwrapSymKeyAesKw(). At first I didn't even realize they were different names.

Some alternate suggestions:
  UnwrapSymKeyAesKwForMechanism()
  UnwrapSymKeyAesKwHelper()
  DoUnwrapSymKeyAesKw()
  UnwrapSymKeyAesKw() --> (function overload)

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1265: SECItem* key_data =
PK11_GetKeyData(decrypted.get());
Can you make this const?

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

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:24: bool KeyUsageAllows(const
blink::WebCryptoKey& key, unsigned int usage) {
This is not what I expect as a caller to KeyUsageAllows().

Instead a create a new function called:

KeyUsageAllowsAnyOf(key, usages);

And also, make the usages parameter of type blink::WebCryptoKeyUsageMask rather
than unsigned int.

Otherwise I am afraid this could be used dangerously in the future.

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:307: return ImportKey(format,
I don't think we should return the error from ImportKey().

Two options I suggest for moving forward on this changelist are:
 (1) Convince me that it is safe to return the error from ImportKey()
 (2) Replace any error from ImportKey() with a generic one like "Import failed
while unwrapping"

For expediency I would go with (2), and we can revisit the decision later if we
want to expose more errors.

My concern is that when unwrapping the key data, the encrypted contents should
be totally private from the web application. By returning the import error, we
may be telling the caller something about the wrapped data. Especially so with
the JWK format, where we tell the caller about unrecognized or parameters, so
they can learn something about encrypted JSON data.

I am not a crypto expert so maybe I am totally wrong it this isn't an issue...
But until I am convinced otherwise, please hide these errors.

I wasn't concerned about this issue for symkey unwrapping, since from what I
could tell the only error we return is the generic Status::Error(). However for
full blown ImportKey() we have a lot more errors.

Cheers, and thanks for thinking about this issue.

[padolph, 2014-03-17 03:24:35]

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

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:493: Status
AesKwUnwrapSymKey(const CryptoData& wrapped_key_data,
On 2014/03/14 22:14:53, eroman wrote:
> I don't like this name as it is a subtle re-ordering of the existing function
> UnwrapSymKeyAesKw(). At first I didn't even realize they were different names.
> 
> Some alternate suggestions:
>   UnwrapSymKeyAesKwForMechanism()
>   UnwrapSymKeyAesKwHelper()
>   DoUnwrapSymKeyAesKw()
>   UnwrapSymKeyAesKw() --> (function overload)

Chose UnwrapSymKeyAesKw. Thank you - I agree it was confusing.

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1254: // temporarily viewed as a
symmetric key to be unwrapped (decrypted).
On 2014/03/13 01:26:34, padolph wrote:
> On 2014/03/13 00:17:18, eroman wrote:
> > Deferring to @rsleevi. I had the impression from Ryan that there was a
> mechanism
> > that could be used directly with decryption. So the way this would work is
to
> do
> > decryption to a buffer, and then import that as a jwk key.
> 
> I tried that method earlier but could not make it work. NSS returns an error
> from PK11_CipherOp().
> 
> The actual failure is in sftk_GetContext() in pkcs11c.c:344:
>    
if((context==NULL)||(context->type!=type)||(needMulti&&!(context->multi))){
>         sftk_FreeSession(session);
> 	return CKR_OPERATION_NOT_INITIALIZED;
> In this case |needMulti| is true and |context->multi| = 0.
> 
> I'm not sure what I did wrong - I followed the same method as you used for
> AES-CBC, just using CKM_NSS_AES_KEY_WRAP where appropriate and adjusting the
> length checks. Does the AES-KW cipher need to be enabled somehow, like
OpenSSL's
> AddCipher()?
> 
> After I hit this snag, I searched around to see how other people were doing
> AES-KW, and found several examples that used PK11_UnwrapSymKey with generic
data
> as I am doing now, so I figured that was the 'canonical' way.
> 
> Ryan, any thoughts?

After a round of discussions with Ryan and experiments with both PK11_CipherOp()
and PK11_Decrypt(), we agreed to stay with PK11_UnwrapSymKey().

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/platform_crypto_nss.cc:1265: SECItem* key_data =
PK11_GetKeyData(decrypted.get());
On 2014/03/14 22:14:53, eroman wrote:
> Can you make this const?

Done.

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

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:24: bool KeyUsageAllows(const
blink::WebCryptoKey& key, unsigned int usage) {
On 2014/03/14 22:14:53, eroman wrote:
> This is not what I expect as a caller to KeyUsageAllows().
> 
> Instead a create a new function called:
> 
> KeyUsageAllowsAnyOf(key, usages);
> 
> And also, make the usages parameter of type blink::WebCryptoKeyUsageMask
rather
> than unsigned int.
> 
> Otherwise I am afraid this could be used dangerously in the future.

Done.

https://codereview.chromium.org/195983010/diff/20001/content/child/webcrypto/...
content/child/webcrypto/shared_crypto.cc:307: return ImportKey(format,
On 2014/03/14 22:14:53, eroman wrote:
> I don't think we should return the error from ImportKey().
> 
> Two options I suggest for moving forward on this changelist are:
>  (1) Convince me that it is safe to return the error from ImportKey()
>  (2) Replace any error from ImportKey() with a generic one like "Import failed
> while unwrapping"
> 
> For expediency I would go with (2), and we can revisit the decision later if
we
> want to expose more errors.
> 
> My concern is that when unwrapping the key data, the encrypted contents should
> be totally private from the web application. By returning the import error, we
> may be telling the caller something about the wrapped data. Especially so with
> the JWK format, where we tell the caller about unrecognized or parameters, so
> they can learn something about encrypted JSON data.
> 
> I am not a crypto expert so maybe I am totally wrong it this isn't an issue...
> But until I am convinced otherwise, please hide these errors.
> 
> I wasn't concerned about this issue for symkey unwrapping, since from what I
> could tell the only error we return is the generic Status::Error(). However
for
> full blown ImportKey() we have a lot more errors.
> 
> Cheers, and thanks for thinking about this issue.

Thanks. I had seen your earlier comment in the code about this issue and meant
to account for it, but forgot in this CL.

I agree that we should not expose import failures in the second step of this
combined unwrap operation. Yes, it would allow information about the encrypted
JWK structure to leak.

Changed to report just Status::Error() for now. If we want more granularity in
the future, we can add a filter parameter to the ImportKey() function (perhaps
optional with a default) to enable switching between "chatty" and "secret" mode.

[padolph, 2014-03-17 04:23:22]

On 2014/03/17 03:24:35, padolph wrote:
> Changed to report just Status::Error() for now. If we want more granularity in
> the future, we can add a filter parameter to the ImportKey() function (perhaps
> optional with a default) to enable switching between "chatty" and "secret"
mode.

I added a test to validate this behavior. I will replace it with a better test
once RSA-ES unwrapping is implemented.

[eroman, 2014-03-17 19:47:46]

lgtm

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:202: // Performs AES-KW decryption on
the input |data|, of which no assumptions are
Not sure what "of which no assumptions are made" means, since we assume that it
is "at least 24 bytes and a multiple of 8 bytes".

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

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto.cc:374: if (!KeyUsageAllows(key,
blink::WebCryptoKeyUsageDecrypt))
I don't like having an extra switch for verifying the usage.

How about instead having different entry points, like:

DecryptForUnwrap() --> or add the check directly into
UnwrapKeyDecryptAndImport()
Decrypt()
DecryptDontCheckUsages()

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

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:2691: // trust the result
because I say so.
Another solution would be to introduce an error along the lines of
Status::ErrorImportKeyFailedForUnwrap(), and code the test expectations against
that.

[padolph, 2014-03-17 22:12:49]

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
File content/child/webcrypto/platform_crypto.h (right):

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/platform_crypto.h:202: // Performs AES-KW decryption on
the input |data|, of which no assumptions are
On 2014/03/17 19:47:47, eroman wrote:
> Not sure what "of which no assumptions are made" means, since we assume that
it
> is "at least 24 bytes and a multiple of 8 bytes".

Done.

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

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto.cc:374: if (!KeyUsageAllows(key,
blink::WebCryptoKeyUsageDecrypt))
On 2014/03/17 19:47:47, eroman wrote:
> I don't like having an extra switch for verifying the usage.
> 
> How about instead having different entry points, like:
> 
> DecryptForUnwrap() --> or add the check directly into
> UnwrapKeyDecryptAndImport()
> Decrypt()
> DecryptDontCheckUsages()

Done.

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

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:2691: // trust the result
because I say so.
On 2014/03/17 19:47:47, eroman wrote:
> Another solution would be to introduce an error along the lines of
> Status::ErrorImportKeyFailedForUnwrap(), and code the test expectations
against
> that.

That error would tell the caller that encryption passed but import failed. It
seems a little safer to not distinguish these during unwrap.

[eroman, 2014-03-17 23:54:31]

patchset 7 LGTM

[eroman, 2014-03-17 23:56:22]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-17 23:57:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/195983010/120001

[commit-bot: I haz the power, 2014-03-18 00:12:20]

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

[commit-bot: I haz the power, 2014-03-18 00:12:20]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[padolph, 2014-03-18 00:18:12]

The CQ bit was checked by padolph@netflix.com

[commit-bot: I haz the power, 2014-03-18 00:23:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/195983010/120001

[commit-bot: I haz the power, 2014-03-18 00:34:34]

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

[commit-bot: I haz the power, 2014-03-18 00:34:35]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[eroman, 2014-03-18 01:02:13]

still lgtm

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

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:2691: // trust the result
because I say so.
On 2014/03/17 22:12:49, padolph wrote:
> On 2014/03/17 19:47:47, eroman wrote:
> > Another solution would be to introduce an error along the lines of
> > Status::ErrorImportKeyFailedForUnwrap(), and code the test expectations
> against
> > that.
> 
> That error would tell the caller that encryption passed but import failed. It
> seems a little safer to not distinguish these during unwrap.

If we care about distinguishing import vs decrypt errors, then should the
decrypt error be masked as well?

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

https://codereview.chromium.org/195983010/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto.cc:29: bool KeyUsageAllowsAnyOf(const
blink::WebCryptoKey& key,
This is unused, and why the commit queue failed it

[padolph, 2014-03-18 01:23:01]

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

https://codereview.chromium.org/195983010/diff/120001/content/child/webcrypto...
content/child/webcrypto/shared_crypto.cc:29: bool KeyUsageAllowsAnyOf(const
blink::WebCryptoKey& key,
On 2014/03/18 01:02:13, eroman wrote:
> This is unused, and why the commit queue failed it

Done.

[padolph, 2014-03-18 01:23:59]

The CQ bit was checked by padolph@netflix.com

[commit-bot: I haz the power, 2014-03-18 01:24:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/195983010/140001

[commit-bot: I haz the power, 2014-03-18 01:29:14]

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

[commit-bot: I haz the power, 2014-03-18 01:29:16]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[eroman, 2014-03-18 01:36:08]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-03-18 01:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/195983010/140001

[padolph, 2014-03-18 02:54:15]

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

https://codereview.chromium.org/195983010/diff/100001/content/child/webcrypto...
content/child/webcrypto/shared_crypto_unittest.cc:2691: // trust the result
because I say so.
On 2014/03/18 01:02:13, eroman wrote:
> On 2014/03/17 22:12:49, padolph wrote:
> > On 2014/03/17 19:47:47, eroman wrote:
> > > Another solution would be to introduce an error along the lines of
> > > Status::ErrorImportKeyFailedForUnwrap(), and code the test expectations
> > against
> > > that.
> > 
> > That error would tell the caller that encryption passed but import failed.
It
> > seems a little safer to not distinguish these during unwrap.
> 
> If we care about distinguishing import vs decrypt errors, then should the
> decrypt error be masked as well?

I don't think so. At that stage it is no different than a regular decrypt, and
we already give error details about decryption. But decryption errors are more
or less generic in themselves, certainly not as detailed as JWK import errors.

I just don't want to make it obvious at what point it breaks.

[commit-bot: I haz the power, 2014-03-18 04:14:30]

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

[commit-bot: I haz the power, 2014-03-18 04:14:32]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[eroman, 2014-03-18 05:55:04]

Looks like legitimate failure: need to add stub for
content::webcrypto::platform::DecryptAesKw() to openssl version.

[padolph, 2014-03-18 16:06:36]

The CQ bit was checked by padolph@netflix.com

[commit-bot: I haz the power, 2014-03-18 16:07:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/195983010/160001

[commit-bot: I haz the power, 2014-03-18 17:59:38]

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

[commit-bot: I haz the power, 2014-03-18 17:59:39]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[padolph, 2014-03-18 18:09:17]

The CQ bit was checked by padolph@netflix.com

[commit-bot: I haz the power, 2014-03-18 18:29:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/padolph@netflix.com/195983010/160001

[commit-bot: I haz the power, 2014-03-18 21:23:02]

Change committed as 257759