doc: revise security-reporting example text #23759

Trott · 2018-10-19T18:08:48Z

Edit for simplicity and clarity.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Edit for simplicity and clarity.

nodejs-github-bot · 2018-10-19T18:08:49Z

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1297/pipeline

Trott · 2018-10-19T18:10:23Z

@nodejs/security-wg @nodejs/security-triage

bnoordhuis · 2018-10-19T21:21:58Z

README.md

+  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.


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

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".)

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.

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.)

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

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

Edit for simplicity and clarity. PR-URL: nodejs#23759 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Trott · 2018-10-22T01:49:50Z

Landed in 2fc0752

Remove Buffer constructor example from security reporting examples. Even though the example text focuses on API compatibility, the pull request cited is about zero-filling vs. not zero-filling, which is not an API compatibility change (or at least is not unambiguously one). The fact that it's a pull request is also problematic, since it's not reporting a security issue but instead proposing a way to address one that has already been reported publicly. Finally, the text focuses on the fact that it was not deemed worth of backporting, but that was determined by a vote by a divided CTC. It is unreasonable to ask someone reporting an issue to make a determination that the CTC/TSC is divided on. In short, it's not a good example for the list it is in. Remove it. Refs: nodejs#23759 (comment)

Edit for simplicity and clarity. PR-URL: #23759 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Remove Buffer constructor example from security reporting examples. Even though the example text focuses on API compatibility, the pull request cited is about zero-filling vs. not zero-filling, which is not an API compatibility change (or at least is not unambiguously one). The fact that it's a pull request is also problematic, since it's not reporting a security issue but instead proposing a way to address one that has already been reported publicly. Finally, the text focuses on the fact that it was not deemed worth of backporting, but that was determined by a vote by a divided CTC. It is unreasonable to ask someone reporting an issue to make a determination that the CTC/TSC is divided on. In short, it's not a good example for the list it is in. Remove it. Refs: nodejs#23759 (comment) PR-URL: nodejs#23817 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Remove Buffer constructor example from security reporting examples. Even though the example text focuses on API compatibility, the pull request cited is about zero-filling vs. not zero-filling, which is not an API compatibility change (or at least is not unambiguously one). The fact that it's a pull request is also problematic, since it's not reporting a security issue but instead proposing a way to address one that has already been reported publicly. Finally, the text focuses on the fact that it was not deemed worth of backporting, but that was determined by a vote by a divided CTC. It is unreasonable to ask someone reporting an issue to make a determination that the CTC/TSC is divided on. In short, it's not a good example for the list it is in. Remove it. Refs: #23759 (comment) PR-URL: #23817 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Edit for simplicity and clarity. PR-URL: #23759 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Remove Buffer constructor example from security reporting examples. Even though the example text focuses on API compatibility, the pull request cited is about zero-filling vs. not zero-filling, which is not an API compatibility change (or at least is not unambiguously one). The fact that it's a pull request is also problematic, since it's not reporting a security issue but instead proposing a way to address one that has already been reported publicly. Finally, the text focuses on the fact that it was not deemed worth of backporting, but that was determined by a vote by a divided CTC. It is unreasonable to ask someone reporting an issue to make a determination that the CTC/TSC is divided on. In short, it's not a good example for the list it is in. Remove it. Refs: #23759 (comment) PR-URL: #23817 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Edit for simplicity and clarity. PR-URL: #23759 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Remove Buffer constructor example from security reporting examples. Even though the example text focuses on API compatibility, the pull request cited is about zero-filling vs. not zero-filling, which is not an API compatibility change (or at least is not unambiguously one). The fact that it's a pull request is also problematic, since it's not reporting a security issue but instead proposing a way to address one that has already been reported publicly. Finally, the text focuses on the fact that it was not deemed worth of backporting, but that was determined by a vote by a divided CTC. It is unreasonable to ask someone reporting an issue to make a determination that the CTC/TSC is divided on. In short, it's not a good example for the list it is in. Remove it. Refs: #23759 (comment) PR-URL: #23817 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Edit for simplicity and clarity. PR-URL: #23759 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Remove Buffer constructor example from security reporting examples. Even though the example text focuses on API compatibility, the pull request cited is about zero-filling vs. not zero-filling, which is not an API compatibility change (or at least is not unambiguously one). The fact that it's a pull request is also problematic, since it's not reporting a security issue but instead proposing a way to address one that has already been reported publicly. Finally, the text focuses on the fact that it was not deemed worth of backporting, but that was determined by a vote by a divided CTC. It is unreasonable to ask someone reporting an issue to make a determination that the CTC/TSC is divided on. In short, it's not a good example for the list it is in. Remove it. Refs: #23759 (comment) PR-URL: #23817 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

doc: revise security-reporting example text

0fd4f2d

Edit for simplicity and clarity.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 19, 2018

vsemozhetbyt added the security Issues and PRs related to security. label Oct 19, 2018

sam-github approved these changes Oct 19, 2018

View reviewed changes

mmarchini approved these changes Oct 19, 2018

View reviewed changes

vdeturckheim approved these changes Oct 19, 2018

View reviewed changes

bnoordhuis approved these changes Oct 19, 2018

View reviewed changes

Trott closed this Oct 22, 2018

Trott mentioned this pull request Oct 22, 2018

doc: remove problematic example from README #23817

Closed

3 tasks

MylesBorins added dont-land-on-v8.x labels Nov 26, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

Trott deleted the revise-security branch January 13, 2022 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: revise security-reporting example text #23759

doc: revise security-reporting example text #23759

Trott commented Oct 19, 2018

nodejs-github-bot commented Oct 19, 2018

Trott commented Oct 19, 2018 •

edited

Loading

bnoordhuis Oct 19, 2018

Trott Oct 19, 2018 •

edited

Loading

Trott Oct 19, 2018

Trott Oct 19, 2018 •

edited

Loading

mcollina Oct 21, 2018

Trott Oct 22, 2018

Trott commented Oct 22, 2018

doc: revise security-reporting example text #23759

doc: revise security-reporting example text #23759

Conversation

Trott commented Oct 19, 2018

Checklist

nodejs-github-bot commented Oct 19, 2018

Trott commented Oct 19, 2018 • edited Loading

bnoordhuis Oct 19, 2018

Choose a reason for hiding this comment

Trott Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Trott Oct 19, 2018

Choose a reason for hiding this comment

Trott Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

mcollina Oct 21, 2018

Choose a reason for hiding this comment

Trott Oct 22, 2018

Choose a reason for hiding this comment

Trott commented Oct 22, 2018

Trott commented Oct 19, 2018 •

edited

Loading

Trott Oct 19, 2018 •

edited

Loading

Trott Oct 19, 2018 •

edited

Loading