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: pass all WebCryptoAPI WPTs #43656

Closed
wants to merge 17 commits into from
Closed

crypto: pass all WebCryptoAPI WPTs #43656

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9952d1f
crypto: pass all webcrypto WPTs
panva Jul 2, 2022
491ea8d
address comments
panva Jul 3, 2022
d9cd385
make format-cpp
panva Jul 3, 2022
f0c0a9c
apply review feedback
panva Jul 3, 2022
3720974
mark test-webcrypto SLOW
panva Jul 3, 2022
4fcfa50
Revert "mark test-webcrypto SLOW"
panva Jul 3, 2022
e305875
apply review feedback
panva Jul 5, 2022
7745cb0
apply review feedback
panva Jul 6, 2022
41d849d
see if increasing wpt timeout helps with ci
panva Jul 12, 2022
07a8676
test if runing wpt js files one by one helps with test-webcrypto
panva Jul 13, 2022
fb5eb54
fix lint
panva Jul 13, 2022
03275d0
apply review feedback
panva Jul 23, 2022
8c05855
update hkdf docs
panva Jul 23, 2022
2561171
add 0-length key hmac and equals tests
panva Jul 23, 2022
09a6e50
add yaml change entries
panva Jul 23, 2022
d51c30c
apply review feedback
panva Jul 23, 2022
c8d7b2c
make lint happy
panva Jul 23, 2022
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
12 changes: 5 additions & 7 deletions lib/internal/crypto/aes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@ const {
generateKey,
} = require('internal/crypto/keygen');

const {
validateInteger,
validateOneOf,
} = require('internal/validators');

const kMaxCounterLength = 128;
const kTagLengths = [32, 64, 96, 104, 112, 120, 128];

