The ECDH and X25519 deriveBits returns an empty string when 'length' is 0 #31146

javifernandez · 2024-07-24T10:40:08Z

`0b46039`

The ECDH and X25519 deriveBits returns an empty string when 'length' is 0
https://bugs.webkit.org/show_bug.cgi?id=276916

Reviewed by Matthew Finkel.

The WebCrypto API spec's draft states that the ECDH's deriveBits should
handle a zero length as any regular number, only throwing an exception
in case of 'null'. The same is stated in the Secure Curves specification
draft for the X25519 algorithm.

We were not supporting 'null' value before, but since r281240 the 'length'
parameter is defined as optional, with 'null' as default value. Hence
in case of a zero length the derived bits are truncated so that the
operation returns an empty string.

* LayoutTests/crypto/subtle/ecdh-derive-bits-length-limits-expected.txt:
* LayoutTests/crypto/subtle/ecdh-derive-bits-length-limits.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:
* Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.cpp:
(WebCore::CryptoAlgorithmECDH::deriveBits):
* Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.cpp:
(WebCore::CryptoAlgorithmX25519::deriveBits):

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

7c8d375

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-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	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-07-24T10:40:16Z

EWS run on previous version of this PR (hash 79ccd0e)

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-24T21:29:51Z

EWS run on previous version of this PR (hash 9af3f2c)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-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	⏳ 🧪 mac-intel-wk2
	✅ 🛠 tv
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-10-03T16:34:51Z

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

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-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	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

nmahendru · 2024-10-11T06:05:28Z

Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.cpp

This technically does work. But I think it would be wise to check for null as the first thing in the function ::deriveBits and bail early.
For Zero, I think we can check right before this lambda and don't even get into doing scalar multiplication and just return a zero length result.

I agree on the optimization for the zero length, which would even allow us to avoid the call to the platformDeriveBits key.

However I'm not so sure about the early bail in case of null; since the change to define length as optional, null means that we should return the full derivedKey. Hence, I don't think we save much to avoid doing the check inside the lambda.

Also, not a strong opinion, but I think it's better to do the length = 0 after the rest of the basic checks (eg. ec parameters, key type, algorithm name). Otherwise we would hide failures in those checks when passing 0 as length.

Otherwise we would hide failures in those checks when passing 0 as length.

It's true vice versa. If we were adding those other checks in this PR instead of zero length, do you think the same argument applies ? I might be missing stuff in the spec. I had a quick look.

optional, null means that we should return the full derivedKey.

I did not know that. Then I guess we need to derive the key.

Otherwise we would hide failures in those checks when passing 0 as length.

It's true vice versa. If we were adding those other checks in this PR instead of zero length, do you think the same argument applies ? I might be missing stuff in the spec. I had a quick look.

Very true; my preference is based on the fact that the 'length' behavior is based on the specific algorithm ; for instance, PBKDF2 and HKDF would throw in case of 0, and we are aiming for ECDH and X25519 to throw on any truncation. So in my mind it makes more sense to ensure first that the algorithm name, key type and usages are validates and only then check the specific values of the 'length' parameter.

But as I said, I don't have a strong opinion on this, because you have a valid point and perhaps it's more efficient to early return based on an "cheap" check of the length's value, avoiding the other checks.

nmahendru

It does work technically but with the spec change the code can be easily made a little better at failing or returning fast.

webkit-early-warning-system · 2024-10-11T10:34:59Z

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

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-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	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.cpp

nmahendru · 2024-10-11T22:12:50Z

Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.cpp

super nit:

Suggested change

