René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Buffer(number) is unsafe","articleBody":"### tl;dr\n\nThis issue proposes:\n1. Change `new Buffer(number)` to return safe, zeroed-out memory\n2. Create a new API for creating uninitialized Buffers, `Buffer.alloc(number)`\n### Update: Jan 15, 2016\n\nUpon 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.\n\nI now support adding two APIs:\n- `Buffer.from(value)` - convert from any type to a buffer\n- `Buffer.alloc(size)` - create an uninitialized buffer with given size\n\nThis solves the core problem that affected `ws` and `bittorrent-dht` which is `Buffer(variable)` getting tricked into taking a number argument.\n### Why is `Buffer` unsafe?\n\nToday, the node.js `Buffer` constructor is overloaded to handle many different argument types like `String`, `Array`, `Object`, `TypedArrayView` (`Uint8Array`, etc.), `ArrayBuffer`, and also `Number`.\n\nThe API is optimized for convenience: you can throw any type at it, and it will try to do what you want.\n\nBecause the Buffer constructor is so powerful, you often see code like this:\n\n``` js\n// Convert UTF-8 strings to hex\nfunction toHex (str) {\n  return new Buffer(str).toString('hex')\n}\n```\n\n**_But what happens if `toHex` is called with a `Number` argument?**_\n### Remote Memory Disclosure\n\nIf 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.\n\nWhen 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.\n### Would this ever be a problem in real code?\n\nYes. It's surprisingly common to forget to check the type of your variables in a dynamically-typed language like JavaScript.\n\nUsually 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.\n\nHere's an example of a vulnerable service that takes a JSON payload and converts it to hex:\n\n``` js\n// Take a JSON payload {str: \"some string\"} and convert it to hex\nvar server = http.createServer(function (req, res) {\n  var data = ''\n  req.setEncoding('utf8')\n  req.on('data', function (chunk) {\n    data += chunk\n  })\n  req.on('end', function () {\n    var body = JSON.parse(data)\n    res.end(new Buffer(body.str).toString('hex'))\n  })\n})\n\nserver.listen(8080)\n```\n\nIn this example, an http client just has to send:\n\n``` json\n{\n  \"str\": 1000\n}\n```\n\nand it will get back 1,000 bytes of uninitialized memory from the server.\n\nThis is a very serious bug. It's similar in severity to the [the Heartbleed bug](http://heartbleed.com/) that allowed disclosure of OpenSSL process memory by remote attackers.\n### Which real-world packages were vulnerable?\n#### [`bittorrent-dht`](https://www.npmjs.com/package/bittorrent-dht)\n\n@mafintosh and I found this issue in one of our own packages, [`bittorrent-dht`](https://www.npmjs.com/package/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.\n\nHere's [the commit](https://github.com/feross/bittorrent-dht/commit/6c7da04025d5633699800a99ec3fbadf70ad35b8) that fixed it. We released a new fixed version, created a [Node Security Project disclosure](https://nodesecurity.io/advisories/68), and deprecated all vulnerable versions on npm so users will get a warning to upgrade to a newer version.\n#### [`ws`](https://www.npmjs.com/package/ws)\n\nThat got us wondering if there were other vulnerable packages. Sure enough, within a short period of time, we found the same issue in [`ws`](https://www.npmjs.com/package/ws), the most popular WebSocket implementation in node.js.\n\nIf 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.\n\nThese were the vulnerable methods:\n\n``` js\nsocket.send(number)\nsocket.ping(number)\nsocket.pong(number)\n```\n\nHere's a vulnerable socket server with some echo functionality:\n\n``` js\nserver.on('connection', function (socket) {\n  socket.on('message', function (message) {\n    message = JSON.parse(message)\n    if (message.type === 'echo') {\n      socket.send(message.data) // send back the user's message\n    }\n  })\n})\n```\n\n`socket.send(number)` called on the server, will disclose server memory.\n\nHere's [the release](https://github.com/websockets/ws/releases/tag/1.0.1) 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](https://nodesecurity.io/advisories/67).\n### What's the solution?\n\nIt's important that node.js offers a fast way to get memory otherwise performance-critical applications would needlessly get a lot slower.\n\nBut we need a better way to _signal our intent_ as programmers. **When we want uninitialized memory, we should request it explicitly.**\n\nSensitive 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.\n#### `Buffer.alloc(number)`\n\nThe 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.\n\n``` js\nvar buf = Buffer.alloc(16) // careful, uninitialized memory!\n\n// Immediately overwrite the uninitialized buffer with data from another buffer\nfor (var i = 0; i \u003c buf.length; i++) {\n  buf[i] = otherBuf[i]\n}\n```\n### How do we fix node.js core?\n\nWe sent [a PR](https://github.com/nodejs/node/pull/4514) (merged as `semver-major`) which defends against one case:\n\n``` js\nvar str = 16\nnew Buffer(str, 'utf8')\n```\n\nIn 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.\n\nBut 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.\n### What's the real long-term fix?\n\nWe 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.\n\nInstead, we believe the best solution is to:\n1. Change `new Buffer(number)` to return safe, zeroed-out memory\n2. Create a new API for creating uninitialized Buffers. We propose: `Buffer.alloc(number)`\n\nThis 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)`.\n### Conclusion\n\nWe 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\".\n\nThis wasn't merely a theoretical exercise because we found the issue in some of the most popular npm packages.\n\nEventually, 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.\n","author":{"url":"https://github.com/feross","@type":"Person","name":"feross"},"datePublished":"2016-01-13T05:19:14Z","url":"https://github.com/nodejs/node/issues/4660"}