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: fix checkPrime crash with large buffers #56559

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −0
Open

crypto: fix checkPrime crash with large buffers #56559

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
5 changes: 5 additions & 0 deletions src/crypto/crypto_random.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,11 @@ Maybe<void> CheckPrimeTraits::AdditionalConfig(
ArrayBufferOrViewContents<unsigned char> candidate(args[offset]);

params->candidate = BignumPointer(candidate.data(), candidate.size());
if (params->candidate.get() == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

A simplified bool check should work here

Suggested change
if (params->candidate.get() == nullptr) {
if (!params->candidate) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Hadn't noticed the bool operator was implemented. Thanks!

ThrowCryptoError(
Environment::GetCurrent(args), ERR_get_error(), "BignumPointer");
return Nothing<void>();
}

CHECK(args[offset + 1]->IsInt32()); // Checks
params->checks = args[offset + 1].As<Int32>()->Value();
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-crypto-prime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const {
generatePrimeSync,
checkPrime,
checkPrimeSync,
randomBytes,
} = require('crypto');

const { Worker } = require('worker_threads');
Expand Down Expand Up @@ -254,6 +255,18 @@ for (const checks of [-(2 ** 31), -1, 2 ** 31, 2 ** 32 - 1, 2 ** 32, 2 ** 50]) {
});
}

{
const bytes = randomBytes(67108864);
Copy link
Member

Choose a reason for hiding this comment

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

Is generating this large of a random number each time necessary? Should be possible to generate a value statically once that trips this error reliably.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean storing the number into a file in fixtures and use that in the test?

Copy link
Member

Choose a reason for hiding this comment

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

doesn't need to be in fixtures (can just be defined inline here) but yes that would work.

Copy link
Member Author

@santigimeno santigimeno Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'll do it in a file: it's 64M. Do we really want to have such a large file in the repo though?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's better than slowing down the CI every run by generating 64M random bytes.

Copy link
Member

Choose a reason for hiding this comment

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

That said, couldn't it just be 64M of zeroes? It doesn't actually need to be random or a prime if I understand correctly.

assert.throws(() => checkPrime(bytes, common.mustNotCall()), {
code: 'ERR_OSSL_BN_BIGNUM_TOO_LONG',
message: /bignum too long/
});
assert.throws(() => checkPrimeSync(bytes), {
code: 'ERR_OSSL_BN_BIGNUM_TOO_LONG',
message: /bignum too long/
});
}

assert(!checkPrimeSync(Buffer.from([0x1])));
assert(checkPrimeSync(Buffer.from([0x2])));
assert(checkPrimeSync(Buffer.from([0x3])));
Expand Down
Loading