I'll take a stab at this in my spare time.

This should be straight forward.

deriveBits() works when using HMAC-SHA-1 as the PRF but PK11_PBEKeyGen() fails when choosing HMAC-SHA-256. I'm not familiar with NSS so I hope you can shed some light on this :) With that working, deriveKey() should be easy to implement. generateKey() might be harder only due to the chrome dialog we have to open to ask for a password.

(In reply to Tim Taubert [:ttaubert] from comment #3) > deriveBits() works when using HMAC-SHA-1 as the PRF but PK11_PBEKeyGen() > fails when choosing HMAC-SHA-256. It's failing due to SEC_ERROR_INVALID_ALGORITHM. That sounds bad, the spec states that the developer can pass any algorithm that implements the digest operation.

Right, /* only SHA1_HMAC is currently supported by PKCS #11 */ https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pbe.c#811

Comment on attachment 8435662 [details] [diff] [review] 0003-Bug-1021607-Implement-import-key-operation-for-PBKDF.patch r=me fwiw, but none of this is really my area of expertise. ;)

Comment on attachment 8435666 [details] [diff] [review] 0004-Bug-1021607-Implement-derive-bits-operation-for-PBKD.patch Hmm. This part is not in the spec. Should we be exposing it to all callers? >+ : mSymKey(aKey.GetSymKey()) Need to handle OOM here, right? >+ KeyAlgorithm algorithm(global, hashName); Using this just to get the mechanism is really not great. For one thing, this is a refcounted object, so explicitly calling its destructor like this is really dangerous. We need a different way to do this bit. I recommend moving the code that converts strings to mechanisms into a static function on KeyAlgorithm that is called from both here and the KeyAlgorithm ctor.

Comment on attachment 8435662 [details] [diff] [review] 0003-Bug-1021607-Implement-import-key-operation-for-PBKDF.patch Review of attachment 8435662 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer we not land this until something can actually be done with PBKDF2 keys. It is tempting to go ahead and define import for all possible keys we might be interested in -- I have a patch in my queue that does this for OAEP and PSS -- but exposing that to developers seems to be making a false promise. r=me with these comments addressed. ::: dom/crypto/WebCryptoTask.cpp @@ +698,5 @@ > + if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_PBKDF2) && > + !aFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_RAW)) { > + mEarlyRv = NS_ERROR_DOM_DATA_ERR; > + return; > + } Does this really need to be special-cased like this? The only other possible format for symmetric keys is JWK. When we add JWK support, we'll be checking algorithm fields anyway, so it will fail at that point. @@ +757,5 @@ > + } else if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_PBKDF2)) { > + if (mKey->HasUsageOtherThan(Key::DERIVEKEY)) { > + return NS_ERROR_DOM_DATA_ERR; > + } > + algorithm = new AesKeyAlgorithm(global, mAlgName, length); Re-using AesKeyAlgorithm like this seems like a fine idea, but if you do, please rename it to something more general, like BasicSymmetricKeyAlgorithm.

