Buffer(number) is unsafe

[feross]

tl;dr

This issue proposes:

1. Change new Buffer(number) to return safe, zeroed-out memory
2. Create a new API for creating uninitialized Buffers, Buffer.alloc(number)

Update: Jan 15, 2016

Upon further consideration, I think that returning zeroed out memory is a separate issue. The core issue is: unsafe buffer allocation should be in a different API.

I now support adding two APIs:

• Buffer.from(value) - convert from any type to a buffer
• Buffer.alloc(size) - create an uninitialized buffer with given size

This solves the core problem that affected ws and bittorrent-dht which is Buffer(variable) getting tricked into taking a number argument.

Why is Buffer unsafe?

Today, the node.js Buffer constructor is overloaded to handle many different argument types like String, Array, Object, TypedArrayView (Uint8Array, etc.), ArrayBuffer, and also Number.

The API is optimized for convenience: you can throw any type at it, and it will try to do what you want.

Because the Buffer constructor is so powerful, you often see code like this:

// Convert UTF-8 strings to hex
function toHex (str) {
  return new Buffer(str).toString('hex')
}

_But what happens if toHex is called with a Number argument?_

Remote Memory Disclosure

If an attacker can make your program call the Buffer constructor with a Number argument, then they can make it allocate uninitialized memory from the node.js process. This could potentially disclose TLS private keys, user data, or database passwords.

When the Buffer constructor is passed a Number argument, it returns an UNINITIALIZED block of memory of the specified size. When you create a Buffer like this, you MUST overwrite the contents before returning it to the user.

Would this ever be a problem in real code?

Yes. It's surprisingly common to forget to check the type of your variables in a dynamically-typed language like JavaScript.

Usually the consequences of assuming the wrong type is that your program crashes with an uncaught exception. But the failure mode for forgetting to check the type of arguments to the Buffer constructor is more catastrophic.

Here's an example of a vulnerable service that takes a JSON payload and converts it to hex:

// Take a JSON payload {str: "some string"} and convert it to hex
var server = http.createServer(function (req, res) {
  var data = ''
  req.setEncoding('utf8')
  req.on('data', function (chunk) {
    data += chunk
  })
  req.on('end', function () {
    var body = JSON.parse(data)
    res.end(new Buffer(body.str).toString('hex'))
  })
})

server.listen(8080)

In this example, an http client just has to send:

{
  "str": 1000
}

and it will get back 1,000 bytes of uninitialized memory from the server.

This is a very serious bug. It's similar in severity to the the Heartbleed bug that allowed disclosure of OpenSSL process memory by remote attackers.

Which real-world packages were vulnerable?

bittorrent-dht

@mafintosh and I found this issue in one of our own packages, bittorrent-dht. The bug would allow anyone on the internet to send a series of messages to a user of bittorrent-dht and get them to reveal 20 bytes at a time of uninitialized memory from the node.js process.

Here's the commit that fixed it. We released a new fixed version, created a Node Security Project disclosure, and deprecated all vulnerable versions on npm so users will get a warning to upgrade to a newer version.

ws

That got us wondering if there were other vulnerable packages. Sure enough, within a short period of time, we found the same issue in ws, the most popular WebSocket implementation in node.js.

If certain APIs were called with Number parameters instead of String or Buffer as expected, then uninitialized server memory would be disclosed to the remote peer.

These were the vulnerable methods:

socket.send(number)
socket.ping(number)
socket.pong(number)

Here's a vulnerable socket server with some echo functionality:

server.on('connection', function (socket) {
  socket.on('message', function (message) {
    message = JSON.parse(message)
    if (message.type === 'echo') {
      socket.send(message.data) // send back the user's message
    }
  })
})

socket.send(number) called on the server, will disclose server memory.

Here's the release where the issue was fixed, with a more detailed explanation. Props to @3rd-Eden for the quick fix. Here's the Node Security Project disclosure.

What's the solution?

It's important that node.js offers a fast way to get memory otherwise performance-critical applications would needlessly get a lot slower.

But we need a better way to signal our intent as programmers. When we want uninitialized memory, we should request it explicitly.

Sensitive functionality should not be packed into a developer-friendly API that loosely accepts many different types. This type of API encourages the lazy practice of passing variables in without checking the type very carefully.

