Implement HKDF for WebCrypto

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: renthraysk, Assigned: ttaubert)

Tracking

(Blocks 1 bug, {dev-doc-needed, html5, wsec-crypto})

44 Branch
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed, relnote-firefox 46+)

Details

(Whiteboard: [adv-main46-])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

4 years ago
Keywords: html5
OS: Unspecified → All
Hardware: Unspecified → All
(Reporter)

Comment 1

4 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)

Updated

4 years ago
Keywords: wsec-crypto
Version: 43 Branch → 44 Branch
Component: Untriaged → DOM: Security
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
The CONCAT KDF was removed from the spec.
Blocks: web-crypto
Summary: Implement the KDFs in WebCrypto spec → Implement HKDF-CTR for WebCrypto
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

3 years ago
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
I fixed a few things left and right but this should work and have a good test coverage.
Attachment #8711708 - Flags: review?(rlb)
(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 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+
(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.
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)
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 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+
s/CryptoOperationData/BufferSource r=smaug (over IRC)
Attachment #8712179 - Flags: review+

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e347384b8db
https://hg.mozilla.org/mozilla-central/rev/8b255b3ff8d3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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?
Depends on: 1243438
(We may need to sort out bug 1243438 -- a build failure in disable-eme builds -- before we uplift this to aurora.)
(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)
(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)]:
relnote-firefox: --- → ?
Flags: needinfo?(ttaubert)
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
(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!
Whiteboard: [adv-main46-]
You need to log in before you can comment on or make changes to this bug.