Define the deriveBits length parameter as optional #30667

javifernandez · 2024-07-10T22:19:45Z

`9586d3d`

Define the deriveBits length parameter as optional
https://bugs.webkit.org/show_bug.cgi?id=276394

Reviewed by Youenn Fablet and Nitin Mahendru.

The PR#345 [1] to the WebCryptoAPI spec defines now the 'length'
parameter as optional, defaulting to 'null'. This change tries to
solve a long-standing interoperability issue in the deriveBits
operation.

This patch implements the required changes in the IDL so that the
'length' parameter is declared as optional, with 'null' as default
value when omitted. The affected algorithms (ECDH, HKDF, PBKDF2
and X25519) are adapted to the parameter's new type.

The PR#43400 [2] defined tests for the new behavior of the afected
algorithms, which they all pass now.

[1] w3c/webcrypto#345
[2] web-platform-tests/wpt#43400

* LayoutTests/crypto/subtle/derive-bits-malformed-parameters-expected.txt:
* LayoutTests/crypto/subtle/derive-bits-malformed-parameters.html:
* LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/derived_bits_length.https.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/derived_bits_length.https.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/interfaces/WebCryptoAPI.idl:
* Source/WebCore/crypto/CryptoAlgorithm.cpp:
(WebCore::CryptoAlgorithm::deriveBits):
* Source/WebCore/crypto/CryptoAlgorithm.h:
* Source/WebCore/crypto/SubtleCrypto.cpp:
(WebCore::SubtleCrypto::deriveKey):
(WebCore::SubtleCrypto::deriveBits):
* Source/WebCore/crypto/SubtleCrypto.h:
* Source/WebCore/crypto/SubtleCrypto.idl:
* Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.cpp:
(WebCore::CryptoAlgorithmECDH::deriveBits):
* Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.h:
* Source/WebCore/crypto/algorithms/CryptoAlgorithmHKDF.cpp:
(WebCore::CryptoAlgorithmHKDF::deriveBits):
* Source/WebCore/crypto/algorithms/CryptoAlgorithmHKDF.h:
* Source/WebCore/crypto/algorithms/CryptoAlgorithmPBKDF2.cpp:
(WebCore::CryptoAlgorithmPBKDF2::deriveBits):
* Source/WebCore/crypto/algorithms/CryptoAlgorithmPBKDF2.h:
* Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.cpp:
(WebCore::CryptoAlgorithmX25519::deriveBits):
* Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.h:

Canonical link: https://commits.webkit.org/281240@main

5db8323

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-07-10T22:19:53Z

EWS run on previous version of this PR (hash 86047c8)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2	❌ 🧪 wincairo-tests
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	❌ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	❌ 🧪 mac-AS-debug-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-07-11T12:08:50Z

EWS run on previous version of this PR (hash 7fd6e2a)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	❌ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-07-11T16:10:09Z

EWS run on previous version of this PR (hash b91c638)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

nmahendru · 2024-07-13T02:56:25Z

LayoutTests/crypto/subtle/ecdh-derive-bits-length-limits.html

