-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Update BoringSSL, add SHA3 to WebCrypto and node:crypto #29323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e6ca20c
14bef08
8ce301b
62a8a4d
22a55d1
8d39b78
dc79dd8
4514cea
4be6752
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,6 +125,10 @@ void CryptoAlgorithmHMAC::importKey(CryptoKeyFormat format, KeyData&& data, cons | |
| return alg.isNull() || alg == ALG384; | ||
| case CryptoAlgorithmIdentifier::SHA_512: | ||
| return alg.isNull() || alg == ALG512; | ||
| case CryptoAlgorithmIdentifier::SHA3_256: | ||
| case CryptoAlgorithmIdentifier::SHA3_384: | ||
| case CryptoAlgorithmIdentifier::SHA3_512: | ||
| return alg.isNull(); | ||
| default: | ||
| return false; | ||
| } | ||
|
Comment on lines
125
to
134
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 When exporting an HMAC-SHA3-256/384/512 key as JWK, no alg field is emitted (the switch in exportKey falls through with a bare break at lines 182-188 of CryptoAlgorithmHMAC.cpp), and the import checkAlgCallback at lines 125-131 accepts any SHA3 JWK whenever alg is null — so a SHA3-256 key exported to JWK can be silently re-imported as SHA3-512. The JWK roundtrip is lossy with respect to the hash variant; consider throwing NotSupportedError for SHA3 HMAC JWK export/import, or documenting that JWK does not preserve the SHA3 hash variant. Extended reasoning...What the bug is and how it manifests When exportKey('jwk', hmacSha3Key) is called, CryptoAlgorithmHMAC.cpp lines 182-188 match all three SHA3 variants and execute a bare break without setting jwk.alg. This is intentional — no IANA-registered JWK alg identifiers exist for HMAC-SHA3. The exported JWK therefore looks like {kty:'oct', k:'...'} with no alg field for any SHA3 variant. The specific code path that triggers it During importKey('jwk', jwkData, {name:'HMAC', hash:'SHA3-512'}), the code reaches CryptoKeyHMAC::importJwk which calls the checkAlgCallback lambda at lines 125-131. For all three SHA3 identifiers the callback returns alg.isNull(). Since the exported JWK never sets alg regardless of which SHA3 variant was used, alg.isNull() is always true — the check passes silently. The import then proceeds with SHA3_512 as the hash but with the key material of the original SHA3-256 key. Why existing code does not prevent it For SHA-2 HMAC, exportKey always sets jwk.alg to a specific string (HS256, HS384, etc.), and the import callback verifies alg == 'HS512' (for example). Cross-variant confusion is rejected at import time. For SHA3, no analogous IANA-registered alg string exists, so the export produces no alg field and the import check is reduced to the vacuous alg.isNull() condition — which is true for all SHA3 export JWKs and therefore cannot distinguish between variants. What the impact would be The JWK roundtrip does not preserve the SHA3 hash variant. A developer who exports a SHA3-256 HMAC key, transmits the JWK, and re-imports it with hash:'SHA3-512' will get an HMAC-SHA3-512 key backed by 32-byte key material without any error. HMAC values computed with the original key will not match those from the re-imported key, silently breaking correctness guarantees. Step-by-step proof
How to fix it Option A (clean): Return NotSupportedError from exportKey and importKey for SHA3 HMAC in JWK format, making the limitation explicit and consistent with the existing rejection of SHA3 as a standalone importKey algorithm in SubtleCrypto.cpp lines 381-387. Option B (best-effort): Emit a non-standard alg string (e.g. HS3-256) and verify it in the import callback, preventing cross-variant confusion at the cost of interoperability with other implementations that follow the JWK spec strictly.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches Node.js's webcrypto behavior: > const k = await crypto.subtle.generateKey({name:"HMAC",hash:"SHA3-256",length:256},true,["sign"])
> (await crypto.subtle.exportKey("jwk", k)).alg
undefinedThere's no IETF-registered JWK |
||
|
|
@@ -178,6 +182,10 @@ void CryptoAlgorithmHMAC::exportKey(CryptoKeyFormat format, Ref<CryptoKey>&& key | |
| case CryptoAlgorithmIdentifier::SHA_512: | ||
| jwk.alg = String(ALG512); | ||
| break; | ||
| case CryptoAlgorithmIdentifier::SHA3_256: | ||
| case CryptoAlgorithmIdentifier::SHA3_384: | ||
| case CryptoAlgorithmIdentifier::SHA3_512: | ||
| break; | ||
| default: | ||
| ASSERT_NOT_REACHED(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| /* | ||
| * Copyright (C) 2024 Apple Inc. All rights reserved. | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions | ||
| * are met: | ||
| * 1. Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * 2. Redistributions in binary form must reproduce the above copyright | ||
| * notice, this list of conditions and the following disclaimer in the | ||
| * documentation and/or other materials provided with the distribution. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' | ||
| * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, | ||
| * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR | ||
| * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS | ||
| * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR | ||
| * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF | ||
| * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS | ||
| * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN | ||
| * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) | ||
| * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF | ||
| * THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
||
| #include "config.h" | ||
| #include "CryptoAlgorithmSHA3.h" | ||
|
|
||
| #if ENABLE(WEB_CRYPTO) | ||
|
|
||
| #include "CryptoDigest.h" | ||
| #include "ScriptExecutionContext.h" | ||
|
|
||
| namespace WebCore { | ||
|
|
||
| static void dispatchDigest(PAL::CryptoDigest::Algorithm algorithm, | ||
| Vector<uint8_t>&& message, CryptoAlgorithm::VectorCallback&& callback, | ||
| CryptoAlgorithm::ExceptionCallback&& exceptionCallback, | ||
| ScriptExecutionContext& context, WorkQueue& workQueue) | ||
| { | ||
| auto digest = PAL::CryptoDigest::create(algorithm); | ||
| if (!digest) { | ||
| exceptionCallback(OperationError, ""_s); | ||
| return; | ||
| } | ||
|
|
||
| if (message.size() < 64) { | ||
| auto moved = WTF::move(message); | ||
| digest->addBytes(moved.begin(), moved.size()); | ||
| auto result = digest->computeHash(); | ||
| ScriptExecutionContext::postTaskTo(context.identifier(), | ||
| [callback = WTF::move(callback), result = WTF::move(result)](auto&) { | ||
| callback(result); | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| workQueue.dispatch(context.globalObject(), | ||
| [digest = WTF::move(digest), message = WTF::move(message), | ||
| callback = WTF::move(callback), | ||
| contextIdentifier = context.identifier()]() mutable { | ||
| digest->addBytes(message.begin(), message.size()); | ||
| auto result = digest->computeHash(); | ||
| ScriptExecutionContext::postTaskTo(contextIdentifier, | ||
| [callback = WTF::move(callback), result = WTF::move(result)](auto&) { | ||
| callback(result); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| #define DEFINE_SHA3(ClassName, DigestAlgo) \ | ||
| Ref<CryptoAlgorithm> ClassName::create() { return adoptRef(*new ClassName); } \ | ||
| CryptoAlgorithmIdentifier ClassName::identifier() const { return s_identifier; } \ | ||
| void ClassName::digest(Vector<uint8_t>&& message, VectorCallback&& callback, \ | ||
| ExceptionCallback&& exceptionCallback, ScriptExecutionContext& context, \ | ||
| WorkQueue& workQueue) \ | ||
| { \ | ||
| dispatchDigest(DigestAlgo, WTF::move(message), WTF::move(callback), \ | ||
| WTF::move(exceptionCallback), context, workQueue); \ | ||
| } | ||
|
|
||
| DEFINE_SHA3(CryptoAlgorithmSHA3_256, PAL::CryptoDigest::Algorithm::SHA3_256) | ||
| DEFINE_SHA3(CryptoAlgorithmSHA3_384, PAL::CryptoDigest::Algorithm::SHA3_384) | ||
| DEFINE_SHA3(CryptoAlgorithmSHA3_512, PAL::CryptoDigest::Algorithm::SHA3_512) | ||
|
|
||
| #undef DEFINE_SHA3 | ||
|
|
||
| } // namespace WebCore | ||
|
|
||
| #endif // ENABLE(WEB_CRYPTO) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /* | ||
| * Copyright (C) 2024 Apple Inc. All rights reserved. | ||
| * | ||
| * Redistribution and use in source and binary forms, with or without | ||
| * modification, are permitted provided that the following conditions | ||
| * are met: | ||
| * 1. Redistributions of source code must retain the above copyright | ||
| * notice, this list of conditions and the following disclaimer. | ||
| * 2. Redistributions in binary form must reproduce the above copyright | ||
| * notice, this list of conditions and the following disclaimer in the | ||
| * documentation and/or other materials provided with the distribution. | ||
| * | ||
| * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' | ||
| * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, | ||
| * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR | ||
| * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS | ||
| * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR | ||
| * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF | ||
| * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS | ||
| * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN | ||
| * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) | ||
| * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF | ||
| * THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
||
| #pragma once | ||
|
|
||
| #if ENABLE(WEB_CRYPTO) | ||
|
|
||
| #include "CryptoAlgorithm.h" | ||
|
|
||
| namespace WebCore { | ||
|
|
||
| class CryptoAlgorithmSHA3_256 final : public CryptoAlgorithm { | ||
| public: | ||
| static constexpr ASCIILiteral s_name = "SHA3-256"_s; | ||
| static constexpr CryptoAlgorithmIdentifier s_identifier = CryptoAlgorithmIdentifier::SHA3_256; | ||
| static Ref<CryptoAlgorithm> create(); | ||
|
|
||
| private: | ||
| CryptoAlgorithmSHA3_256() = default; | ||
| CryptoAlgorithmIdentifier identifier() const final; | ||
| void digest(Vector<uint8_t>&&, VectorCallback&&, ExceptionCallback&&, ScriptExecutionContext&, WorkQueue&) final; | ||
| }; | ||
|
|
||
| class CryptoAlgorithmSHA3_384 final : public CryptoAlgorithm { | ||
| public: | ||
| static constexpr ASCIILiteral s_name = "SHA3-384"_s; | ||
| static constexpr CryptoAlgorithmIdentifier s_identifier = CryptoAlgorithmIdentifier::SHA3_384; | ||
| static Ref<CryptoAlgorithm> create(); | ||
|
|
||
| private: | ||
| CryptoAlgorithmSHA3_384() = default; | ||
| CryptoAlgorithmIdentifier identifier() const final; | ||
| void digest(Vector<uint8_t>&&, VectorCallback&&, ExceptionCallback&&, ScriptExecutionContext&, WorkQueue&) final; | ||
| }; | ||
|
|
||
| class CryptoAlgorithmSHA3_512 final : public CryptoAlgorithm { | ||
| public: | ||
| static constexpr ASCIILiteral s_name = "SHA3-512"_s; | ||
| static constexpr CryptoAlgorithmIdentifier s_identifier = CryptoAlgorithmIdentifier::SHA3_512; | ||
| static Ref<CryptoAlgorithm> create(); | ||
|
|
||
| private: | ||
| CryptoAlgorithmSHA3_512() = default; | ||
| CryptoAlgorithmIdentifier identifier() const final; | ||
| void digest(Vector<uint8_t>&&, VectorCallback&&, ExceptionCallback&&, ScriptExecutionContext&, WorkQueue&) final; | ||
| }; | ||
|
|
||
| } // namespace WebCore | ||
|
|
||
| #endif // ENABLE(WEB_CRYPTO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 Calling
pbkdf2Sync('pw', 'salt', 1000, 32, 'blake2s256')panics the Zig runtime becauseblake2s256passes the PBKDF2 blocklist check (only.shake128and.shake256are blocked) butEVP.Algorithm.md()returnsnullfor it — andPBKDF2.run()force-unwraps that null via.?at line 25. This is a pre-existing bug (blake2s256 was never blocked before this PR either), but this PR modifies the exact switch statement that should also guard it.Extended reasoning...
What the bug is and how it manifests
blake2s256is a valid entry inEVP.Algorithm.map(EVP.zigline ~77), soPBKDF2.fromJSresolves it to the.blake2s256algorithm variant and proceeds past the invalid-algorithm gate. However,EVP.Algorithm.md()only handles.blake2b256and.blake2b512in its explicit match arms;.blake2s256falls through to theelse => nullbranch and returnsnull.PBKDF2.run()then callsalgorithm.md().?(line 25), which force-unwraps an optional null — a Zig runtime panic that crashes the entire process.The specific code path that triggers it
pbkdf2Sync('pw', 'salt', 1000, 32, 'blake2s256')callsPBKDF2.fromJS.EVP.Algorithm.map.fromJSCaseInsensitiveresolves'blake2s256'to.blake2s256..shake128and.shake256;.blake2s256falls to theelse => |alg| break :brk algarm and is accepted.PBKDF2.run()callsalgorithm.md().?—md()returnsnullfor.blake2s256— Zig panics.Why existing code doesn't prevent it
Before this PR,
sha3variants were also listed in the blocklist. The PR correctly removed them (sincesha3now has real EVP implementations). However,blake2s256was also never blocked, and the modification to this switch statement is the exact right place to also block it.EVP.Algorithm.md()has noblake2s256case because BoringSSL does not expose anEVP_blake2s256()function (onlyEVP_blake2b256/EVP_blake2b512exist in BoringSSL's public API).Impact
Any call to Node's
pbkdf2/pbkdf2Syncwith digest'blake2s256'will terminate the process with a Zig panic. This is a process-crashing denial-of-service if user-supplied digest names flow into PBKDF2.How to fix it
Add
.blake2s256to thebreak :invalidlist inPBKDF2.fromJS:Alternatively, change
PBKDF2.run()to handle a nullmd()gracefully instead of force-unwrapping.Step-by-step proof
EVP.Algorithm.mapincludes"blake2s256" => .blake2s256— the name resolves successfully.EVP.Algorithm.md()match arms:.blake2b256,.blake2b512,.md4, ...,.@"sha3-512"— no.blake2s256case; hitselse => null.PBKDF2.fromJSblocklist:.shake128, .shake256 => break :invalid;.blake2s256is not listed, falls toelse => |alg| break :brk alg.PBKDF2.run()line 25:algorithm.md().?force-unwrapsnull→ Zig runtime panic, process terminates.