Closed
Bug 1200341
Opened 9 years ago
Closed 9 years ago
Implement HKDF for WebCrypto
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: renthraysk, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, html5, wsec-crypto, Whiteboard: [adv-main46-])
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
smaug
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.157 Safari/537.36 Steps to reproduce: Implement WebCrypto HKDF and ConcatKDF. Given that DH & ECDH have been implemented, it does seem the common practice to use a KDF to derive keys from the shared secret, and not use the shared secret directly.
Reporter | ||
Updated•9 years ago
|
Reporter | ||
Comment 1•9 years ago
|
||
So it seems cannot derive a PBKDF2 key from a ECDH shared secret. So there are no usable KDFs with DH/ECDH key exchanges.
Reporter | ||
Comment 2•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Keywords: wsec-crypto
Version: 43 Branch → 44 Branch
Updated•9 years ago
|
Component: Untriaged → DOM: Security
Product: Firefox → Core
Updated•9 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 3•9 years ago
|
||
The CONCAT KDF was removed from the spec.
Blocks: web-crypto
Summary: Implement the KDFs in WebCrypto spec → Implement HKDF-CTR for WebCrypto
Assignee | ||
Comment 4•9 years ago
|
||
Note: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27425 HKDF-CTR will be renamed to HKDF and stick to RFC 5869.
Summary: Implement HKDF-CTR for WebCrypto → Implement HKDF for WebCrypto
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
I fixed a few things left and right but this should work and have a good test coverage.
Attachment #8711708 -
Flags: review?(rlb)
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to RenThraysk from comment #2) > Created attachment 8665674 [details] > Attempt to derive a PBKDF2 key from a ECDH exchange This isn't quite how the API is supposed to be used. You first would call deriveBits("ECDH"), then import those as "raw" into an HKDF or PBDFK2 key. Then you can finally call deriveBits() or deriveKey() for either of the KDFs.
Comment 7•9 years ago
|
||
Comment on attachment 8711708 [details] [diff] [review] 0001-Bug-1200341-Implement-HKDF-for-WebCrypto.patch Review of attachment 8711708 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/crypto/WebCryptoTask.cpp @@ +1447,5 @@ > + mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR; > + return; > + } > + > + // JWK is not supported for HKDF and PBKDF2. I realize that JWK support is not in the spec, but I don't see a good technical reason to keep it out. Will support work if we just remove this check? ::: dom/crypto/test/test_WebCrypto_HKDF.html @@ +64,5 @@ > + return crypto.subtle.deriveBits(alg, x, 4); > + } > + > + crypto.subtle.importKey("raw", key, "HKDF", false, ["deriveBits"]) > + .then(deriveBits) For these cases where deriveBits is one line, it would be more readable just to have ".then(k => crypto.subtle.deriveBits(alg, k, 4))" @@ +65,5 @@ > + } > + > + crypto.subtle.importKey("raw", key, "HKDF", false, ["deriveBits"]) > + .then(deriveBits) > + // The last 4 bits should be zeroes (1000 1101 => 1000 0000). What does the parenthetical mean? ::: dom/crypto/test/test_WebCrypto_PBKDF2.html @@ +38,5 @@ > ); > > // ----------------------------------------------------------------------------- > TestArray.addTest( > + "Unwrapping a PBKDF2 key in PKCS8 format should fail", This seems like an unrelated fix, but I'll let it slide. Do you want to test JWK (in)tolerance for HKDF?
Attachment #8711708 -
Flags: review?(rlb) → review+
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Richard Barnes [:rbarnes] from comment #7) > ::: dom/crypto/WebCryptoTask.cpp > @@ +1447,5 @@ > > + mEarlyRv = NS_ERROR_DOM_NOT_SUPPORTED_ERR; > > + return; > > + } > > + > > + // JWK is not supported for HKDF and PBKDF2. > > I realize that JWK support is not in the spec, but I don't see a good > technical reason to keep it out. Will support work if we just remove this > check? Removed. > ::: dom/crypto/test/test_WebCrypto_HKDF.html > @@ +64,5 @@ > > + return crypto.subtle.deriveBits(alg, x, 4); > > + } > > + > > + crypto.subtle.importKey("raw", key, "HKDF", false, ["deriveBits"]) > > + .then(deriveBits) > > For these cases where deriveBits is one line, it would be more readable just > to have ".then(k => crypto.subtle.deriveBits(alg, k, 4))" Sure. > @@ +65,5 @@ > > + } > > + > > + crypto.subtle.importKey("raw", key, "HKDF", false, ["deriveBits"]) > > + .then(deriveBits) > > + // The last 4 bits should be zeroes (1000 1101 => 1000 0000). > > What does the parenthetical mean? It's supposed to describe that here PK11_Derive() derives a full byte, 0x8d. We truncate it to 0x80, i.e. set the 4 LSBs to zero. > Do you want to test JWK (in)tolerance for HKDF? Yes! Added a JWK import test for HKDF and PBKDF2.
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8711708 [details] [diff] [review] 0001-Bug-1200341-Implement-HKDF-for-WebCrypto.patch Olli, can you please look at the .webidl file changes? The latest spec is here: https://dvcs.w3.org/hg/webcrypto-api/raw-file/tip/spec/Overview.html#hkdf-ctr-params It has |label| and |context| parameters that will change to |salt| and |info| soon: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27425
Attachment #8711708 -
Flags: review?(bugs)
Assignee | ||
Comment 10•9 years ago
|
||
r=rbarnes Forgot to rename HkdfCtrParams to HkdfParams. Chrome implemented the same IDL spec btw: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/modules/crypto/NormalizeAlgorithm.cpp#810
Attachment #8711708 -
Attachment is obsolete: true
Attachment #8711708 -
Flags: review?(bugs)
Attachment #8712149 -
Flags: review?(bugs)
Comment 11•9 years ago
|
||
Comment on attachment 8712149 [details] [diff] [review] 0001-Bug-1200341-Implement-HKDF-for-WebCrypto-r-rbarnes-s.patch, v2 We should add BufferSource to some .webidl and just use it here. But that can happen in a different bug.
Attachment #8712149 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•9 years ago
|
||
s/CryptoOperationData/BufferSource r=smaug (over IRC)
Attachment #8712179 -
Flags: review+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e347384b8db https://hg.mozilla.org/integration/mozilla-inbound/rev/8b255b3ff8d3
Comment 14•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e347384b8db https://hg.mozilla.org/mozilla-central/rev/8b255b3ff8d3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8712149 [details] [diff] [review] 0001-Bug-1200341-Implement-HKDF-for-WebCrypto-r-rbarnes-s.patch, v2 Approval Request Comment [Feature/regressing bug #]: [User impact if declined]: [Describe test coverage new/current, TreeHerder]: Extensive test suite. [Risks and why]: Low risk, new API feature. [String/UUID change made/needed]: None. I would like to request uplift to Aurora for both patches here. They introduce the "HKDF" cipher to the WebCrypto API and it would be great to ship that earlier. Both patches apply without any modifications.
Attachment #8712149 -
Flags: approval-mozilla-aurora?
Comment 16•9 years ago
|
||
(We may need to sort out bug 1243438 -- a build failure in disable-eme builds -- before we uplift this to aurora.)
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #16) > (We may need to sort out bug 1243438 -- a build failure in disable-eme > builds -- before we uplift this to aurora.) This was caused by the second changeset which we don't really need on Aurora. That was merely cleanup without any functionality changes. If we get uplift approval we should uplift only the first changeset.
Comment on attachment 8712149 [details] [diff] [review] 0001-Bug-1200341-Implement-HKDF-for-WebCrypto-r-rbarnes-s.patch, v2 Improves crypto, has test coverage, sounds good to me. Please uplift to aurora.
Attachment #8712149 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This also seems worth a release note. Tim can you suggest wording for one?
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 20•9 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19) > This also seems worth a release note. Tim can you suggest wording for one? Yes, thanks for the reminder! Release Note Request (optional, but appreciated) [Why is this notable]: Devs probably want to know they can now use HKDF in Firefox' WebCrypto implementation. [Suggested wording]: WebCrypto: HKDF support [Links (documentation, blog post, etc)]:
Updated•9 years ago
|
Keywords: dev-doc-needed
Added to release notes for 46 as suggested in comment 20. Once we have developer docs I can add a link. For now I will link to https://developer.mozilla.org/en-US/docs/Web/API/Web_Crypto_API if that sounds ok to you.
Wording changed slightly to: Added HKDF support for Web Crypto API
Assignee | ||
Comment 23•9 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21) > Added to release notes for 46 as suggested in comment 20. > > Once we have developer docs I can add a link. For now I will link to > https://developer.mozilla.org/en-US/docs/Web/API/Web_Crypto_API if that > sounds ok to you. Yes, that's good. Thanks for adding!
Updated•8 years ago
|
Whiteboard: [adv-main46-]
You need to log in
before you can comment on or make changes to this bug.
Description
•