Expand Down Expand Up @@ -227,8 +222,11 @@ function aesCipher(mode, key, data, algorithm) {

async function aesGenerateKey(algorithm, extractable, keyUsages) {
const { name, length } = algorithm;
validateInteger(length, 'algorithm.length');
validateOneOf(length, 'algorithm.length', kAesKeyLengths);
if (!ArrayPrototypeIncludes(kAesKeyLengths, length)) {
throw lazyDOMException(
'AES key length must be 128, 192, or 256 bits',
'OperationError');
}

const checkUsages = ['wrapKey', 'unwrapKey'];
if (name !== 'AES-KW')
Expand Down
29 changes: 14 additions & 15 deletions lib/internal/crypto/ec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const {
ArrayPrototypeIncludes,
ObjectKeys,
Promise,
SafeSet,
Expand All @@ -17,11 +18,6 @@ const {
kSigEncP1363,
} = internalBinding('crypto');

const {
validateOneOf,
validateString,
} = require('internal/validators');

const {
codes: {
ERR_MISSING_OPTION,
Expand Down Expand Up @@ -88,11 +84,12 @@ function createECPublicKeyRaw(namedCurve, keyData) {

async function ecGenerateKey(algorithm, extractable, keyUsages) {
const { name, namedCurve } = algorithm;
validateString(namedCurve, 'algorithm.namedCurve');
validateOneOf(
namedCurve,
'algorithm.namedCurve',
ObjectKeys(kNamedCurveAliases));

if (!ArrayPrototypeIncludes(ObjectKeys(kNamedCurveAliases), namedCurve)) {
throw lazyDOMException(
'Unrecognized namedCurve',
'NotSupportedError');
}

const usageSet = new SafeSet(keyUsages);
switch (name) {
Expand Down Expand Up @@ -168,11 +165,13 @@ async function ecImportKey(
keyUsages) {

const { name, namedCurve } = algorithm;
validateString(namedCurve, 'algorithm.namedCurve');
validateOneOf(
namedCurve,
'algorithm.namedCurve',
ObjectKeys(kNamedCurveAliases));

if (!ArrayPrototypeIncludes(ObjectKeys(kNamedCurveAliases), namedCurve)) {
throw lazyDOMException(
'Unrecognized namedCurve',
'NotSupportedError');
}

let keyObject;
const usagesSet = new SafeSet(keyUsages);
switch (format) {
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/crypto/hkdf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ function hkdfSync(hash, key, salt, info, length) {
}

async function hkdfDeriveBits(algorithm, baseKey, length) {
validateUint32(length, 'length');
const { hash } = algorithm;
const salt = getArrayBufferOrView(algorithm.salt, 'algorithm.salt');
const info = getArrayBufferOrView(algorithm.info, 'algorithm.info');
Expand All @@ -153,6 +152,9 @@ async function hkdfDeriveBits(algorithm, baseKey, length) {
if (length !== undefined) {
if (length === 0)
throw lazyDOMException('length cannot be zero', 'OperationError');
if (length === null)
throw lazyDOMException('length cannot be null', 'OperationError');
validateUint32(length, 'length');
if (length % 8) {
throw lazyDOMException(
'length must be a multiple of 8',
Expand Down
3 changes: 0 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 @@ -38,7 +38,6 @@ const {
ERR_ILLEGAL_CONSTRUCTOR,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_OUT_OF_RANGE,
}
} = require('internal/errors');

Expand Down Expand Up @@ -588,8 +587,6 @@ function prepareSecretKey(key, encoding, bufferOnly = false) {

function createSecretKey(key, encoding) {
key = prepareSecretKey(key, encoding, true);
if (key.byteLength === 0)
throw new ERR_OUT_OF_RANGE('key.byteLength', '> 0', key.byteLength);
panva marked this conversation as resolved.
Show resolved Hide resolved
const handle = new KeyObjectHandle();
handle.init(kKeyTypeSecret, key);
return new SecretKeyObject(handle);
Expand Down
3 changes: 0 additions & 3 deletions lib/internal/crypto/mac.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,6 @@ async function hmacImportKey(
case 'raw': {
const checkLength = keyData.byteLength * 8;

if (checkLength === 0 || algorithm.length === 0)
throw lazyDOMException('Zero-length key is not supported', 'DataError');

// The Web Crypto spec allows for key lengths that are not multiples of
// 8. We don't. Our check here is stricter than that defined by the spec
// in that we require that algorithm.length match keyData.length * 8 if
Expand Down
10 changes: 8 additions & 2 deletions lib/internal/crypto/pbkdf2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,16 @@ function check(password, salt, iterations, keylen, digest) {
}

async function pbkdf2DeriveBits(algorithm, baseKey, length) {
validateUint32(length, 'length');
const { iterations } = algorithm;
let { hash } = algorithm;
const salt = getArrayBufferOrView(algorithm.salt, 'algorithm.salt');
if (hash === undefined)
throw new ERR_MISSING_OPTION('algorithm.hash');
validateInteger(iterations, 'algorithm.iterations', 1);
validateInteger(iterations, 'algorithm.iterations');
if (iterations === 0)
throw lazyDOMException(
'iterations cannot be zero',
'OperationError');

hash = normalizeHashName(hash.name);

Expand All @@ -114,6 +117,9 @@ async function pbkdf2DeriveBits(algorithm, baseKey, length) {
if (length !== undefined) {
if (length === 0)
throw lazyDOMException('length cannot be zero', 'OperationError');
if (length === null)
throw lazyDOMException('length cannot be null', 'OperationError');
validateUint32(length, 'length');
if (length % 8) {
throw lazyDOMException(
'length must be a multiple of 8',
Expand Down
6 changes: 5 additions & 1 deletion lib/internal/crypto/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,11 @@ const validateByteSource = hideStackFrames((val, name) => {
});

function onDone(resolve, reject, err, result) {
if (err) return reject(err);
if (err) {
return reject(lazyDOMException(
'The operation failed for an operation-specific reason',
'OperationError'));
}
resolve(result);
}

Expand Down
3 changes: 0 additions & 3 deletions lib/internal/crypto/webcrypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,9 +494,6 @@ async function importGenericSecretKey(

const checkLength = keyData.byteLength * 8;

if (checkLength === 0 || length === 0)
throw lazyDOMException('Zero-length key is not supported', 'DataError');

// The Web Crypto spec allows for key lengths that are not multiples of
// 8. We don't. Our check here is stricter than that defined by the spec
// in that we require that algorithm.length match keyData.length * 8 if
Expand Down
45 changes: 37 additions & 8 deletions src/crypto/crypto_hkdf.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,20 +103,49 @@ bool HKDFTraits::DeriveBits(
EVPKeyCtxPointer ctx =
EVPKeyCtxPointer(EVP_PKEY_CTX_new_id(EVP_PKEY_HKDF, nullptr));
if (!ctx || !EVP_PKEY_derive_init(ctx.get()) ||
!EVP_PKEY_CTX_hkdf_mode(ctx.get(),
EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND) ||
!EVP_PKEY_CTX_set_hkdf_md(ctx.get(), params.digest) ||
!EVP_PKEY_CTX_set1_hkdf_salt(
ctx.get(), params.salt.data<unsigned char>(), params.salt.size()) ||
!EVP_PKEY_CTX_set1_hkdf_key(
ctx.get(),
reinterpret_cast<const unsigned char*>(params.key->GetSymmetricKey()),
params.key->GetSymmetricKeySize()) ||
!EVP_PKEY_CTX_add1_hkdf_info(
ctx.get(), params.info.data<unsigned char>(), params.info.size())) {
return false;
}

if (params.key->GetSymmetricKeySize() != 0) {
if (!EVP_PKEY_CTX_hkdf_mode(ctx.get(),
EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND) ||
!EVP_PKEY_CTX_set1_hkdf_salt(
ctx.get(), params.salt.data<unsigned char>(), params.salt.size()) ||
Copy link
Member

Choose a reason for hiding this comment

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

Does a non-zero-length key work with a 0-length salt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://datatracker.ietf.org/doc/html/rfc5869#section-2.2

Copy link
Member

Choose a reason for hiding this comment

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

I know it's allowed, I am just wondering if OpenSSL implements it that way. The RFC's statement if not provided could be interpreted as "if the user does not call EVP_PKEY_CTX_set1_hkdf_salt".

Now I am confused as to whether an empty salt is the same as passing no salt in our current implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Our implementation does not allow to omit passing salt, we require the argument, albeit we allow it to be zero-length.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does a non-zero-length key work with a 0-length salt?

that's also covered by a passing wpt btw

!EVP_PKEY_CTX_set1_hkdf_key(ctx.get(),
reinterpret_cast<const unsigned char*>(
params.key->GetSymmetricKey()),
params.key->GetSymmetricKeySize())) {
return false;
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a brief comment to the beginning of the else branch to explain why it exists for future readers?

If we accept that this branch must exist (until we fully drop OpenSSL 1.1.1), then we should probably either

  • remove the other branch that uses EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND and pass the key to HMAC in this new branch (that should be equivalent as far as I can tell), or
  • add a TODO comment saying to remove this branch once we have dropped OpenSSL 1.1.1.

The first option would give us improved coverage because all HMAC operations would go through it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you add a brief comment to the beginning of the else branch to explain why it exists for future readers?

Sure.

If we accept that this branch must exist (until we fully drop OpenSSL 1.1.1), then we should probably either

  • remove the other branch that uses EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND and pass the key to HMAC in this new branch (that should be equivalent as far as I can tell)

I think that can be done as a follow-up, I am not up for such challenge myself.

  • add a TODO comment saying to remove this branch once we have dropped OpenSSL 1.1.1.

You mean to change the implementation to use EVP_KDF-HKDF when 1.1.1 is dropped?

unsigned char temp_key[EVP_MAX_MD_SIZE];
unsigned int len = sizeof(temp_key);
if (params.salt.size()) {
panva marked this conversation as resolved.
Show resolved Hide resolved
if (HMAC(params.digest,
params.salt.data(),
params.salt.size(),
nullptr,
0,
temp_key,
&len) == nullptr) {
return false;
}
} else {
char salt[EVP_MAX_KEY_LENGTH] = {0};
if (HMAC(params.digest, salt, sizeof(salt), nullptr, 0, temp_key, &len) ==
panva marked this conversation as resolved.
Show resolved Hide resolved
nullptr) {
return false;
}
}
if (!EVP_PKEY_CTX_hkdf_mode(ctx.get(), EVP_PKEY_HKDEF_MODE_EXPAND_ONLY) ||
!EVP_PKEY_CTX_set1_hkdf_key(ctx.get(), temp_key, len)) {
return false;
}
}

size_t length = params.length;
ByteSource::Builder buf(length);
if (EVP_PKEY_derive(ctx.get(), buf.data<unsigned char>(), &length) <= 0)
Expand Down
1 change: 0 additions & 1 deletion src/crypto/crypto_keys.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,6 @@ void KeyObjectData::MemoryInfo(MemoryTracker* tracker) const {
}

std::shared_ptr<KeyObjectData> KeyObjectData::CreateSecret(ByteSource key) {
CHECK(key);
Copy link
Member

Choose a reason for hiding this comment

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

We might need to verify that no other uses of KeyObjectData (and related structures) assume that the stored pointer is not nullptr. Otherwise, we might end up with aborts/crashes if someone constructs 0-length keys and uses them with other APIs.

Copy link
Member Author

@panva panva Jul 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Why is a zero length buffer key data treated as a nullptr? I would like to keep the check here but I don't understand the C++ keyobject implementation enough.

Copy link
Member

Choose a reason for hiding this comment

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

It might be because malloc(len) for len = 0 is allowed to return a nullptr. The standard only requires the returned pointer to be valid for len bytes, which nullptr fulfills when len = 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a suggestion that would allow us to keep this check?

return std::shared_ptr<KeyObjectData>(new KeyObjectData(std::move(key)));
}

Expand Down
8 changes: 7 additions & 1 deletion test/common/wpt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ class WPTRunner {

// TODO(joyeecheung): work with the upstream to port more tests in .html
// to .js.
runJsTests() {
async runJsTests() {
let queue = [];

// If the tests are run as `node test/wpt/test-something.js subset.any.js`,
Expand Down Expand Up @@ -459,6 +459,12 @@ class WPTRunner {
);
this.inProgress.delete(testFileName);
});

await new Promise((resolve) => {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to reject when the worker emits an error event, you could also use
await events.once(worker, 'exit').

That being said, it looks like the caller never awaits the returned Promise (e.g., in test-webcrypto.js). What is the point in creating the Promise here if the caller does not await it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The point is to process the queue one by one. This has helped stabilize CI, especially the keygen tests that easily bogged the hosts down minutes at a time.

Copy link
Member Author

@panva panva Jul 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If you want to reject when the worker emits an error event, you could also use await events.once(worker, 'exit').

Rejecting would skip running the rest of the queue. So maybe this?

await Promise.allSettled([events.once(worker, 'exit')])

or

await events.once(worker, 'exit').catch(() => {})

worker.on('exit', () => {
resolve();
});
});
}

process.on('exit', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/wpt/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Last update:
- user-timing: https://github.com/web-platform-tests/wpt/tree/df24fb604e/user-timing
- wasm/jsapi: https://github.com/web-platform-tests/wpt/tree/1dd414c796/wasm/jsapi
- wasm/webapi: https://github.com/web-platform-tests/wpt/tree/fd1b23eeaa/wasm/webapi
- WebCryptoAPI: https://github.com/web-platform-tests/wpt/tree/cdd0f03df4/WebCryptoAPI
- WebCryptoAPI: https://github.com/web-platform-tests/wpt/tree/edca84af42/WebCryptoAPI
- webidl/ecmascript-binding/es-exceptions: https://github.com/web-platform-tests/wpt/tree/a370aad338/webidl/ecmascript-binding/es-exceptions

[Web Platform Tests]: https://github.com/web-platform-tests/wpt
Expand Down
Loading