Skip to content

crypto.pseudoRandomBytes results in a segmentation fault #38090

Description

@zyscoder

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

Activity

  1. added
    cryptoIssues and PRs related to the crypto subsystem.
    on Apr 5, 2021
  2. Ayase-252 commented on Apr 5, 2021

    @Ayase-252
    Member

    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
    
  3. aduh95 commented on Apr 5, 2021

    @aduh95
    Contributor

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

  4. apple502j commented on Apr 5, 2021

    @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.
    
  5. zyscoder commented on Apr 5, 2021

    @zyscoder
    Author

    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.

  6. aduh95 commented on Apr 5, 2021

    @aduh95
    Contributor

    @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.

  7. Ayase-252 commented on Apr 5, 2021

    @Ayase-252
    Member

    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.

  8. aduh95 commented on Apr 5, 2021

    @aduh95
    Contributor

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

  9. Ayase-252 commented on Apr 5, 2021

    @Ayase-252
    Member

    @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

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

    Let's see assertSize

    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

    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.

    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.

  10. added a commit that references this issue on May 1, 2021
  11. Ayase-252 commented on May 13, 2021

    @Ayase-252
    Member

    Closing as #38096 has been backported to v14.x, it should be fixed.

  12. FossPrime commented on May 14, 2021

    @FossPrime

    I think this is the same issue, but it's happening in Node 16.1.0

    When I run: node --experimental-repl-await and paste:

    const UDEF = typeof undefined
    const sha256 = async (str) => {
      const self = {} // Isomorphic polyfill, for node, repl, window, workers
      self.crypto = typeof crypto === UDEF || crypto.webcrypto ? (await import('crypto')).webcrypto : crypto
      self.TextEncoder = typeof TextEncoder === UDEF ? (await import('util')).TextEncoder : TextEncoder
    
      // Browser code will work with or without self.
      console.log(self.crypto)
      const buf = await self.crypto.subtle.digest("SHA-256", new self.TextEncoder("utf-8").encode(str))
      return Array.prototype.map.call(new Uint8Array(buf), x=>(('00'+x.toString(16)).slice(-2))).join('')
    }
    await sha256('Hello Mars')
    

    in a project with an "imports" to a non existing folder

    I get a segmentation fault core dump

  13. aduh95 commented on May 14, 2021

    @aduh95
    Contributor

    @rayfoss I'm not able to reproduce on v16.1.0 nor on master (macOS 11.3.1). If you are positive there's another bug, maybe it'd be worth opening a new issue.

  14. FossPrime commented on May 14, 2021

    @FossPrime

    @aduh95 Tracked it down to having an imports to a folder that doesn't exist... not sure why that last line trigger it, but making the last line an assignment doesn't. Regardless, thats not a useful error.

    asciicast

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.cryptoIssues and PRs related to the crypto subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions