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.

Sign up for GitHub

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

doc: revise security-reporting example text #23759

Closed
wants to merge 1 commit into from
Closed

doc: revise security-reporting example text #23759

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 7 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,17 +175,15 @@ nonetheless.
### Public disclosure preferred

- [#14519](https://github.com/nodejs/node/issues/14519): _Internal domain
function can be used to cause segfaults_. Causing program termination using
either the public JavaScript APIs or the private bindings layer APIs requires
the ability to execute arbitrary JavaScript code, which is already the highest
level of privilege possible.
function can be used to cause segfaults_. Requires the ability to execute
arbitrary JavaScript code. That is already the highest level of privilege
possible.

- [#12141](https://github.com/nodejs/node/pull/12141): _buffer: zero fill
Buffer(num) by default_. The buffer constructor behavior was documented,
but found to be prone to [mis-use](https://snyk.io/blog/exploiting-buffer/).
It has since been changed, but despite much debate, was not considered misuse
prone enough to justify fixing in older release lines and breaking our
API stability contract.
Buffer(num) by default_. The documented `Buffer()` behavior was prone to
[misuse](https://snyk.io/blog/exploiting-buffer/). It has since changed. It
was not deemed serious enough to fix in older releases and breaking API
stability.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "breaking API stability" -> "changing the API in a backwards incompatible way"?

Copy link
Member Author

@Trott Trott Oct 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even really true? (I'm talking about both the existing text and your revision.) The issue was zero-filling vs. not zero-filling, but whether that broke API compatibility was (and is) up for debate. IMO, there was never any guarantee about the contents of a buffer created that way, so this did not break API compatibility. Even the one person saying it broke API compatibility in nodejs/CTC#91 hedged. (@rvagg described it as "technically breaking".)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's even more confusing because it's a pull request and not an issue. I wonder if we shouldn't just remove this from the list altogether TBH.

Copy link
Member Author

@Trott Trott Oct 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that it was ultimately deemed not worth backporting was an issue that had divided opinion on the CTC. To expect a reporter to make that sort of judgment call is unfair, I think. Moreover, I'm not sure what that even has to do with whether or not to disclose privately. Maybe the real point here is that the vulnerability was already well-known by the time the pull request was opened. But that's not going to help someone who is wondering whether they should disclose something to us privately or publicly. (IMO, the answer should be: If you're even asking that question, disclose privately.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think Buffer constructor is a good example for this. Maybe can we just remove it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will land as-is and revise this part subsequently, since I'm looking the immediately-following section next...


### Private disclosure preferred

Expand Down