`crypto.pseudoRandomBytes` results in a segmentation fault

[zyscoder]

• Version: v14.15.1
• Platform: Linux 5.8.0-38-generic The binary and long term compatibility with node #43~20.04.1-Ubuntu SMP Tue Jan 12 16:39:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: crypto.pseudoRandomBytes

What steps will reproduce the bug?

Setup a node instance,

» node

and run the following javascript code.

crypto.pseudoRandomBytes(2147483648)

Then a segmentation fault occurs.

How often does it reproduce? Is there a required condition?

This problem can always be triggered following the steps above.
Noticed that 2147483647 would not cause this problem. It seems to result from an integer overflow.

What is the expected behavior?

If any error occurs, an exception or other similar error-reporting stuff should be thrown. There is no reason to abort the whole node process.

What do you see instead?

» node
Welcome to Node.js v14.15.1.
Type ".help" for more information.
> crypto.pseudoRandomBytes(2147483647)
<Buffer b7 d9 2d 27 37 50 a1 4b df f3 a2 ad fa 85 16 8e e6 a2 c4 98 a3 48 9a 65 1f 3a c1 9e d8 c1 6e bb ed ef ed 85 dc ae b7 6f 5c 53 7e 09 fb a9 1d 66 7e d5 ... 2147483597 more bytes>
> crypto.pseudoRandomBytes(2147483648)
[1]    2791292 segmentation fault  node

Additional information

[Ayase-252]

According to the docs

size The number of bytes to generate. The size must not be larger than 2**31 - 1.

It seems we have know it will not work in 2 ** 31(2147483648). And there is an validation here but it did not work.

The issue also occurs when execute in node

Welcome to Node.js v14.16.0.
Type ".help" for more information.
> crypto.randomBytes(2147483648)
[1]    83064 segmentation fault  node

[aduh95]

Doesn't reproduce on Node.js v15.x, probably fixed by #35093.

[apple502j]

Can repro on Windows 10 as well.

(5628.5a40): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
node!v8::internal::compiler::RepresentationChanger::Uint32OverflowOperatorFor+0x42699:
00007ff7`01593079 f3aa            rep stos byte ptr [rdi]
0:000> !load msec
0:000> !exploitable

!exploitable 1.6.0.0
Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - User Mode Write AV starting at node!v8::internal::compiler::RepresentationChanger::Uint32OverflowOperatorFor+0x0000000000042699 (Hash=0xccd97b0f.0xbe9a6a99)

User mode write access violations that are not near NULL are exploitable.

[zyscoder]

I'm not a JavaScript developer. I am just working on some research to detect bugs and NodeJS is one of my test targets.
Thus maybe some of my issues do not make sense, and thanks for your patient reply.
I am working on the LTS versions now. I have a personal question that whether issues that occurred on the old versions like this one are meaningful for you.
Since confirmed-bug is important for me. If there is no sense of these problems, then maybe I should save my machine time to focus on more meaningful places.

[aduh95]

@zyscoder First of all, thanks for all the reports you sent! Bug reports on LTS versions are certainly valid, Node.js as project would accept PR fixing that kind of crash for all supported versions.
One of the drawbacks of using an LTS version to search for bugs is you might end up discovering bugs already fixed on master: we have a "maturation" policy before backporting commits to LTS releases, so most of the times such patch will end up automatically in the LTS release line a few month later. This particular bug was fixed on a semver-major commit, so it won't be backported to v14.x line until someone takes the time to open a backport PR for it, which could never happen if no one volunteers.

TL;DR, you might want to target the master branch (or maybe v16.0.0-rc.1) to decrease the probability of finding already reported bugs.

[Ayase-252]

After digging into the code, I found there is a discrepancy of validation in JS side and C++ side. So, technically, the issue "exists" in master branch but it guarded by C++ backend (introduced in #35093). I would like to open a PR to address it.

[aduh95]

@Ayase-252 why does it matter if it's handled by C++ or JS code?

[Ayase-252]

@aduh95
TL;DL; It seems the buffer.constants.MAX_LENGTH increased from MAX_INT(2** 31 - 1) to 2 ** 32 somehow in 64 bits platform(Refs: #38093). It breaks the validation in JS side. However, it has an extra guard in C++ in v15, but not in v14.

Both v14 and v15 has validation for size argument, as

node/lib/internal/crypto/random.js

Line 92 in 4f387c2

size = assertSize(size, 1, 0, Infinity);

Let's see assertSize

node/lib/internal/crypto/random.js

Lines 75 to 89 in 4f387c2

	function assertSize(size, elementSize, offset, length) {
	validateNumber(size, 'size');
	size *= elementSize;
	if (NumberIsNaN(size) || size > kMaxPossibleLength || size < 0) {
	throw new ERR_OUT_OF_RANGE('size',
	`>= 0 && <= ${kMaxPossibleLength}`, size);
	}
	if (size + offset > length) {
	throw new ERR_OUT_OF_RANGE('size + offset', `<= ${length}`, size + offset);
	}
	return size >>> 0; // Convert to uint32.
	}

Note that kMaxPossibleLength is

node/lib/internal/crypto/random.js

Line 61 in 4f387c2

const kMaxPossibleLength = MathMin(kMaxLength, kMaxUint32);

where kMaxLength is buffer.kMaxLength (alias of buffer.constants.MAX_LENGTH).

Since buffer.kMaxLength raised to 2 ** 32, what it actually doing is ensures that whether size is less than 2 ** 32 - 1, not 2 ** 31 - 1 as expected.

However, in v15, an extra guard is introduced as part of #35093. It ensures size must be less than 2 ** 31 - 1 and prevents abort when size is larger than 2 ** 31.

node/src/crypto/crypto_random.cc

Lines 54 to 57 in 4f387c2

	if (UNLIKELY(size > INT_MAX)) {
	THROW_ERR_OUT_OF_RANGE(env, "buffer is too large");
	return Nothing<bool>();
	}

But v14 is out of luck to have a C++ guard, therefore aborts when passing 2 ** 31.