Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix uninitialized memory in buffers #4706

Closed

Conversation

thejefflarson
Copy link

This commit fixes an uninitialized memory bug #4660, which will cause sensitive data to be exposed by array buffers. The current behavior is not only insecure, but contrary to what the v8 api expects of ArrayBuffer::Allocator.

The v8 documentation, clearly states that ArrayBuffer::Allocator implementations must return zero-initialized memory from a call to Allocate.

This commit fixes an uninitialized memory bug nodejs#4660, which will
cause sensitive data to be exposed by array buffers. The current
behavior is not only insecure, but contrary to what the v8 api
expects of ArrayBuffer::Allocator.

The v8 documentation [1], clearly states that ArrayBuffer::Allocator
implementations must return zero-initialized memory from a call to
Allocate.
@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. buffer Issues and PRs related to the buffer subsystem. ctc-agenda labels Jan 15, 2016
@rvagg
Copy link
Member

rvagg commented Jan 15, 2016

Sorry to be brutal about this but this is extremely unlikely to be merged. The current code is intentional and the interaction with the current V8 was built as part of a negotiation with the V8 team in order to properly support Node's intended Buffer API. A Buffer is not an ArrayBuffer but it now uses ArrayBuffer for backing because V8 deprecated the previous API with which we used to attach the custom objects to V8 objects, the association between the two is purely a result of being backed into a corner by the V8 team on the API. Using ArrayBuffer API expectations to enforce new behaviour on Buffer doesn't make sense from that perspective. If you want an ArrayBuffer in Node.js then you can allocate them and get the same semantics as the browser. If you want to use Buffer then you have to accept the semantics as they are currently defined.

There are some very complex factors involved in moving to such a change and it's not going to be done as lightly as this if it's done at all. Node is not a browser and cannot be held to the same constraints as the browser, which is what V8 is designed for. Following the same logic of safety we would be removing swathes of core APIs to turn Node into a sandboxed server environment.

If you wish to participate in the ongoing discussion then feel free to join in on #4660.

@thejefflarson
Copy link
Author

@rvagg Saw a serious problem and fixed it. JavaScript is supposed to be memory safe and the current behavior exposes server-side secrets for no conceivable gain. I simply cannot understand why a user of a higher level language would need to worry about the details of initialized vs uninitialized memory. I guess I'm not privy to those discussions, and maybe the vulnerability doesn't worry you?

@rvagg
Copy link
Member

rvagg commented Jan 15, 2016

I'm not going to be baited in trying to rehash everything that's been covered in #4660 and other issues on this matter. I'd recommend limiting the discussion to there.

@Fishrock123
Copy link
Contributor

I think one of these issues on the ctc-agenda is enough.

@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

I second @rvagg's -1 on this particular PR. I see no possibility that this variant of the proposed changes would be accepted.

@mscdex
Copy link
Contributor

mscdex commented Jan 15, 2016

-1

@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

Given the -1's, I'm closing this. The issue is still open, however, and there are other alternative solutions still being discussed. @thejefflarson, even tho this PR isn't landing, I definitely thank you for the contribution.

@nodejs/ctc - if any of you feel this should be reopened, feel free :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants