Integer overflow in StringBytes::Encode hex leads to a crash or an infinite loop

[srmish-jfrog]

Version

No response

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

Run the following code (node buf_test_1.js):

const buf = Buffer.alloc(0x80000000);
buf.toString('hex');

console.log('Never gets executed.');

It will most likely crash (with an access violation exit code on Windows or with a segmentation fault on Linux) and won’t reach the last line of code.

Now run the following code (node buf_test_2.js):

const buf = Buffer.alloc(0xffffffff);
buf.toString('hex');

console.log('Never gets executed.');

This code will run infinitely in a loop and will never end.
These issues also manifest when running untrusted code in a sandboxed vm, which is the more likely scenario for exploitation:

const vm = require('node:vm'); // Also works with "vm2"

vm.runInThisContext(`
const buf = Buffer.alloc(0xffffffff);
buf.toString('hex');
`, {timeout:2000});

console.log('Never gets executed.');

Even though a timeout is defined, the code executes forever.

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

Stable reproduction

What is the expected behavior?

Console shows "Never gets executed"

What do you see instead?

The code executes forever (infinite loop)

Additional information

Note - This was first reported as a security issue, but rejected since the main attack scenario is when using the vm module

Summary:
Due to a type mismatch between variables in the string_bytes module, an integer overflow occurs, which can cause a crash or an infinite loop.

Description:
The code in string_bytes.cc encodes and decodes different types of string encodings. In the StringBytes::Encode function, there is a case that takes care of “hex” encoding:

    case HEX: {
      size_t dlen = buflen * 2;
      char* dst = node::UncheckedMalloc(dlen);
      if (dst == nullptr) {
        *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate);
        return MaybeLocal<Value>();
      }
      size_t written = hex_encode(buf, buflen, dst, dlen);
      CHECK_EQ(written, dlen);

      return ExternOneByteString::New(isolate, dst, dlen, error);
    }

The “dlen” variable created in this code is of type “size_t”. It is passed to the StringBytes::hex_encode function, which looks like this:

size_t StringBytes::hex_encode(
    const char* src,
    size_t slen,
    char* dst,
    size_t dlen) {
  // We know how much we'll write, just make sure that there's space.
  CHECK(dlen >= slen * 2 &&
      "not enough space provided for hex encode");

  dlen = slen * 2;
  for (uint32_t i = 0, k = 0; k < dlen; i += 1, k += 2) {
    static const char hex[] = "0123456789abcdef";
    uint8_t val = static_cast<uint8_t>(src[i]);
    dst[k + 0] = hex[val >> 4];
    dst[k + 1] = hex[val & 15];
  }

  return dlen;
}

Also here the type of “dlen” is “size_t”, but the types of the variables “i” and “k” are “uint32_t”. In 64-bit operating systems, “size_t” is equivalent to a 64-bit unsigned integer. In a case that “dlen” is more than 32 bits long (for example 0x100000000), the variable “k” will be increased until it reaches the maximum value for an unsigned 32-bit integer (0xfffffffe since “k” increases by 2 in each iteration), and will the wrap around to 0. The expression “k < dlen” in the “for” statement will always be true, meaning that the loop runs infinitely. Since the array “src” is being read in the index “i” (which increases by 1 in every iteration, hence not wrapping around at the same time as “k”), at some point it will try reading from a memory address that doesn’t belong to “src”, and will eventually crash.
The variable “dlen” is equal to “src”’s length times 2. For example, if “src”’s length is 0x80000000, “dlen” will be equal to 0x100000000, causing a crash at some point after the variable “i” passes 0x80000000.
An infinite loop without crashing exists in the case that src’s length is 0x100000000, meaning “dlen” is 0x200000000. In this case both “i” and “k” keep wrapping forever, and the code never finishes running.
These code flows are not likely to appear organically, but in cases where there is use of the “vm” module to run untrusted code in a sandbox, these situations could be real and easily cause crashes or infinite loops. “vm”’s timeout option does not work in this case. Reaching this vulnerable code is possible by using the Buffer object, which is enabled by default in both “vm” and “vm2” modules without the need to use any external modules.

[bnoordhuis]

Want to send a pull request? For background, that code was written at a time when node / V8 didn't support buffers larger than 1 or 2 GB. Subsequent refactors then switched some things to size_t but not everything.

[srmish-jfrog]

The required change would be changing "i" and "k" to size_t, I didn't submit a PR because I'm not 100% sure of all the side effects of this change. Alternatively, we can return an error if dlen is larger than 0xFFFFFFFF

[bnoordhuis]

Switching to size_t should be sufficient and gives the desired behavior. Buffers > 4 GB are possible nowadays.