Skip to content

Commit ae61cd6

Browse files
panvasxa
panva
authored and
sxa
committed
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. Signed-off-by: Filip Skokan <panva.ip@gmail.com> PR-URL: #63363 Backport-PR-URL: #63563 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
1 parent 3d05a1d commit ae61cd6

13 files changed

Lines changed: 985 additions & 155 deletions

‎lib/internal/crypto/diffiehellman.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ const {
55
FunctionPrototypeCall,
66
MathCeil,
77
ObjectDefineProperty,
8-
PromisePrototypeThen,
98
SafeSet,
109
TypedArrayPrototypeGetBuffer,
1110
Uint8Array,
@@ -65,6 +64,7 @@ const {
6564
const {
6665
getArrayBufferOrView,
6766
jobPromise,
67+
jobPromiseThen,
6868
toBuf,
6969
kHandle,
7070
} = require('internal/crypto/util');
@@ -405,7 +405,7 @@ function ecdhDeriveBits(algorithm, baseKey, length) {
405405
if (length === null)
406406
return bits;
407407

408-
return PromisePrototypeThen(bits, (bits) => {
408+
return jobPromiseThen(bits, (bits) => {
409409
// If the length is not a multiple of 8 the nearest ceiled
410410
// multiple of 8 is sliced.
411411
const sliceLength = MathCeil(length / 8);

‎lib/internal/crypto/util.js‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@ const {
1414
ObjectEntries,
1515
ObjectKeys,
1616
ObjectPrototypeHasOwnProperty,
17+
PromisePrototypeThen,
1718
PromiseReject,
19+
PromiseWithResolvers,
1820
SafeSet,
1921
StringPrototypeToUpperCase,
2022
Symbol,
@@ -76,6 +78,7 @@ const {
7678
emitExperimentalWarning,
7779
filterDuplicateStrings,
7880
lazyDOMException,
81+
setOwnProperty,
7982
} = require('internal/util');
8083

8184
const {
@@ -89,6 +92,7 @@ const {
8992
isDataView,
9093
isArrayBufferView,
9194
isAnyArrayBuffer,
95+
isPromise,
9296
} = require('internal/util/types');
9397

9498
const kHandle = Symbol('kHandle');
@@ -669,6 +673,69 @@ function jobPromise(getJob) {
669673
}
670674
}
671675

676+
// Temporarily shadow inherited then accessors on WebCrypto result objects.
677+
// Promise resolution reads "then" synchronously for thenable assimilation.
678+
// Returning an own undefined data property keeps that lookup from reaching
679+
// user-mutated prototypes.
680+
function prepareWebCryptoResult(value) {
681+
if ((value === null || typeof value !== 'object') &&
682+
typeof value !== 'function') {
683+
return false;
684+
}
685+
if (isPromise(value) || ObjectPrototypeHasOwnProperty(value, 'then'))
686+
return false;
687+
setOwnProperty(value, 'then', undefined);
688+
return true;
689+
}
690+
691+
// Remove the temporary then property installed by prepareWebCryptoResult().
692+
function cleanupWebCryptoResult(value) {
693+
delete value.then;
694+
}
695+
696+
// Resolve a WebCrypto promise while inherited then accessors are shadowed.
697+
function resolveWebCryptoResult(resolve, value) {
698+
const shouldCleanupResult = prepareWebCryptoResult(value);
699+
try {
700+
resolve(value);
701+
} finally {
702+
if (shouldCleanupResult)
703+
cleanupWebCryptoResult(value);
704+
}
705+
}
706+
707+
// Run a WebCrypto promise reaction and settle the outer promise.
708+
function settleJobPromise(handler, resolve, reject, value, isRejected) {
709+
try {
710+
if (typeof handler === 'function') {
711+
resolveWebCryptoResult(resolve, handler(value));
712+
} else if (isRejected) {
713+
reject(value);
714+
} else {
715+
resolveWebCryptoResult(resolve, value);
716+
}
717+
} catch (err) {
718+
reject(err);
719+
}
720+
}
721+
722+
// Promise.prototype.then gets promise.constructor to determine the result
723+
// promise's species. These promises are internal WebCrypto intermediates, so
724+
// make that lookup stay on the promise itself instead of user-mutated state.
725+
function jobPromiseThen(promise, onFulfilled, onRejected) {
726+
const {
727+
promise: resultPromise,
728+
resolve,
729+
reject,
730+
} = PromiseWithResolvers();
731+
setOwnProperty(promise, 'constructor', undefined);
732+
PromisePrototypeThen(
733+
promise,
734+
(value) => settleJobPromise(onFulfilled, resolve, reject, value, false),
735+
(value) => settleJobPromise(onRejected, resolve, reject, value, true));
736+
return resultPromise;
737+
}
738+
672739
// In WebCrypto, the publicExponent option in RSA is represented as a
673740
// WebIDL "BigInteger"... that is, a Uint8Array that allows an arbitrary
674741
// number of leading zero bits. Our conventional APIs for reading
@@ -891,6 +958,9 @@ module.exports = {
891958
validateByteSource,
892959
validateKeyOps,
893960
jobPromise,
961+
jobPromiseThen,
962+
cleanupWebCryptoResult,
963+
prepareWebCryptoResult,
894964
validateMaxBufferLength,
895965
bigIntArrayToUnsignedBigInt,
896966
bigIntArrayToUnsignedInt,

0 commit comments

Comments
 (0)