(In reply to Richard Barnes [:rbarnes] from comment #8) > I would prefer we not land this until something can actually be done with > PBKDF2 keys. I agree, this should wait for deriveBits() and deriveKey() to be implemented. > ::: dom/crypto/WebCryptoTask.cpp > @@ +698,5 @@ > > + if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_PBKDF2) && > > + !aFormat.EqualsLiteral(WEBCRYPTO_KEY_FORMAT_RAW)) { > > + mEarlyRv = NS_ERROR_DOM_DATA_ERR; > > + return; > > + } > > Does this really need to be special-cased like this? The only other > possible format for symmetric keys is JWK. When we add JWK support, we'll > be checking algorithm fields anyway, so it will fail at that point. Good point, it really doesn't need the special case. We'll also throw NS_ERROR_DOM_NOT_SUPPORTED_ERR then which is what the spec actually says. > @@ +757,5 @@ > > + } else if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_PBKDF2)) { > > + if (mKey->HasUsageOtherThan(Key::DERIVEKEY)) { > > + return NS_ERROR_DOM_DATA_ERR; > > + } > > + algorithm = new AesKeyAlgorithm(global, mAlgName, length); > > Re-using AesKeyAlgorithm like this seems like a fine idea, but if you do, > please rename it to something more general, like BasicSymmetricKeyAlgorithm. Sure, that sounds like a good idea.

(In reply to Boris Zbarsky [:bz] from comment #7) > Hmm. This part is not in the spec. Should we be exposing it to all callers? You mean .deriveBits() is not available for PBKDF2? Is that stated somewhere explicitly? I guess this is implicit due to .importKey() only accepting "deriveKey" as usage? Makes sense. I was probably confused by the algorithm table that seems to list operations and not methods? Although that's missing generateKey for PBKDF2?

Comment on attachment 8435666 [details] [diff] [review] 0004-Bug-1021607-Implement-derive-bits-operation-for-PBKD.patch Review of attachment 8435666 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this seems fine. I share Boris's concern about name-to-mechanism mapping. That might be a good thing to put up in WebCryptoCommon.h, for the cases where it works. It would also be nice if you could somehow make it so that deriveKey and deriveBits don't end up duplicating a whole bunch of code. ISTM there are basically two ways to do this: 1. Pass a boolean to the DerivePbkdfBitsTask ctor and handle it in DoCrypto/AfterCrypto 2. Define deriveKey() as deriveBits().then(importKey) If the former can be made to work, it's probably simpler. Basically the same thing as CreateEncryptDecryptTask and CreateSignVerifyTask. You appear to have discovered the dependency on bug 554827 :) I'll see what I can do to get that un-stuck.

> > @@ +757,5 @@ > > > + } else if (mAlgName.EqualsLiteral(WEBCRYPTO_ALG_PBKDF2)) { > > > + if (mKey->HasUsageOtherThan(Key::DERIVEKEY)) { > > > + return NS_ERROR_DOM_DATA_ERR; > > > + } > > > + algorithm = new AesKeyAlgorithm(global, mAlgName, length); > > > > Re-using AesKeyAlgorithm like this seems like a fine idea, but if you do, > > please rename it to something more general, like BasicSymmetricKeyAlgorithm. > > Sure, that sounds like a good idea. Actually, on second thought: We should be consistent with the spec, but the spec is wrong on this. I filed a bug. https://www.w3.org/Bugs/Public/show_bug.cgi?id=26011

> You mean .deriveBits() is not available for PBKDF2? I meant specifically Pbkdf2Params, but I see it in the spec now. I have no idea why I didn't find it before.

Carrying over r=bz,rbarnes.

(In reply to Richard Barnes [:rbarnes] from comment #12) > Actually, on second thought: We should be consistent with the spec, but the > spec is wrong on this. I filed a bug. > https://www.w3.org/Bugs/Public/show_bug.cgi?id=26011 Ah, somehow I didn't see the bugmail on this. Feel free to ignore the patch introducing BasicSymmetricKeyAlgorithm until we get an answer on this. The suggestion you made in the w3 bug tracker seems like the better solution.

(In reply to Tim Taubert [:ttaubert] from comment #10) > You mean .deriveBits() is not available for PBKDF2? Is that stated somewhere > explicitly? I guess this is implicit due to .importKey() only accepting > "deriveKey" as usage? Makes sense. So I'm still a little confused. It seems like .deriveBits() is available for PBKDF2, the table has a check mark for it. OTOH .importKey() for PBKDF2 doesn't accept anything other than "deriveKey" as usage. Either .deriveBits() should not be exposed for PBKDF2 or the description for operations "Generate key" and "Import key" should be updated to include the "deriveBits" usage, no?

Forgot to update the place where we check HmacKeyGenParams and determine a default key length.

This is pretty much the same patch as before, only using MapAlgorithmNameToMechanism().

This patch implements .deriveKey() by re-using ImportSymmetricKeyTask. I'd really like to get your feedback on the approach here. I hit another problem, which might be a spec problem. When deriving from an imported PBKDF2 key the third parameter is |AlgorithmIdentifier derivedKeyType|. This specifies a symmetric algorithm that supports raw key import (those currently are HMAC and AES variants). importKey("raw", "PBKDF2", ...).then(x => { deriveKey("PBKDF2", x, "HMAC-SHA-1", ...).then(key => { console.log(key); }); }); For HMAC it's quite simple, we can deduce the number of bits to derive from the hash function's block size. For AES however there doesn't seem any possibility to pass any of the 128/192/256 bit key lengths. This would mean that if I wanted to use PBKDF2 to derive an AES 256-bit key I would have to call deriveBits("raw", {name: "AES-GCM", length: 256}).then(deriveKey) myself, which doesn't seem like that is the goal. Am I right that this is a problem with the spec or am I missing something here?

(In reply to Tim Taubert [:ttaubert] from comment #21) > This would mean that if I wanted to use PBKDF2 to derive an AES 256-bit key > I would have to call deriveBits("raw", {name: "AES-GCM", length: > 256}).then(deriveKey) myself, which doesn't seem like that is the goal. I actually meant to write: deriveBits("PBKDF2", x, 256).then(k => importKey("raw", k, {name: "AES-GCM", length: 256}))

Comment on attachment 8436532 [details] [diff] [review] 0005-Bug-1021607-Implement-derive-key-operation-for-PBKDF.patch Review of attachment 8436532 [details] [diff] [review]: ----------------------------------------------------------------- You're on a good path here. Just a few minor comments. ::: dom/crypto/WebCryptoTask.cpp @@ +633,5 @@ > > class ImportKeyTask : public WebCryptoTask > { > public: > ImportKeyTask(JSContext* aCx, I see what you're doing here, and I think it's mostly fine. I would prefer if we could leave the aFormat and aKeyData parameters here, since it makes the importKey cases simpler. Then when deriveKey creates its ImportSymmetricKeyTask, it can pass in default or dummy values. For format, in the deriveKey case, the value will always be WEBCRYTPO_FORMAT_RAW, so you can just provide NS_LITERAL_STRING(WEBCRYPTO_FORMAT_RAW) to the constructor. But this design pattern is also likely to get used for unwrapKey, in which case it will have a format string to pass through at ctor time. You might check with bz, though, to verify how that should be done, especially for KeyData. @@ +755,5 @@ > mEarlyComplete = true; > return NS_OK; > } > > + void SetKeyData(const nsAString& aFormat, const KeyData& aKeyData) It seems like this should just branch based on mFormat. Also, you need to handle OOM or bz will yell at you :) @@ +1423,5 @@ > + > +private: > + virtual void Resolve() MOZ_OVERRIDE { > + mTask->SetKeyData(mResult); > + mTask->DispatchWithPromise(mResultPromise); I do like this approach. It seems like it minimizes code duplication, and it will be a good precedent for unwrapKey/wrapKey. Have you compared it to the event model that's laid out in the spec? @@ +1524,5 @@ > algName.EqualsLiteral(WEBCRYPTO_ALG_HMAC)) { > + ImportSymmetricKeyTask* task = new ImportSymmetricKeyTask(aCx, aAlgorithm, > + aExtractable, > + aKeyUsages); > + task->SetKeyData(aFormat, aKeyData); This will be unnecessary if you put those parameters back. ::: dom/crypto/test/tests.js @@ +962,5 @@ > + > + crypto.subtle.importKey("raw", key, alg, false, ["deriveKey"]) > + .then( doDerive, fail ) > + .then( complete(that, function (x) { > + return hasKeyFields(x) && x.algorithm.length == 512; You should attempt to do something with the key, like an encryption or HMAC. I got caught with some errors with importKey where it looked like the key imported successfully, but the actual key handle inside the key was bogus.

(In reply to Tim Taubert [:ttaubert] from comment #21) > This would mean that if I wanted to use PBKDF2 to derive an AES 256-bit key > I would have to call deriveBits("raw", {name: "AES-GCM", length: > 256}).then(deriveKey) myself, which doesn't seem like that is the goal. > > Am I right that this is a problem with the spec or am I missing something > here? The derivedKeyType input to deriveKey is the same type of object as the algorithm input to importKey, so there should be no problem. Your deriveBits call above seems really bizarre; do you mean this? > deriveBits("PBKDF2", key, 256).then(function(x) { > return importKey("raw", x, "AES-GCM", false, ['encrypt', 'decrypt']); > }) What you should be able to do is: > deriveKey("PBKDF2", key, {name:"AES-GCM", length:256}, false, ['encrypt', 'decrypt']) Which is sort of what you're doing anyway :)

Same as before but with OOM check.

Introduces GetDefaultKeySizeForAlgorithm() and MapHashAlgorithmNameToBlockSize(). The latter is handy for HMAC key generation and both are needed to properly support PBKDF2 key derivation for AES and HMAC.

Implemented key derivation as suggested, and added hmac signing and verification to the deriveKey() test. I'm not super sure about the event model but looking at the spec it seems like the current behavior should be fine... Can you please elaborate what exactly you're concerned about here?

Comment on attachment 8449745 [details] [diff] [review] 0005-Bug-1021607-Introduce-GetDefaultKeySizeForAlgorithm.patch Review of attachment 8449745 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/WebCryptoTask.cpp @@ +178,5 @@ > + > + // Read AES key length from given algorithm object. > + if (algName.EqualsLiteral(WEBCRYPTO_ALG_AES_CBC) || > + algName.EqualsLiteral(WEBCRYPTO_ALG_AES_CTR) || > + algName.EqualsLiteral(WEBCRYPTO_ALG_AES_GCM)) { Please open a bug to use this in GenerateSymmetricKeyTask. Or go ahead and fix it here (not sure what etiquette is).

Comment on attachment 8449741 [details] [diff] [review] 0004-Bug-1021607-Implement-derive-bits-operation-for-PBKD.patch, v3 Review of attachment 8449741 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks OK to me. You should probably get bz and keeler to review for the IDL and PSM parts, respectively. ::: dom/crypto/WebCryptoTask.cpp @@ +1441,5 @@ > + } > + > + // Check that we got a symmetric key > + if (mSymKey.Length() == 0) { > + mEarlyRv = NS_ERROR_DOM_DATA_ERR; NS_ERROR_DOM_INVALID_ACCESS_ERR? @@ +1497,5 @@ > + ATTEMPT_BUFFER_TO_SECITEM(salt, mSalt); > + > + ScopedSECAlgorithmID alg_id(PK11_CreatePBEV2AlgorithmID( > + SEC_OID_PKCS5_PBKDF2, SEC_OID_HMAC_SHA1, mHashOidTag, > + mLength, mIterations, salt)); I'm not familiar with how PK11_CreatePBEV2AlgorithmID works. Can you clarify what purpose the SEC_OID_HMAC_SHA1 fixed value serves?

Comment on attachment 8449757 [details] [diff] [review] 0006-Bug-1021607-Implement-derive-keys-operation-for-PBKD.patch, v2 Review of attachment 8449757 [details] [diff] [review]: ----------------------------------------------------------------- Couple of minor comments. Please check with bz to see if anything about the dummy data scheme causes him concern. ::: dom/crypto/WebCryptoTask.cpp @@ +980,5 @@ > } > > + void SetKeyData(CryptoBuffer aKeyData) > + { > + mKeyData = aKeyData; Seems like this needs to return nsresult to handle OOM. ::: dom/crypto/test/tests.js @@ +1073,5 @@ > + hash: {name: "SHA-1"} > + }; > + > + return crypto.subtle.deriveKey(alg, x, algDerived, false, ["sign", "verify"]) > + .then(x => { I would prefer if we could use traditional closures here instead of arrow functions, just to be consistent among the tests. @@ +1091,5 @@ > + var data = new Uint8Array(1024); > + > + return crypto.subtle.sign("HMAC", x, data) > + .then(sig => { > + return crypto.subtle.verify("HMAC", x, sig, data); Here as well.

(In reply to Richard Barnes [:rbarnes] from comment #30)d SetKeyData(CryptoBuffer aKeyData) > > + { > > + mKeyData = aKeyData; > > Seems like this needs to return nsresult to handle OOM. I thought an OOM here would be handled by the |if (mKeyData.Length() == 0)| check in BeforeCrypto(), no?

Carrying over r=rbarnes.

Comment on attachment 8451272 [details] [diff] [review] 0006-Bug-1021607-Implement-derive-keys-operation-for-PBKD.patch, v3 Boris, does the |CryptoOperationData dummy| usage here in DerivePbkdfKeyTask look ok to you? In combination with the length check in ImportSymmetricKeyTask::BeforeCrypto()?

Carrying over r=rbarnes. (In reply to Richard Barnes [:rbarnes] from comment #29) > > + // Check that we got a symmetric key > > + if (mSymKey.Length() == 0) { > > + mEarlyRv = NS_ERROR_DOM_DATA_ERR; > > NS_ERROR_DOM_INVALID_ACCESS_ERR? Done. > @@ +1497,5 @@ > > + ScopedSECAlgorithmID alg_id(PK11_CreatePBEV2AlgorithmID( > > + SEC_OID_PKCS5_PBKDF2, SEC_OID_HMAC_SHA1, mHashOidTag, > > + mLength, mIterations, salt)); > > I'm not familiar with how PK11_CreatePBEV2AlgorithmID works. Can you > clarify what purpose the SEC_OID_HMAC_SHA1 fixed value serves? Added a comment to clarify that this parameter is unused and would only be used for PBMAC1/PBES2 but not for key generation.

Comment on attachment 8451280 [details] [diff] [review] 0004-Bug-1021607-Implement-derive-bits-operation-for-PBKD.patch, v4 Boris, can you please take a look at the IDL changes? David, can you please review the PSM changes? Thanks!

Carrying over r=rbarnes. Renamed GetDefaultKeySizeForAlgorithm() to GetKeySizeForAlgorithm() as for AES it returns the specified key size. (In reply to Richard Barnes [:rbarnes] from comment #28) > > + // Read AES key length from given algorithm object. > > + if (algName.EqualsLiteral(WEBCRYPTO_ALG_AES_CBC) || > > + algName.EqualsLiteral(WEBCRYPTO_ALG_AES_CTR) || > > + algName.EqualsLiteral(WEBCRYPTO_ALG_AES_GCM)) { > > Please open a bug to use this in GenerateSymmetricKeyTask. Or go ahead and > fix it here (not sure what etiquette is). Fixing here should be fine, did that.

Needed an update because GetKeySizeForAlgorithm() got renamed. Carrying over r=rbarnes.

Comment on attachment 8451280 [details] [diff] [review] 0004-Bug-1021607-Implement-derive-bits-operation-for-PBKD.patch, v4 Review of attachment 8451280 [details] [diff] [review]: ----------------------------------------------------------------- r=me for PSM bits. ::: dom/crypto/WebCryptoTask.cpp @@ +1499,5 @@ > + // Always pass in cipherAlg=SEC_OID_HMAC_SHA1 (i.e. PBMAC1) as this > + // parameter is unused for key generation. It is currently only used > + // for PBKDF2 authentication or key (un)wrapping when specifying an > + // encryption algorithm (PBES2). > + ScopedSECAlgorithmID alg_id(PK11_CreatePBEV2AlgorithmID( nit: I'm not a fan of underscores in variable names. How about "algID" or "algId"? @@ +1500,5 @@ > + // parameter is unused for key generation. It is currently only used > + // for PBKDF2 authentication or key (un)wrapping when specifying an > + // encryption algorithm (PBES2). > + ScopedSECAlgorithmID alg_id(PK11_CreatePBEV2AlgorithmID( > + SEC_OID_PKCS5_PBKDF2, SEC_OID_HMAC_SHA1, mHashOidTag, How about SEC_OID_UNKNOWN instead of SEC_OID_HMAC_SHA1? Looking at the code, I think that will work. ::: security/manager/ssl/src/ScopedNSSTypes.h @@ +273,5 @@ > { > return SECITEM_FreeItem(s, true); > } > > +inline void SECOID_DestroyAlgorithmID_true(SECAlgorithmID * a) nit: 'SECAlgorithmID* a' (I know much of this file doesn't match that style, but it's what we're moving towards)

Comment on attachment 8451280 [details] [diff] [review] 0004-Bug-1021607-Implement-derive-bits-operation-for-PBKD.patch, v4 Review of attachment 8451280 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/crypto/test/tests.js @@ +1047,5 @@ > + } > +); > + > +// ----------------------------------------------------------------------------- > +/*TestArray.addTest( I forgot to ask - why is this commented-out?

(In reply to David Keeler (:keeler) [use needinfo?] from comment #39) > > +// ----------------------------------------------------------------------------- > > +/*TestArray.addTest( > > I forgot to ask - why is this commented-out? That's due to bug 554827. The only supported PRF right now is SHA-1. I tested that the test here succeeds when changing the default PRF to SHA-256 though.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #38) > > + ScopedSECAlgorithmID alg_id(PK11_CreatePBEV2AlgorithmID( > > nit: I'm not a fan of underscores in variable names. How about "algID" or > "algId"? Done. > @@ +1500,5 @@ > > + // parameter is unused for key generation. It is currently only used > > + // for PBKDF2 authentication or key (un)wrapping when specifying an > > + // encryption algorithm (PBES2). > > + ScopedSECAlgorithmID alg_id(PK11_CreatePBEV2AlgorithmID( > > + SEC_OID_PKCS5_PBKDF2, SEC_OID_HMAC_SHA1, mHashOidTag, > > How about SEC_OID_UNKNOWN instead of SEC_OID_HMAC_SHA1? Looking at the code, > I think that will work. I also wanted to do that but sec_pkcs5CreateAlgorithmID() fails when passing SEC_OID_UNKNOWN as cipherAlg: https://mxr.mozilla.org/mozilla-central/source/security/nss/lib/pk11wrap/pk11pbe.c#592 (And to be sure I just changed that and tests do indeed fail.) > > +inline void SECOID_DestroyAlgorithmID_true(SECAlgorithmID * a) > > nit: 'SECAlgorithmID* a' (I know much of this file doesn't match that style, > but it's what we're moving towards) Done.

Comment on attachment 8451280 [details] [diff] [review] 0004-Bug-1021607-Implement-derive-bits-operation-for-PBKD.patch, v4 >+ nsIGlobalObject *global = xpc::GetNativeForGlobal(JS::CurrentGlobalOrNull(aCx)); This is unused. Just drop it. r=me with that.

Comment on attachment 8451293 [details] [diff] [review] 0006-Bug-1021607-Implement-derive-keys-operation-for-PBKD.patch, v4 >+ void SetKeyData(CryptoBuffer aKeyData) This is going to copy the buffer twice, no? Please pass a const ref instead. And add a comment that OOM will just get treated as an error in BeforeCrypto. >+ const nsString format(NS_LITERAL_STRING(WEBCRYPTO_KEY_FORMAT_RAW)); NS_NAMED_LITERAL_STRING r=me

Consolidated patch, incorporating bz comments, and applying cleanly against current mozilla-central. Clean try run here: https://tbpl.mozilla.org/?tree=Try&rev=b874f15f5cbd

I think we should talk about this in the release notes.

Added to the release notes with "WebCrypto API: RSA-OAEP, PBKDF2 and AES-KW support" as wording. bug 1021607 + bug 1034852 + bug 1026398