crypto: harden WebCrypto jobs and promise handling#63363
Open
panva wants to merge 4 commits into
Open
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
lib / src
panva
added
crypto
Member
Author
|
First benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1847/ Second benchmark (same target): https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1848/ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #63363 +/- ##
==========================================
- Coverage 90.06% 90.03% -0.03%
==========================================
Files 714 714
Lines 225648 226074 +426
Branches 42711 42796 +85
==========================================
+ Hits 203237 203556 +319
- Misses 14200 14261 +61
- Partials 8211 8257 +46
🚀 New features to boost your workflow:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Member
Author
|
@Renegade334 you are incorrect in your assessments. The public methods still reject, not throw. Nothing is changed on the surface as evidenced by the lack of test changes and pre-existing passing comprehensive test cov including WPTs. |
Member
|
Oh good, code review hid webcrypto.js as a "large diff" 🤦♂️ I see the new machinery, apologies. (FWIW |
Member
Author
|
@Renegade334 i can do this across the board // WebCrypto methods return promises, including for synchronous validation
// failures. Keep that conversion in one place so method bodies stay readable.
function callSubtleCryptoMethod(fn, receiver, args) {
try {
const result = ReflectApply(fn, receiver, args);
// PromiseResolve(promise) reads promise.constructor. Avoid re-wrapping
// native promises so Promise.prototype.constructor pollution is ignored.
return isPromise(result) ? result : PromiseResolve(result);
} catch (err) {
return PromiseReject(err);
}
}That way PromiseResolve(result) is only called on what isn't a native job, which i now realize includes the key export jobs for which i can do a follow up to re-introduce a simpler native key export job now that everything goes through handles already, if key exports would go through jobs too then the only PromiseResolve that would be used would be for useless early returns. And it's fairly useless for PromiseReject. Oh and the composed methods (wrapKey, encapsulateKey, decapsulateKey)... I have think about those a bit. |
Member
|
Nice thought! Any significant impact on benchmarks? |
Member
Author
🤷♂️ we'll see when i do it |
Member
Author
|
Third benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1849/ wrt benchmark runs 1 and 2, the async results can't be trusted. |
panva
force-pushed
the
webcrypto-job-mode
branch
5 times, most recently
from
68a3a2b to
1d5f8f1
Compare
Member
Author
|
fix one find 2 more... the webcrypto job mode is a good start but this really shows we need all methods to end with a native promise to get rid of the js promise machinery. export, import, composite methods, ... 😭 |
Add a WebCrypto-specific CryptoJob mode that returns a promise from run() and resolves it when native work is finished. Encode job output directly as Web Crypto values, including CryptoKey instances and CryptoKeyPair dictionaries. Convert operation-specific setup failures from AdditionalConfig into OperationError rejections. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Remove async function wrappers from SubtleCrypto methods while keeping their public promise-returning behaviour. Route method entry points through a shared helper that converts synchronous validation errors into rejected promises. Let the internal implementations return native job promises directly. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Pass CryptoKey handles directly into KDF jobs instead of exporting secret bytes in lib. Normalize HKDF, PBKDF2, and Argon2 around the same job construction pattern so WebCrypto derivation paths avoid extra key material copies and keep operation failures in native job handling. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Avoid re-wrapping native WebCrypto promises with PromiseResolve(), since resolving a promise can read its user-mutated constructor. Add a helper for chaining internal WebCrypto job promises without consulting Promise species state, and use it for intermediate job results. Also align JWK wrapping and unwrapping with the spec's fresh-global JSON handling by detaching internal JWK values from user prototypes. Use the internal UTF-8 encoder/decoder bindings instead of shared TextEncoder/TextDecoder prototype methods. Expand the WebCrypto prototype pollution regression test to cover SubtleCrypto methods, export formats, zero-length KDF results, JWK toJSON/kty pollution, and encoder/decoder prototype poisoning. Signed-off-by: Filip Skokan <panva.ip@gmail.com>
Member
Author
|
rebased. |
Renegade334
reviewed
May 18, 2026
Comment on lines
+742
to
+746
| ObjectDefineProperty(promise, 'constructor', { | ||
| __proto__: null, | ||
| configurable: true, | ||
| value: undefined, | ||
| }); |
Member
There was a problem hiding this comment.
The only issue with these sorts of modifications is that they invalidate a V8 fast path, so presumably come with a performance hit:
Setting the value to be the Promise constructor, rather than undefined, does still allow for some short-circuiting AFAICT.
Member
Author
There was a problem hiding this comment.
I think undefined is the right tradeoff here.
Using constructor: Promise preserves the V8 fast path only while Promise[Symbol.species] is not polluted. Once Symbol.species is replaced, deleted, or turned into an accessor, constructor: Promise still reaches user-controlled state during Promise.prototype.then.
I’d rather keep the invariant simple. The extra cost is limited to the internal chained WebCrypto jobs, and avoids having two subtly different paths depending on ambient global state.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After quite the BoringSSL detour I'm coming back to Web Cryptography hardening related to #59699 (comment) and #59699 (comment) in hopes that we can eventually mark #59699 as resolved.
crypto: add WebCrypto CryptoJob mode
Add a WebCrypto-specific CryptoJob mode that returns a promise from run() and resolves it when native work is finished.
Encode job output directly as Web Crypto values, including CryptoKey instances and CryptoKeyPair dictionaries. Convert operation-specific setup failures from AdditionalConfig into OperationError rejections.
crypto: remove async from WebCrypto methods
Remove async function wrappers from SubtleCrypto methods while keeping their public promise-returning behaviour.
Route method entry points through a shared helper that converts synchronous validation errors into rejected promises. Let the internal implementations return native job promises directly.
crypto: pass CryptoKey handles to KDF jobs
Pass CryptoKey handles directly into KDF jobs instead of exporting secret bytes in lib.
Normalize HKDF, PBKDF2, and Argon2 around the same job construction pattern so WebCrypto derivation paths avoid extra key material copies and keep operation failures in native job handling.
crypto: harden WebCrypto against prototype pollution
Avoid re-wrapping native WebCrypto promises with PromiseResolve(), since resolving a promise can read its user-mutated constructor.
Add a helper for chaining internal WebCrypto job promises without consulting Promise species state, and use it for intermediate job results.
Also align JWK wrapping and unwrapping with the spec's fresh-global JSON handling by detaching internal JWK values from user prototypes. Use the internal UTF-8 encoder/decoder bindings instead of shared TextEncoder/TextDecoder prototype methods.
Expand the WebCrypto prototype pollution regression test to cover SubtleCrypto methods, export formats, zero-length KDF results, JWK toJSON/kty pollution, and encoder/decoder prototype poisoning.