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.

Sign up for GitHub

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: refactor to avoid unsafe array iteration #37364

Merged
merged 1 commit into from
Feb 19, 2021
Merged

crypto: refactor to avoid unsafe array iteration #37364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions lib/internal/crypto/aes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
ArrayPrototypePush,
MathFloor,
Promise,
ReflectApply,
SafeSet,
TypedArrayPrototypeSlice,
} = primordials;
Expand Down Expand Up @@ -252,12 +253,12 @@ async function aesImportKey(
extractable,
keyUsages) {
const { name } = algorithm;
const checkUsages = ['wrapKey', 'unwrapKey'];
const usagesSet = new SafeSet(keyUsages);
const checkUsages = [usagesSet, 'wrapKey', 'unwrapKey'];
if (name !== 'AES-KW')
ArrayPrototypePush(checkUsages, 'encrypt', 'decrypt');

const usagesSet = new SafeSet(keyUsages);
if (hasAnyNotIn(usagesSet, ...checkUsages)) {
if (ReflectApply(hasAnyNotIn, null, checkUsages)) {
Copy link
Member

Choose a reason for hiding this comment

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

These call sites of hasAnyNotIn are much harder to read now, for no real benefit.

It might be better to simply change hasAnyNotIn to accept an array as the second argument instead of a variable number of arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll try that, if it doesn't have a negative perf impact I'm all for it.

throw lazyDOMException(
'Unsupported key usage for an AES key',
'SyntaxError');
Expand Down
9 changes: 5 additions & 4 deletions lib/internal/crypto/diffiehellman.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

const {
ArrayBufferPrototypeSlice,
ArrayPrototypePush,
FunctionPrototypeCall,
MathFloor,
ObjectDefineProperty,
Promise,
ReflectApply,
SafeSet,
} = primordials;

Expand Down Expand Up @@ -347,16 +349,15 @@ function deriveBitsDH(publicKey, privateKey, callback) {
}

function verifyAcceptableDhKeyUse(name, type, usages) {
let checkSet;
const args = [usages];
switch (type) {
case 'private':
checkSet = ['deriveBits', 'deriveKey'];
ArrayPrototypePush(args, 'deriveBits', 'deriveKey');
break;
case 'public':
checkSet = [];
break;
}
if (hasAnyNotIn(usages, ...checkSet)) {
if (ReflectApply(hasAnyNotIn, null, args)) {
Copy link
Member

Choose a reason for hiding this comment

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

Dito.

throw lazyDOMException(
`Unsupported key usage for an ${name} key`,
'SyntaxError');
Expand Down
8 changes: 4 additions & 4 deletions lib/internal/crypto/dsa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,16 @@ const {
} = require('internal/crypto/util');

function verifyAcceptableDsaKeyUse(name, type, usages) {
let checkSet;
let check;
switch (type) {
case 'private':
checkSet = ['sign'];
check = 'sign';
break;
case 'public':
checkSet = ['verify'];
check = 'verify';
break;
}
if (hasAnyNotIn(usages, ...checkSet)) {
if (hasAnyNotIn(usages, check)) {
Copy link
Member

Choose a reason for hiding this comment

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

Dito.

throw lazyDOMException(
`Unsupported key usage for an ${name} key`,
'SyntaxError');
Expand Down
12 changes: 7 additions & 5 deletions lib/internal/crypto/ec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
'use strict';

const {
ArrayPrototypePush,
ObjectKeys,
Promise,
ReflectApply,
SafeSet,
} = primordials;

Expand Down Expand Up @@ -59,10 +61,10 @@ const {
} = require('internal/crypto/keys');

function verifyAcceptableEcKeyUse(name, type, usages) {
let checkSet;
const args = [usages];
switch (name) {
case 'ECDH':
checkSet = ['deriveKey', 'deriveBits'];
ArrayPrototypePush(args, 'deriveKey', 'deriveBits');
break;
case 'NODE-ED25519':
// Fall through
Expand All @@ -71,14 +73,14 @@ function verifyAcceptableEcKeyUse(name, type, usages) {
case 'ECDSA':
switch (type) {
case 'private':
checkSet = ['sign'];
ArrayPrototypePush(args, 'sign');
break;
case 'public':
checkSet = ['verify'];
ArrayPrototypePush(args, 'verify');
break;
}
}
if (hasAnyNotIn(usages, ...checkSet)) {
if (ReflectApply(hasAnyNotIn, null, args)) {
Copy link
Member

Choose a reason for hiding this comment

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

Dito.

throw lazyDOMException(
`Unsupported key usage for a ${name} key`,
'SyntaxError');
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/crypto/hkdf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ function hkdfSync(hash, key, salt, info, length) {
} = validateParameters(hash, key, salt, info, length));

const job = new HKDFJob(kCryptoJobSync, hash, key, salt, info, length);
const [err, bits] = job.run();
const { 0: err, 1: bits } = job.run();
if (err !== undefined)
throw err;

Expand Down
11 changes: 6 additions & 5 deletions lib/internal/crypto/keygen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
const {
FunctionPrototypeCall,
ObjectDefineProperty,
SafeArrayIterator,
} = primordials;

const {
Expand Down Expand Up @@ -75,7 +76,7 @@ function generateKeyPair(type, options, callback) {
job.ondone = (error, result) => {
if (error) return FunctionPrototypeCall(callback, job, error);
// If no encoding was chosen, return key objects instead.
let [pubkey, privkey] = result;
let { 0: pubkey, 1: privkey } = result;
pubkey = wrapKey(pubkey, PublicKeyObject);
privkey = wrapKey(privkey, PrivateKeyObject);
FunctionPrototypeCall(callback, job, null, pubkey, privkey);
Expand All @@ -97,11 +98,11 @@ function handleError(ret) {
if (ret == null)
return; // async

const [err, keys] = ret;
const { 0: err, 1: keys } = ret;
if (err !== undefined)
throw err;

const [publicKey, privateKey] = keys;
const { 0: publicKey, 1: privateKey } = keys;

// If no encoding was chosen, return key objects instead.
return {
Expand Down Expand Up @@ -156,7 +157,7 @@ function parseKeyEncoding(keyType, options = {}) {
function createJob(mode, type, options) {
validateString(type, 'type');

const encoding = parseKeyEncoding(type, options);
const encoding = new SafeArrayIterator(parseKeyEncoding(type, options));

if (options !== undefined)
validateObject(options, 'options');
Expand Down Expand Up @@ -341,7 +342,7 @@ function handleGenerateKeyError(ret) {
if (ret === undefined)
return; // async

const [err, key] = ret;
const { 0: err, 1: key } = ret;
if (err !== undefined)
throw err;

Expand Down
20 changes: 17 additions & 3 deletions lib/internal/crypto/keys.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const {
ArrayFrom,
ArrayPrototypeSlice,
ObjectDefineProperty,
ObjectSetPrototypeOf,
Symbol,
Expand Down Expand Up @@ -160,6 +161,11 @@ const [
}

class AsymmetricKeyObject extends KeyObject {
// eslint-disable-next-line no-useless-constructor
constructor(type, handle) {
super(type, handle);
}
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

get asymmetricKeyType() {
return this[kAsymmetricKeyType] ||
(this[kAsymmetricKeyType] = this[kHandle].getAsymmetricKeyType());
Expand Down Expand Up @@ -390,13 +396,21 @@ function getKeyObjectHandle(key, ctx) {
}

function getKeyTypes(allowKeyObject, bufferOnly = false) {
return [
const types = [
'ArrayBuffer',
'Buffer',
'TypedArray',
'DataView',
...(!bufferOnly ? ['string'] : []),
...(!bufferOnly && allowKeyObject ? ['KeyObject', 'CryptoKey'] : [])];
'string', // Only if bufferOnly == false
'KeyObject', // Only if allowKeyObject == true && bufferOnly == false
'CryptoKey', // Only if allowKeyObject == true && bufferOnly == false
];
if (bufferOnly) {
return ArrayPrototypeSlice(types, 0, 4);
} else if (!allowKeyObject) {
return ArrayPrototypeSlice(types, 0, 5);
}
return types;
Copy link
Member

Choose a reason for hiding this comment

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

This is so much less intuitive than the original implementation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I have considered using .push to add the entries instead of slicing them out, but ended up doing it that way to improve the performance.

}

function prepareAsymmetricKey(key, ctx) {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/crypto/pbkdf2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ function pbkdf2Sync(password, salt, iterations, keylen, digest) {
keylen,
digest);

const [err, result] = job.run();
const { 0: err, 1: result } = job.run();
if (err !== undefined)
throw err;

Expand Down
6 changes: 3 additions & 3 deletions lib/internal/crypto/random.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ function randomFillSync(buf, offset = 0, size) {
offset,
size);

const [ err ] = job.run();
const err = job.run()[0];
if (err)
throw err;

Expand Down Expand Up @@ -471,7 +471,7 @@ function generatePrimeSync(size, options = {}) {
validateUint32(size, 'size', true);

const job = createRandomPrimeJob(kCryptoJobSync, size, options);
const [err, prime] = job.run();
const { 0: err, 1: prime } = job.run();
if (err)
throw err;
return job.result(prime);
Expand Down Expand Up @@ -548,7 +548,7 @@ function checkPrimeSync(candidate, options = {}) {
validateUint32(checks, 'options.checks');

const job = new CheckPrimeJob(kCryptoJobSync, candidate, checks);
const [err, result] = job.run();
const { 0: err, 1: result } = job.run();
if (err)
throw err;

Expand Down
14 changes: 8 additions & 6 deletions lib/internal/crypto/rsa.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
'use strict';

const {
ArrayPrototypePush,
Promise,
ReflectApply,
SafeSet,
Uint8Array,
} = primordials;
Expand Down Expand Up @@ -71,29 +73,29 @@ const kRsaVariants = {
};

function verifyAcceptableRsaKeyUse(name, type, usages) {
let checkSet;
const args = [usages];
switch (name) {
case 'RSA-OAEP':
switch (type) {
case 'private':
checkSet = ['decrypt', 'unwrapKey'];
ArrayPrototypePush(args, 'decrypt', 'unwrapKey');
break;
case 'public':
checkSet = ['encrypt', 'wrapKey'];
ArrayPrototypePush(args, 'encrypt', 'wrapKey');
break;
}
break;
default:
switch (type) {
case 'private':
checkSet = ['sign'];
ArrayPrototypePush(args, 'sign');
break;
case 'public':
checkSet = ['verify'];
ArrayPrototypePush(args, 'verify');
break;
}
}
if (hasAnyNotIn(usages, ...checkSet)) {
if (ReflectApply(hasAnyNotIn, null, args)) {
Copy link
Member

Choose a reason for hiding this comment

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

Dito.

throw lazyDOMException(
`Unsupported key usage for an ${name} key`,
'SyntaxError');
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/crypto/scrypt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ function scryptSync(password, salt, keylen, options = defaults) {
({ password, salt, keylen } = options);
const job = new ScryptJob(
kCryptoJobSync, password, salt, N, r, p, maxmem, keylen);
const [err, result] = job.run();
const { 0: err, 1: result } = job.run();

if (err !== undefined)
throw err;
Expand Down