Skip to content
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

crypto: validate RSA-PSS saltLength in subtle.sign and subtle.verify #52262

Merged
merged 1 commit into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 26 additions & 23 deletions lib/internal/crypto/rsa.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const {
MathCeil,
SafeSet,
Uint8Array,
} = primordials;
Expand All @@ -27,6 +28,7 @@ const {

const {
bigIntArrayToUnsignedInt,
getDigestSizeInBytes,
getUsagesUnion,
hasAnyNotIn,
jobPromise,
Expand Down Expand Up @@ -306,35 +308,36 @@ async function rsaImportKey(
}, keyUsages, extractable);
}

function rsaSignVerify(key, data, { saltLength }, signature) {
let padding;
if (key.algorithm.name === 'RSA-PSS') {
padding = RSA_PKCS1_PSS_PADDING;
// TODO(@jasnell): Validate maximum size of saltLength
// based on the key size:
// Math.ceil((keySizeInBits - 1)/8) - digestSizeInBytes - 2
validateInt32(saltLength, 'algorithm.saltLength', -2);
}

async function rsaSignVerify(key, data, { saltLength }, signature) {
const mode = signature === undefined ? kSignJobModeSign : kSignJobModeVerify;
const type = mode === kSignJobModeSign ? 'private' : 'public';

if (key.type !== type)
throw lazyDOMException(`Key must be a ${type} key`, 'InvalidAccessError');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is not sync anymore. Does this make this semver-major change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not. The entire API surface of webcrypto is async functions.


return jobPromise(() => new SignJob(
kCryptoJobAsync,
signature === undefined ? kSignJobModeSign : kSignJobModeVerify,
key[kKeyObject][kHandle],
undefined,
undefined,
undefined,
data,
normalizeHashName(key.algorithm.hash.name),
saltLength,
padding,
undefined,
signature));
return jobPromise(() => {
if (key.algorithm.name === 'RSA-PSS') {
validateInt32(
saltLength,
'algorithm.saltLength',
0,
MathCeil((key.algorithm.modulusLength - 1) / 8) - getDigestSizeInBytes(key.algorithm.hash.name) - 2);
}

return new SignJob(
kCryptoJobAsync,
signature === undefined ? kSignJobModeSign : kSignJobModeVerify,
key[kKeyObject][kHandle],
undefined,
undefined,
undefined,
data,
normalizeHashName(key.algorithm.hash.name),
saltLength,
key.algorithm.name === 'RSA-PSS' ? RSA_PKCS1_PSS_PADDING : undefined,
undefined,
signature);
});
}


Expand Down
10 changes: 10 additions & 0 deletions lib/internal/crypto/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,15 @@ function getBlockSize(name) {
}
}

function getDigestSizeInBytes(name) {
switch (name) {
case 'SHA-1': return 20;
case 'SHA-256': return 32;
case 'SHA-384': return 48;
case 'SHA-512': return 64;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a default with unreachable (like throw error)

Copy link
Member Author

@panva panva Mar 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done the same way as the method above. I don't feel it's necessary. Used hashes are verified far before this is ever invoked.

}
}

const kKeyOps = {
sign: 1,
verify: 2,
Expand Down Expand Up @@ -596,6 +605,7 @@ module.exports = {
bigIntArrayToUnsignedBigInt,
bigIntArrayToUnsignedInt,
getBlockSize,
getDigestSizeInBytes,
getStringOption,
getUsagesUnion,
secureHeapUsed,
Expand Down
35 changes: 35 additions & 0 deletions test/parallel/test-webcrypto-sign-verify-rsa.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,35 @@ async function testSign({
});
}

async function testSaltLength(keyLength, hash, hLen) {
const { publicKey, privateKey } = await subtle.generateKey({
name: 'RSA-PSS',
modulusLength: keyLength,
publicExponent: new Uint8Array([1, 0, 1]),
hash,
}, false, ['sign', 'verify']);

const data = Buffer.from('Hello, world!');
const max = keyLength / 8 - hLen - 2;

const signature = await subtle.sign({ name: 'RSA-PSS', saltLength: max }, privateKey, data);
await assert.rejects(
subtle.sign({ name: 'RSA-PSS', saltLength: max + 1 }, privateKey, data), (err) => {
assert.strictEqual(err.name, 'OperationError');
assert.strictEqual(err.cause?.code, 'ERR_OUT_OF_RANGE');
assert.strictEqual(err.cause?.message, `The value of "algorithm.saltLength" is out of range. It must be >= 0 && <= ${max}. Received ${max + 1}`);
return true;
});
await subtle.verify({ name: 'RSA-PSS', saltLength: max }, publicKey, signature, data);
await assert.rejects(
subtle.verify({ name: 'RSA-PSS', saltLength: max + 1 }, publicKey, signature, data), (err) => {
assert.strictEqual(err.name, 'OperationError');
assert.strictEqual(err.cause?.code, 'ERR_OUT_OF_RANGE');
assert.strictEqual(err.cause?.message, `The value of "algorithm.saltLength" is out of range. It must be >= 0 && <= ${max}. Received ${max + 1}`);
return true;
});
}

(async function() {
const variations = [];

Expand All @@ -206,5 +235,11 @@ async function testSign({
variations.push(testSign(vector));
});

for (const keyLength of [1024, 2048]) {
for (const [hash, hLen] of [['SHA-1', 20], ['SHA-256', 32], ['SHA-384', 48], ['SHA-512', 64]]) {
variations.push(testSaltLength(keyLength, hash, hLen));
}
}

await Promise.all(variations);
})().then(common.mustCall());
Loading