if (length && !(*length)) {

if (!length.value_or(0)) {

I think this is not correct; we would return an empty string in case "null". The spec states:

If length is null:
Return secret

ohh. right.. then what you have is perfect.
To re-state, Truncate only if length is explicitly provided!

nmahendru

Thanks for all the work on this PR and the spec!

webkit-early-warning-system · 2024-10-14T09:57:44Z

EWS run on current version of this PR (hash 7c8d375)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	✅ 🧪 win-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	✅ 🧪 mac-intel-wk2
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-commit-queue · 2024-10-14T10:00:02Z

Commit message contains (OOPS!) and Nitin Mahendru is not a reviewer, blocking PR #31146. Details: Build #15819

sysrqb · 2024-10-17T15:09:24Z

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

Is this test cases for 256 bits, and similar below, not valuable anymore? How does that relate to the 0-length check?

Is this test cases for 256 bits, and similar below, not valuable anymore? How does that relate to the 0-length check?

The test case checked that when passing 0 as 'length', the result was the full string; now we should return an empty string instead.

sysrqb

Changes lgtm.

…is 0 https://bugs.webkit.org/show_bug.cgi?id=276916 Reviewed by Matthew Finkel. The WebCrypto API spec's draft states that the ECDH's deriveBits should handle a zero length as any regular number, only throwing an exception in case of 'null'. The same is stated in the Secure Curves specification draft for the X25519 algorithm. We were not supporting 'null' value before, but since r281240 the 'length' parameter is defined as optional, with 'null' as default value. Hence in case of a zero length the derived bits are truncated so that the operation returns an empty string. * LayoutTests/crypto/subtle/ecdh-derive-bits-length-limits-expected.txt: * LayoutTests/crypto/subtle/ecdh-derive-bits-length-limits.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: * Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.cpp: (WebCore::CryptoAlgorithmECDH::deriveBits): * Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.cpp: (WebCore::CryptoAlgorithmX25519::deriveBits): Canonical link: https://commits.webkit.org/285383@main

webkit-commit-queue · 2024-10-18T08:03:19Z

Committed 285383@main (0b46039): https://commits.webkit.org/285383@main

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

javifernandez requested a review from zdobersek as a code owner July 24, 2024 10:40

javifernandez self-assigned this Jul 24, 2024

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

javifernandez marked this pull request as draft July 24, 2024 10:53

javifernandez force-pushed the deriveBits-zero-length branch from 79ccd0e to 9af3f2c Compare July 24, 2024 21:29

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

annevk requested a review from nmahendru October 2, 2024 06:18

javifernandez requested review from nt1m and removed request for nmahendru October 3, 2024 16:31

javifernandez marked this pull request as ready for review October 3, 2024 16:32

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

javifernandez force-pushed the deriveBits-zero-length branch from 9af3f2c to a66c770 Compare October 3, 2024 16:34

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

javifernandez requested a review from nmahendru October 3, 2024 22:00

nmahendru reviewed Oct 11, 2024

View reviewed changes

nmahendru requested changes Oct 11, 2024

View reviewed changes

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

javifernandez force-pushed the deriveBits-zero-length branch from a66c770 to e6ce092 Compare October 11, 2024 10:34

javifernandez requested a review from nmahendru October 11, 2024 12:17

nmahendru reviewed Oct 11, 2024

View reviewed changes

Source/WebCore/crypto/algorithms/CryptoAlgorithmX25519.cpp Outdated Show resolved Hide resolved

nmahendru reviewed Oct 11, 2024

View reviewed changes

nmahendru approved these changes Oct 11, 2024

View reviewed changes

javifernandez force-pushed the deriveBits-zero-length branch from e6ce092 to 7c8d375 Compare October 14, 2024 09:57

javifernandez added the merge-queue Applied to send a pull request to merge-queue label Oct 14, 2024

webkit-commit-queue added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Oct 14, 2024

javifernandez requested a review from youennf October 14, 2024 15:14

sysrqb reviewed Oct 17, 2024

View reviewed changes

sysrqb approved these changes Oct 17, 2024

View reviewed changes

javifernandez added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Oct 18, 2024

webkit-commit-queue force-pushed the deriveBits-zero-length branch from 7c8d375 to 0b46039 Compare October 18, 2024 08:03

webkit-commit-queue merged commit 0b46039 into WebKit:main Oct 18, 2024

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

The ECDH and X25519 deriveBits returns an empty string when 'length' is 0 #31146

The ECDH and X25519 deriveBits returns an empty string when 'length' is 0 #31146

Uh oh!

Conversation

javifernandez commented Jul 24, 2024 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Oct 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmahendru Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmahendru left a comment

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmahendru left a comment

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Oct 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sysrqb left a comment

Choose a reason for hiding this comment

Uh oh!

webkit-commit-queue commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

javifernandez commented Jul 24, 2024 •

edited by webkit-early-warning-system

Loading

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

edited

Loading

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

edited

Loading

webkit-early-warning-system commented Oct 3, 2024 •

edited

Loading

nmahendru Oct 11, 2024 •

edited

Loading

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

edited

Loading

webkit-early-warning-system commented Oct 14, 2024 •

edited

Loading