Buffer.alloc(number)

The functionality of creating buffers with uninitialized memory should be part of another API. We propose Buffer.alloc(number). This way, it's not part of an API that frequently gets user input of all sorts of different types passed into it.

var buf = Buffer.alloc(16) // careful, uninitialized memory!

// Immediately overwrite the uninitialized buffer with data from another buffer
for (var i = 0; i < buf.length; i++) {
  buf[i] = otherBuf[i]
}

How do we fix node.js core?

We sent a PR (merged as semver-major) which defends against one case:

var str = 16
new Buffer(str, 'utf8')

In this situation, it's implied that the programmer intended the first argument to be a string, since they passed an encoding as a second argument. Today, node.js will allocate uninitialized memory in the case of new Buffer(number, encoding), which is probably not what the programmer intended.

But this is only a partial solution, since if the programmer does new Buffer(variable) (without an encoding parameter) there's no way to know what they intended. If variable is sometimes a number, then uninitialized memory will sometimes be returned.

What's the real long-term fix?

We could deprecate and remove new Buffer(number) and use Buffer.alloc(number) when we need uninitialized memory. But that would break 1000s of packages. So that's a no-go.

Instead, we believe the best solution is to:

1. Change new Buffer(number) to return safe, zeroed-out memory
2. Create a new API for creating uninitialized Buffers. We propose: Buffer.alloc(number)

This way, existing code continues working and the impact on the npm ecosystem will be minimal. Over time, npm maintainers can migrate performance-critical code to use Buffer.alloc(number) instead of new Buffer(number).

Conclusion

We think there's a serious design issue with the Buffer API as it exists today. It promotes insecure software by putting high-risk functionality into a convenient API with friendly "developer ergonomics".

This wasn't merely a theoretical exercise because we found the issue in some of the most popular npm packages.

Eventually, we hope that node.js core can switch to this new, safer behavior. We believe the impact on the ecosystem would be minimal since it's not a breaking change. Well-maintained, popular packages would be updated to use Buffer.alloc quickly, while older, insecure packages would magically become safe from this attack vector.

[feross]

For those interested in getting the new behavior we propose without a change to node core, we published a user-land package: safe-buffer.

[JacksonTian]

Now, the new Buffer(number, encoding) will throw exception, detail is here: 3b27dd5 . I introduce a new API Buffer.encode() for encoding case.

[rvagg]

You could make similar arguments about child_process being unsafe, about loading native addons being unsafe, even about fs being unsafe. The more bubble-wrap we put in core, the more users are likely to make the false assumption that programming in Node.js is like programming in the browser, which it absolutely is not. That's not a -1 to this proposal (in fact, this exact discussion is an ongoing on that keeps on coming up, we have an active thread in the security repo to thrash this out too), just a word of caution because we keep on having discussions like this where we want to move closer to a sandbox but that will never be the case for Node and it would be even more unsafe to lull users into the perception that they are as protected as in the browser (there's a very good chance that's exactly what happened to TrendMicro).

It should be noted that the primary reason for not zero-filling by default is performance. So new Buffer() should be considered a form of malloc(). In the past, it has been demonstrated that allocating filled memory has significant performance penalties to a plain malloc(). I keep on suggesting that those who are advocating switching the default to a clean allocation should do some benchmarking to demonstrate what kind of impact it might have. Otherwise, this argument is dead-in-the-water because the "it's faster" argument currently holds this one in place.

[trevnorris]

This discussion has been had several times in the last few weeks.

If a "high-risk functionality" application doesn't have proper type checking in place then it already has a security issue. This problem can't be dished into Buffer as if it's at fault.

Not zero filling the Buffer is completely a performance issue, but the impact on performance is more than trivial. Even at allocations as low as 1KB. And while this type of impact probably wouldn't harm your standard web app, it can noticeably affect performance of a node process.

The concern is when a number is passed when something else is expected. Again this is a bug in the developers code, and TBH it doesn't make much sense to impact the performance of the many modules out there today and force them to update their code to use new syntax because of that.

And let's be honest, in the years that Buffer has worked exactly this way how many times has this been reportedly seen as an issue in the wild. The only reason you're here now is because it affected modules you were directly involved with.

