crypto: harden WebCrypto jobs and promise handling#63363
Open
panva wants to merge 4 commits into
Open
Conversation
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>
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.04% -0.02%
==========================================
Files 714 714
Lines 225502 225989 +487
Branches 42628 42760 +132
==========================================
+ Hits 203089 203496 +407
- Misses 14195 14231 +36
- Partials 8218 8262 +44
🚀 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
2 times, most recently
from
e9ecc87 to
6f9aedc
Compare
panva
force-pushed
the
webcrypto-job-mode
branch
2 times, most recently
from
8aad2b1 to
68a3a2b
Compare
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
|
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, ... 😭 |
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.