@@ -85,7 +85,7 @@
    return Promise.resolve().then(function(result) {
        // P-256
        return Promise.all([
-            deriveBits(P256, 0, 256),


Nit: While you are at it, how about add:
deriveBits(P256, 0) also and such to all the cases so that they also get tested here.
I am aware that such tests are in wpt, but this will read better in itself.

Makes sense. As a matter of fact I've been thinking on porting those tests to WPT; anyway, this will be a follow up patch, so I'll add the new test cases here meanwhile.

Now that I think about it, it's unfortunate the deriveBits utility function defines the "expectedLength" as the last parameter. This makes more complex to add test cases for the "omitted" case.

Do you think it's worth to do a refactoring of the test because of this ?

nmahendru · 2024-07-13T02:57:26Z

Source/WebCore/crypto/algorithms/CryptoAlgorithmHKDF.cpp

 {
-    if (!length || length % 8) {
+    if (!length || !(*length) || *length % 8) {


:rant
This thing is super onfortunate :D
:end-rant

yeah, I'd rather have *length != 0 but it seems the style-checker complains about that.

nmahendru

Looks good to me.

youennf

LGTM.
Could you beef up the commit message and explain why it is ok to change length=0 behavior?
AIUI from the GitHub issue, usage is very low so this is likely ok but it would be good that you confirm this.
And maybe the length=0 change should be done in a separate PR in case of regression.

youennf · 2024-07-22T08:21:29Z

LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.any-expected.txt

@@ -49,7 +49,7 @@ PASS SubtleCrypto interface: operation verify(AlgorithmIdentifier, CryptoKey, Bu
 PASS SubtleCrypto interface: operation digest(AlgorithmIdentifier, BufferSource)
 PASS SubtleCrypto interface: operation generateKey(AlgorithmIdentifier, boolean, sequence<KeyUsage>)
 PASS SubtleCrypto interface: operation deriveKey(AlgorithmIdentifier, CryptoKey, AlgorithmIdentifier, boolean, sequence<KeyUsage>)
-PASS SubtleCrypto interface: operation deriveBits(AlgorithmIdentifier, CryptoKey, unsigned long)
+FAIL SubtleCrypto interface: operation deriveBits(AlgorithmIdentifier, CryptoKey, unsigned long) assert_equals: property has wrong .length expected 3 but got 2


Can we resync that test?

youennf · 2024-07-22T08:26:23Z

.../web-platform-tests/WebCryptoAPI/derive_bits_keys/derived_bits_length.https.any-expected.txt

 PASS X25519 derivation with 256 as 'length' parameter
-FAIL X25519 derivation with 0 as 'length' parameter assert_array_equals: Derived bits do not match the expected result. lengths differ, expected array object "" length 0, got object "63,245,136,2,149,247,97,118,8,143,137,228,61,254,190,126,161,149,0,8" length 32
+PASS X25519 derivation with 0 as 'length' parameter


I think it is probably ok to both make the parameter optional and change the behavior for length 0.
But maybe it would be a bit safer to split in two.
@javifernandez, how confident are you with this change of behavior?

I think there is consensus in the PR about pursuing the same behavior for the four algorithms when length=0 is used, so that it returns an empty string. However, I think there is a bit of a mess in terms of interoperability regarding this operation:

https://wpt.fyi/results/WebCryptoAPI/derive_bits_keys/derived_bits_length.https.any.html?label=master&label=experimental

Chrome already returns an empty string for X25519, while WebKit doesn´t. However, Chrome doesn't do the same with HKDF (although it does for PBKDF2), while WebKit does.

Chrome also asked to do the length=0 change, for its affected algorithms, in a separate change, so I'll do the same for WebKit.

webkit-early-warning-system · 2024-07-22T14:10:14Z

EWS run on previous version of this PR (hash 3d5c213)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-07-22T21:40:44Z

EWS run on current version of this PR (hash 5db8323)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 wincairo-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

javifernandez · 2024-07-23T11:16:46Z

The failures in the gtk-wk2 bot seems unrelated to this change. So I'll try the merge queue now.

https://bugs.webkit.org/show_bug.cgi?id=276394 Reviewed by Youenn Fablet and Nitin Mahendru. The PR#345 [1] to the WebCryptoAPI spec defines now the 'length' parameter as optional, defaulting to 'null'. This change tries to solve a long-standing interoperability issue in the deriveBits operation. This patch implements the required changes in the IDL so that the 'length' parameter is declared as optional, with 'null' as default value when omitted. The affected algorithms (ECDH, HKDF, PBKDF2 and X25519) are adapted to the parameter's new type. The PR#43400 [2] defined tests for the new behavior of the afected algorithms, which they all pass now. [1] w3c/webcrypto#345 [2] web-platform-tests/wpt#43400 * LayoutTests/crypto/subtle/derive-bits-malformed-parameters-expected.txt: * LayoutTests/crypto/subtle/derive-bits-malformed-parameters.html: * LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/derived_bits_length.https.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/derived_bits_length.https.any.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/WebCryptoAPI/idlharness.https.any.worker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/interfaces/WebCryptoAPI.idl: * Source/WebCore/crypto/CryptoAlgorithm.cpp: (WebCore::CryptoAlgorithm::deriveBits): * Source/WebCore/crypto/CryptoAlgorithm.h: * Source/WebCore/crypto/SubtleCrypto.cpp: (WebCore::SubtleCrypto::deriveKey): (WebCore::SubtleCrypto::deriveBits): * Source/WebCore/crypto/SubtleCrypto.h: * Source/WebCore/crypto/SubtleCrypto.idl: * Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.cpp: (WebCore::CryptoAlgorithmECDH::deriveBits): * Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.h: * Source/WebCore/crypto/algorithms/CryptoAlgorithmHKDF.cpp: (WebCore::CryptoAlgorithmHKDF::deriveBits): * Source/WebCore/crypto/algorithms/CryptoAlgorithmHKDF.h: * Source/WebCore/crypto/algorithms/CryptoAlgorithmPBKDF2.cpp: (WebCore::CryptoAlgorithmPBKDF2::deriveBits): * Source/WebCore/crypto/algorithms/CryptoAlgorithmPBKDF2.h: * Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.cpp: (WebCore::CryptoAlgorithmX25519::deriveBits): * Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.h: Canonical link: https://commits.webkit.org/281240@main

webkit-commit-queue · 2024-07-23T11:24:02Z

Committed 281240@main (9586d3d): https://commits.webkit.org/281240@main

Reviewed commits have been landed. Closing PR #30667 and removing active labels.

javifernandez requested a review from zdobersek as a code owner July 10, 2024 22:19

javifernandez self-assigned this Jul 10, 2024

javifernandez added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Jul 10, 2024

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 10, 2024

javifernandez marked this pull request as draft July 11, 2024 11:14

javifernandez removed the merging-blocked Applied to prevent a change from being merged label Jul 11, 2024

javifernandez force-pushed the deriveBits-length branch from 86047c8 to 7fd6e2a Compare July 11, 2024 12:08

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 11, 2024

javifernandez requested a review from youennf July 11, 2024 15:57

javifernandez marked this pull request as ready for review July 11, 2024 15:58

javifernandez removed the merging-blocked Applied to prevent a change from being merged label Jul 11, 2024

javifernandez force-pushed the deriveBits-length branch from 7fd6e2a to b91c638 Compare July 11, 2024 16:10

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 11, 2024

aproskuryakov requested a review from nmahendru July 12, 2024 00:24

nmahendru reviewed Jul 13, 2024

View reviewed changes

nmahendru approved these changes Jul 13, 2024

View reviewed changes

youennf approved these changes Jul 22, 2024

View reviewed changes

javifernandez removed the merging-blocked Applied to prevent a change from being merged label Jul 22, 2024

javifernandez force-pushed the deriveBits-length branch from b91c638 to 3d5c213 Compare July 22, 2024 14:10

javifernandez force-pushed the deriveBits-length branch from 3d5c213 to 5db8323 Compare July 22, 2024 21:40

javifernandez added the merge-queue Applied to send a pull request to merge-queue label Jul 23, 2024

webkit-commit-queue force-pushed the deriveBits-length branch from 5db8323 to 9586d3d Compare July 23, 2024 11:23

webkit-commit-queue merged commit 9586d3d into WebKit:main Jul 23, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define the deriveBits length parameter as optional #30667

Define the deriveBits length parameter as optional #30667

javifernandez commented Jul 10, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jul 10, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 11, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 11, 2024 •

edited

Loading

nmahendru Jul 13, 2024

javifernandez Jul 15, 2024

javifernandez Jul 22, 2024

nmahendru Jul 13, 2024

javifernandez Jul 15, 2024

nmahendru left a comment

youennf left a comment

youennf Jul 22, 2024

javifernandez Jul 22, 2024

youennf Jul 22, 2024

javifernandez Jul 22, 2024

webkit-early-warning-system commented Jul 22, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 22, 2024 •

edited

Loading

javifernandez commented Jul 23, 2024

webkit-commit-queue commented Jul 23, 2024

Define the deriveBits length parameter as optional #30667

Define the deriveBits length parameter as optional #30667

Conversation

javifernandez commented Jul 10, 2024 • edited by webkit-early-warning-system Loading

webkit-early-warning-system commented Jul 10, 2024 • edited Loading

webkit-early-warning-system commented Jul 11, 2024 • edited Loading

webkit-early-warning-system commented Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmahendru left a comment

Choose a reason for hiding this comment

youennf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jul 22, 2024 • edited Loading

webkit-early-warning-system commented Jul 22, 2024 • edited Loading

javifernandez commented Jul 23, 2024

webkit-commit-queue commented Jul 23, 2024

javifernandez commented Jul 10, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jul 10, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 11, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 11, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 22, 2024 •

edited

Loading

webkit-early-warning-system commented Jul 22, 2024 •

edited

Loading