safe-buffer
Safer Node.js Buffer API
install
npm install safe-buffer
usage
The goal of this package is to provide a safe replacement for the node.js Buffer
.
It's a drop-in replacement for Buffer
. You can use it by adding one require
line to
the top of your node.js modules:
var Buffer = require('safe-buffer')
new Buffer('hey', 'utf8')
new Buffer([1, 2, 3], 'utf8')
new Buffer(obj)
new Buffer(16)
Buffer.alloc(16)
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:
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.
From the node.js docs:
new Buffer(size)
The underlying memory for Buffer
instances created in this way is not initialized.
The contents of a newly created Buffer
are unknown and could contain sensitive
data. Use buf.fill(0)
to initialize a Buffer to zeroes.
(Emphasis our own.)
Whenever the programmer intended to create an uninitialized Buffer
you often see code
like this:
var buf = new Buffer(16)
for (var i = 0; i < buf.length; i++) {
buf[i] = otherBuf[i]
}
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:
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?
Mathius Buus and I
(Feross Aboukhadijeh) 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.
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)
}
})
})
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
Arnout Kazemier 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)
for (var i = 0; i < buf.length; i++) {
buf[i] = otherBuf[i]
}
How do we fix node.js core?
We sent a PR to node.js core (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.
We believe the best solution is to:
-
Change new Buffer(number)
to return safe, zeroed-out memory
-
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.
Fortunately, there's an easy fix that can be applied today. Use safe-buffer
in place of
buffer
.
var Buffer = require('safe-buffer')
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.
links
credit
The original issues in bittorrent-dht
(disclosure) and
ws
(disclosure) were discovered by
Mathias Buus and
Feross Aboukhadijeh.
Thanks to Adam Baldwin for helping disclose these issues
and for his work running the Node Security Project.
Thanks to John Hiesey for proofreading this README and
auditing the code.
license
MIT. Copyright (C) Feross Aboukhadijeh