-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Fix uninitialized memory in buffers #4706
Conversation
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.
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 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. |
@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? |
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. |
I think one of these issues on the |
I second @rvagg's -1 on this particular PR. I see no possibility that this variant of the proposed changes would be accepted. |
-1 |
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 :-) |
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 toAllocate
.