1200341 - Implement HKDF for WebCrypto

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[RenThraysk]

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.

Comment 1

[RenThraysk]

So it seems cannot derive a PBKDF2 key from a ECDH shared secret. So there are no usable KDFs with DH/ECDH key exchanges.

Comment 2

[RenThraysk]

Attachment: Attempt to derive a PBKDF2 key from a ECDH exchange

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

The CONCAT KDF was removed from the spec.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Note: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27425

HKDF-CTR will be renamed to HKDF and stick to RFC 5869.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1200341-Implement-HKDF-for-WebCrypto.patch

I fixed a few things left and right but this should work and have a good test coverage.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(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

[Richard Barnes [:rbarnes]]

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?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(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 9

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1200341-Implement-HKDF-for-WebCrypto-r-rbarnes-s.patch, v2

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1200341-Use-BufferSource-instead-of-CryptoOperat.patch

s/CryptoOperationData/BufferSource r=smaug (over IRC)

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e347384b8db
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b255b3ff8d3

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e347384b8db
https://hg.mozilla.org/mozilla-central/rev/8b255b3ff8d3

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 16

[Daniel Holbert [:dholbert]]

(We may need to sort out bug 1243438 -- a build failure in disable-eme builds -- before we uplift this to aurora.)

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(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 18

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

This also seems worth a release note. Tim can you suggest wording for one?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(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)]:

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Wording changed slightly to: Added HKDF support for Web Crypto API

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

(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!