[feross]

@rvagg @trevnorris You both raise good points. Node is not the browser, and performance is critical.

The difference with modules like child_process and fs is that they're very obviously doing powerful and sensitive operations. Even the most trivial use of them will confront the developer with the fact that they're interacting with their OS on a low level and need to be careful.

Buffer is different. 95% of the time, it's safe and idiot-proof. 5% of the time you need to be very, very careful.

You can pass in:

• Array –> safe
• Buffer –> safe
• ArrayBuffer –> safe
• Uint8Array –> safe
• Uint16Array –> safe
• the rest of the typed array types –> safe
• Any array-like –> safe
• Object (reverse of .toJSON()) –> safe
• String –> safe
• Number –> _SURPRISE! REMOTE MEMORY DISCLOSURE!_

The API seems like it's trying to lay a trap for the user.

Look at the contributors on each of those repos. Some of the most talented node.js developers looked at this code extensively and didn't notice this issue for years.

[feross]

And while this type of impact probably wouldn't harm your standard web app, it can noticeably affect performance of a node process.

To be clear, I'm not suggesting that node core should use zeroed-out buffers. It would continue using unallocated buffers, but that functionality would move to a different API away from the one that looks and behaves (and now actually is) a Uint8Array.

[mscdex]

-1 to changing new Buffer(number) to zero-out by default. Even if there was a separate API to preserve the behavior, that means a lot of code change for a lot of packages and application code. I'd personally be more open to a separate method that created zeroed-out Buffers, that way it's an explicit opt-in and you are (or should be) at that point aware of the tradeoff you're making.

[ChALkeR]

separate method that created zeroed-out Buffers

That would not fix anything, there is already a new Buffer(number).fill(0).

[silverwind]

I see around 20-25% perf regression using .fill() on small buffers (on ARM here):

node -e "for(var i = 1e6; i >= 0; i--) Buffer(1e3)"  6.16s user 0.19s system 100% cpu 6.287 total
node -e "for(var i = 1e6; i >= 0; i--) Buffer(1e3)"  6.10s user 0.19s system 101% cpu 6.222 total
node -e "for(var i = 1e6; i >= 0; i--) Buffer(1e3)"  5.97s user 0.17s system 101% cpu 6.074 total

node -e "for(var i = 1e6; i >= 0; i--) Buffer(1e3).fill()"  8.41s user 0.16s system 100% cpu 8.504 total
node -e "for(var i = 1e6; i >= 0; i--) Buffer(1e3).fill()"  7.89s user 0.14s system 100% cpu 7.968 total
node -e "for(var i = 1e6; i >= 0; i--) Buffer(1e3).fill()"  7.94s user 0.24s system 100% cpu 8.111 total

[feross]

I see around 20-25% perf regression using .fill() on small buffers

Nice.

Remember too, this is not a 20-25% perf regression across the board. It's only going to apply when all of these conditions are true:

• it's user-land code
• they're calling new Buffer(number) because they need uninitialized memory
• and, they don't upgrade to Buffer.alloc()

Node.js core would not be slower. Uses of new Buffer(string), new Buffer(array), etc. would not be slower.

[silverwind]

I think I'm in support of this, but I'm not aware of all the use cases for Buffer(number), so take that with a grain of salt.

Instead of introducing new API, how about an opt-in?

Buffer(100); // safe
Buffer.unsafe = true;
Buffer(100); // unsafe

[okdistribute]

Thanks feross for explaining this well, I learned something tonight! I am relatively new to Node - I gained my open source chops in Python land. In Python, slow performance is sort of a given. If you want to make a Python program more performant, you can optimize it in certain ways, which are well known. Performance then is an advanced problem. The old adage "make it work, then make it fast" works well for these kinds of dynamic, interpreted languages. After all, we are not writing in C here, and the beauty of languages like Node and Python is the ease with which external, more performant packages can be accessed when necessary.

20-25% is actually quite a lot slower, true -- but how many calls to Buffer are using Buffer(Number) and require high performance? It seems as though this being fixed in core will bring great gratitude from newcomers and old hats alike. As a relatively new node programmer, I am certainly grateful - my memory won't be stolen!