{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/9531","repository_url":"https://api.github.com/repos/nodejs/node","labels_url":"https://api.github.com/repos/nodejs/node/issues/9531/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/9531/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/9531/events","html_url":"https://github.com/nodejs/node/issues/9531","id":188351787,"number":9531,"title":"buffer: discuss future direction of Buffer constructor API","user":{"login":"addaleax","id":899444,"avatar_url":"https://avatars.githubusercontent.com/u/899444?v=3","gravatar_id":"","url":"https://api.github.com/users/addaleax","html_url":"https://github.com/addaleax","followers_url":"https://api.github.com/users/addaleax/followers","following_url":"https://api.github.com/users/addaleax/following{/other_user}","gists_url":"https://api.github.com/users/addaleax/gists{/gist_id}","starred_url":"https://api.github.com/users/addaleax/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/addaleax/subscriptions","organizations_url":"https://api.github.com/users/addaleax/orgs","repos_url":"https://api.github.com/users/addaleax/repos","events_url":"https://api.github.com/users/addaleax/events{/privacy}","received_events_url":"https://api.github.com/users/addaleax/received_events","type":"User","site_admin":false},"labels":[{"id":155267414,"url":"https://api.github.com/repos/nodejs/node/labels/buffer","name":"buffer","color":"f7c6c7","default":false},{"id":171787081,"url":"https://api.github.com/repos/nodejs/node/labels/discuss","name":"discuss","color":"02d7e1","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":46,"created_at":"2016-11-09T21:03:01Z","updated_at":"2016-11-23T22:37:16Z","closed_at":null,"body":"In today’s CTC meeting we discussed reverting the `DeprecationWarning` for calling `Buffer` without `new` that was introduced in `v7` (PR up [here](https://github.com/nodejs/node/pull/9529)), and it became clear that we need to come up with a long-term plan on what exactly we want to achieve, how to do that and improve our messaging about it, both before and after our actions. I’ll *try* to sum up what exactly we are talking about; obviously, I am somewhat biased, having been involved in plenty of the previous discussion here. (This has still gotten pretty long btw, so I hope a lot of people will find the information in here useful enough to warrant a Wall of Text.)\r\n\r\nThe `Buffer` constructor has the usability flaw that it accepts input with different type signatures, so `new Buffer('abcdef')` and `new Buffer(100)` will both return valid buffers, and in the latter case, the `Buffer` will contain 100 bytes of unitialized memory. This is a security problem for two reasons:\r\n\r\n1. When passing unvalidated user input (e.g. from a JSON request) to the `Buffer` constructor where a string is expected but a number is actually passed, uninitialized memory will be returned:\r\n\r\n```js\r\n// This is a dangerous example of converting a string to Base64!\r\nnew Buffer(someStringReceivedFromTheUser).toString('base64')\r\n```\r\n\r\nPassing the value `100` here will return a slice of memory that may contain garbage, but generally can contain any value previously stored in memory, including credentials, source code, and much more. @ChALkeR has a pretty good write-up of this: https://github.com/ChALkeR/notes/blob/master/Buffer-knows-everything.md\r\n\r\n2. Accidentally accepting large numeric values can very quickly increase resource usage, and can be turned into a Denial-of-Service attack against vulnerable applications.\r\n\r\nAgain, @ChALkeR has a very-good write-up on these security issues at https://github.com/ChALkeR/notes/blob/master/Lets-fix-Buffer-API.md. It predates the current `Buffer.alloc()`/`Buffer.from()` situation, but it contains a helpful FAQ with answers to questions like “Why not just make `Buffer(number)` zero-fill everything by default?”.\r\n\r\nSo far, in Node v6.0.0 the safer `Buffer.alloc()`/`Buffer.from()` API was introduced and later backported to the v5.10.0 and v4.5.0 releases. Additionally, v6.0.0 came with a documentation-only deprecation of the old `Buffer()` API.\r\n\r\nIn June, https://github.com/nodejs/node/pull/7152 was opened, which seeks to deprecate the old `Buffer()` API using a runtime deprecation, i.e. printing a single warning per Node process when `Buffer()` or `new Buffer()` is executed for the first time. Currently, that PR is still open. A reduced version of it, https://github.com/nodejs/node/pull/8169, was landed as a semver-major change in v7.0.0, that emits and displays `DeprecationWarning`s for uses of `Buffer()` only, but excludes uses of `new Buffer()`.\r\n\r\nI had summarized the goals and possible actions before that decision was made in https://github.com/nodejs/node/pull/7152#issuecomment-241355246 ¹; And @jasnell has written a then-current long-term plan in https://github.com/nodejs/node/pull/7152#issuecomment-240753218 that would include runtime deprecations of `new Buffer()` in v8.0.0 and later actual breaking changes to the `Buffer` constructor.\r\n\r\nThe reason for this distinction was trying to keep the possibility of making `Buffer` a proper ES6 class at some point in the far, far future open, which would mean that calling `new Buffer()` *may* always work. (Effects of turning `Buffer` into a class would be proper subclassability and breaking `Buffer()` without `new`. It is, however, completely possible to add a separate class to the API that would behave like the current `Buffer` implementation does, only with these differences.)\r\n\r\nAs a result of that deprecation for `Buffer()` without `new` in v7.0.0, significant pushback from well-known members of the community ensued, both in the threads on https://github.com/nodejs/node/pull/7152 and https://github.com/nodejs/node/pull/8169. On the one hand, it became clear that we failed in our messaging to make clear that the primary motivation for that change was helping our users avoid serious security issues; on the other hand, the added deprecation warning seemed to be incongruent with the expectations of stability and backwards compatibility that module authors and consumers have, as far as Node core is concerned.\r\n\r\nAs a result of this, the CTC decided to consider reverting the deprecation warning, possibly temporarily, and the corresponding PR is in https://github.com/nodejs/node/pull/9529. The decision on that has yet to be made, but the desire has been expressed to reach a decision soon to limit the number of v7.x versions with possibly incongruent behaviour.\r\n\r\nFrom following the discussions, it is obvious that the path forward is a contentious issue; right now, the opinions range from *never* introducing a runtime deprecation for any version of the `Buffer` constructor, to applying one for all uses of it at the next semver-major release in v8.0.0.\r\n\r\nThe strongest and most frequently expressed argument for fully runtime-deprecating the `Buffer` constructor soon remains that users may not be aware that parts of their application use an unsafe API and should be warned about that.\r\n\r\nOn the other side, the warning itself is perceived as a very disruptive change to the ecosystem, suggesting that it is definitely worth exploring alternative ways to reduce the usage of both `Buffer()` and `new Buffer()`.\r\n\r\n/cc @nodejs/collaborators \r\n\r\n---------\r\n\r\n¹ It may or may not be obvious from the way I articulate my thoughts here – I try to stick to stating facts – but in hindsight, I regret writing it this way."},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/262647023","html_url":"https://github.com/nodejs/node/issues/9531#issuecomment-262647023","issue_url":"https://api.github.com/repos/nodejs/node/issues/9531","id":262647023,"user":{"login":"trevnorris","id":289174,"avatar_url":"https://avatars.githubusercontent.com/u/289174?v=3","gravatar_id":"","url":"https://api.github.com/users/trevnorris","html_url":"https://github.com/trevnorris","followers_url":"https://api.github.com/users/trevnorris/followers","following_url":"https://api.github.com/users/trevnorris/following{/other_user}","gists_url":"https://api.github.com/users/trevnorris/gists{/gist_id}","starred_url":"https://api.github.com/users/trevnorris/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/trevnorris/subscriptions","organizations_url":"https://api.github.com/users/trevnorris/orgs","repos_url":"https://api.github.com/users/trevnorris/repos","events_url":"https://api.github.com/users/trevnorris/events{/privacy}","received_events_url":"https://api.github.com/users/trevnorris/received_events","type":"User","site_admin":false},"created_at":"2016-11-23T22:37:16Z","updated_at":"2016-11-23T22:37:16Z","body":"Apart from all of this, there's a solution for allowing Buffer to be extended that I believe touches outside the security peripheral. Basically it would come down to:\r\n```js\r\nfunction Buffer(arg, encodingOrOffset, length) {\r\n  if (typeof new.target === 'function' && new.target !== Buffer) {\r\n    // continue w/ the old API\r\n  }\r\n  // ...\r\n}\r\n```\r\nI believe allowing the old API to be used, w/o a warning, is specific enough that it wouldn't be a security concern. So the only question in this case is, would it be a strange enough API discrepancy to have the existing API deprecated with simply `new Buffer()` but not when it's extended."}}
{"actor":{"display_login":"trevnorris"}}