Skip to content

Commit 03e6e99

Browse files
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>
1 parent c6785ab commit 03e6e99

13 files changed

Lines changed: 1043 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
TypedArrayPrototypeGetBuffer,
109
Uint8Array,
1110
} = primordials;
@@ -59,6 +58,7 @@ const {
5958
const {
6059
getArrayBufferOrView,
6160
jobPromise,
61+
jobPromiseThen,
6262
toBuf,
6363
kHandle,
6464
} = require('internal/crypto/util');
@@ -368,7 +368,7 @@ function ecdhDeriveBits(algorithm, baseKey, length) {
368368
if (length === null)
369369
return bits;
370370

371-
return PromisePrototypeThen(bits, (bits) => {
371+
return jobPromiseThen(bits, (bits) => {
372372
// If the length is not a multiple of 8 the nearest ceiled
373373
// multiple of 8 is sliced.
374374
const sliceLength = MathCeil(length / 8);

lib/internal/crypto/util.js

Lines changed: 77 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
SafeMap,
1921
SafeSet,
2022
StringPrototypeToUpperCase,
@@ -90,6 +92,7 @@ const {
9092
isDataView,
9193
isArrayBufferView,
9294
isAnyArrayBuffer,
95+
isPromise,
9396
} = require('internal/util/types');
9497

9598
const kHandle = Symbol('kHandle');
@@ -677,6 +680,77 @@ function jobPromise(getJob) {
677680
}
678681
}
679682

683+
// Temporarily shadow inherited then accessors on WebCrypto result objects.
684+
// Promise resolution reads "then" synchronously for thenable assimilation.
685+
// Returning an own undefined data property keeps that lookup from reaching
686+
// user-mutated prototypes.
687+
function prepareWebCryptoResult(value) {
688+
if ((value === null || typeof value !== 'object') &&
689+
typeof value !== 'function') {
690+
return false;
691+
}
692+
if (isPromise(value) || ObjectPrototypeHasOwnProperty(value, 'then'))
693+
return false;
694+
ObjectDefineProperty(value, 'then', {
695+
__proto__: null,
696+
configurable: true,
697+
value: undefined,
698+
});
699+
return true;
700+
}
701+
702+
// Remove the temporary then property installed by prepareWebCryptoResult().
703+
function cleanupWebCryptoResult(value) {
704+
delete value.then;
705+
}
706+
707+
// Resolve a WebCrypto promise while inherited then accessors are shadowed.
708+
function resolveWebCryptoResult(resolve, value) {
709+
const shouldCleanupResult = prepareWebCryptoResult(value);
710+
try {
711+
resolve(value);
712+
} finally {
713+
if (shouldCleanupResult)
714+
cleanupWebCryptoResult(value);
715+
}
716+
}
717+
718+
// Run a WebCrypto promise reaction and settle the outer promise.
719+
function settleJobPromise(handler, resolve, reject, value, isRejected) {
720+
try {
721+
if (typeof handler === 'function') {
722+
resolveWebCryptoResult(resolve, handler(value));
723+
} else if (isRejected) {
724+
reject(value);
725+
} else {
726+
resolveWebCryptoResult(resolve, value);
727+
}
728+
} catch (err) {
729+
reject(err);
730+
}
731+
}
732+
733+
// Promise.prototype.then gets promise.constructor to determine the result
734+
// promise's species. These promises are internal WebCrypto intermediates, so
735+
// make that lookup stay on the promise itself instead of user-mutated state.
736+
function jobPromiseThen(promise, onFulfilled, onRejected) {
737+
const {
738+
promise: resultPromise,
739+
resolve,
740+
reject,
741+
} = PromiseWithResolvers();
742+
ObjectDefineProperty(promise, 'constructor', {
743+
__proto__: null,
744+
configurable: true,
745+
value: undefined,
746+
});
747+
PromisePrototypeThen(
748+
promise,
749+
(value) => settleJobPromise(onFulfilled, resolve, reject, value, false),
750+
(value) => settleJobPromise(onRejected, resolve, reject, value, true));
751+
return resultPromise;
752+
}
753+
680754
// In WebCrypto, the publicExponent option in RSA is represented as a
681755
// WebIDL "BigInteger"... that is, a Uint8Array that allows an arbitrary
682756
// number of leading zero bits. Our conventional APIs for reading
@@ -899,6 +973,9 @@ module.exports = {
899973
validateByteSource,
900974
validateKeyOps,
901975
jobPromise,
976+
jobPromiseThen,
977+
cleanupWebCryptoResult,
978+
prepareWebCryptoResult,
902979
validateMaxBufferLength,
903980
bigIntArrayToUnsignedBigInt,
904981
bigIntArrayToUnsignedInt,

0 commit comments

Comments
 (0)