doc: describe what security issues are #14485

sam-github · 2017-07-25T19:54:57Z

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc,security

Trott · 2017-07-25T19:57:11Z

README.md

+the Node.js application or its system for which the attacker does not already
+have the capability.
+
+To illustrate the point, here are some examples of issues that are not


Nit: "examples" (plural) but there appears to be only one example provided

Trott · 2017-07-25T19:57:57Z

Can we link to a separate doc rather than putting the text right in the README? (Maybe the doc can even live in the security-wg repl?)

sam-github · 2017-07-25T19:58:08Z

This is an attempt to clarify what kinds of issues will or will not be considered "security vulnerabilities" by the Security Response Team. I lifted much of the text from https://www.python.org/news/security/, because Node.js is not a Web Browser, it does not allow (natively) remote code execution, so has a different security model from Browsers, which has occaisonally created confusion.

Trott · 2017-07-25T20:00:27Z

README.md

@@ -173,6 +173,23 @@ Your email will be acknowledged within 24 hours, and you’ll receive a more
 detailed response to your email within 48 hours indicating the next steps in
 handling your report.

+There is not any hard and fast rule to determine if a bug is worth reporting.


Nit: Consider changing worth reporting here and below to something else? If it's a bug, it's worth reporting. It's just a question of whether it's a security issue or whether the bug is in Node.js itself or end-user code.

Maybe change this entire sentence to:

It can be difficult to determine if an issue is a valid security issue or not.

Then change any attack worth reporting via the security address in the next sentence to a valid security issue?

sam-github · 2017-07-25T20:00:58Z

@Trott we can spread our docs as thin as we like, through multiple github repos (nodejs/node, nodejs/LTS, nodejs/security-wg), and through various files, and wikis, but I don't find that helpful.

This is the point at which people are told where to report security issues, its the point they should be told what to report as well, IMO.

There is no current place in the security-wg repo that this information fits. It doesn't have any relationship to any other info in the README (https://github.com/nodejs/security-wg/blob/master/README.md), and I could just dump the text in a standalone .md file and link to it, but that seems a bit odd to me.

Trott · 2017-07-25T20:04:49Z

README.md

+There is not any hard and fast rule to determine if a bug is worth reporting as
+a security issue. The general rule is any issue worth reporting via the
+security address must allow an attacker to affect the confidentiality, integrity
+and availability of the Node.js application or its system for which the attacker


Nit: and -> or?

I think its accurate as is. Its a security issue if it affect the app OR the system the app is on (either one). Its not necessary that it effect the app AND the system.

@sam-github I think you misread me. I'm talking about changing and to or, not the other way around. Right now, it says it's a valid security issue if it affects "the confidentiality, integrity, and availability of the Node.js application...". I think it should say `or availability of the Node.js application...". Like, if you can affect availability, that's a security issue even if it doesn't also affect confidentiality.

I did misreadh, changed.

drifkin · 2017-07-25T20:32:46Z

README.md

@@ -173,6 +173,23 @@ Your email will be acknowledged within 24 hours, and you’ll receive a more
 detailed response to your email within 48 hours indicating the next steps in
 handling your report.

+There is not any hard and fast rule to determine if a bug is worth reporting as
+a security issue. The general rule is any issue worth reporting via the
+security address must allow an attacker to affect the confidentiality, integrity


I think it may be worth defining "attacker" here. What abilities does the attacker have? (e.g., do they interface with the system over HTTP, fs, etc.?)

Can you suggest some wording? Couldn't an attacker could have any capability, where the important thing is they are attempting to do something they are not intended to be able to do? That is, that they are attempting to escalate privilege? Of course, as the capability of executing arbitrary js code is already the highest privelege, escalation from there is not possible.

Ah, yes, that makes sense. I think what threw me off is the whole no-escalating-above-arbitrary-js-code is explained in the example. I think it might help clarify to pull it up to this paragraph.

Given that the definition is fairly abstract, it would also be helpful to have a few examples of attackers, along with escalations that would be considered security vulnerabilities. I imagine that we're often thinking of attackers as HTTP clients that try to escalate above being able to send HTTP messages to the system. It's clear that being able to remotely execute code from an HTTP request is an escalation that would be considered a security vulnerability, but are there examples of more subtle escalations that would count?

Showing examples of particular issues that were in fact considered security issues would also help. Do we have examples we can use? I think nodejs/security-wg#17 will help us get good examples: having a private disclosure process that retroactively turns public once the issue is resolved will make the boundaries more clear.

sam-github · 2017-07-31T22:39:53Z

I'm going to go through the sec release history and try to find some good examples, as in https://www.python.org/news/security/, that flesh out the kinds of things Node does and does not consider a vulnerability.

mhdawson · 2017-08-01T02:07:13Z

README.md

+- Causing program termination using either the public Javascript APIs or the
+  private bindings layer APIs. Doing so requires the ability to execute
+  arbitrary Javascript code, which is already the highest level of privilege
+  possible.


I'd agree more examples, including some positive examples would be useful.

sam-github · 2017-08-01T19:06:29Z

possible examples:

8.0.0

new Buffer(num) and Buffer(num) will zero-fill new Buffer instances
[7eb1b4658e]
#12141.
buffer: Zero-fill excess bytes in new Buffer objects created with
Buffer.concat() while providing a totalLength parameter that exceeds the
total length of the original Buffer objects being concatenated. (Сковорода
Никита Андреевич)
ref: https://github.com/nodejs/node-private/pull/64
ref: 8fb8c46303
url.resolve may transfer the auth portion of the url when resolving between two full hosts, see #1435.

4.8.3

tls:
- fix rare segmentation faults when using TLS
  - (Trevor Norris) #11947
  - (Ben Noordhuis) #11898
  - (jBarz) #11776

nodejs/security-wg#35 (comment)

jasnell · 2017-08-24T04:55:37Z

README.md

@@ -173,6 +173,51 @@ Your email will be acknowledged within 24 hours, and you’ll receive a more
 detailed response to your email within 48 hours indicating the next steps in
 handling your report.

+There is not any hard and fast rule to determine if a bug is worth reporting as


s/There is not any hard and fast rule/There are no hard and fast rules

jasnell · 2017-08-24T04:56:16Z

README.md

@@ -173,6 +173,51 @@ Your email will be acknowledged within 24 hours, and you’ll receive a more
 detailed response to your email within 48 hours indicating the next steps in
 handling your report.

+There is not any hard and fast rule to determine if a bug is worth reporting as
+a security issue. The general rule is any issue worth reporting via the


I'd strike via the security address

jasnell · 2017-08-24T04:56:29Z

README.md

@@ -173,6 +173,51 @@ Your email will be acknowledged within 24 hours, and you’ll receive a more
 detailed response to your email within 48 hours indicating the next steps in
 handling your report.

+There is not any hard and fast rule to determine if a bug is worth reporting as
+a security issue. The general rule is any issue worth reporting via the
+security address must allow an attacker to affect the confidentiality, integrity


s/affect/compromise

jasnell · 2017-08-24T04:57:20Z

README.md

+a security issue. The general rule is any issue worth reporting via the
+security address must allow an attacker to affect the confidentiality, integrity
+or availability of the Node.js application or its system for which the attacker
+does not already have the capability.


The "for which the attacker does not already have the capability" is a bit odd. Perhaps just strike that part?

If you have the right to run arbitrary js code, calling process.exit() isn't a vuln. If you have the mysql credentials, being able to dump the DB isn't a vuln. If you need shell access to execute an exploit that gives you shell access, its not a vuln. I think this is important, a vulnerability allows capability escalation, if you already have the capability, it isn't a vuln.

jasnell · 2017-08-24T04:58:04Z

I think I'd much prefer to see this in a doc/guides document rather than the readme

sam-github · 2017-08-24T13:58:02Z

I think I'd much prefer to see this in a doc/guides document rather than the readme

As I said:

This is the point at which people are told where to report security issues, its the point they should be told what to report as well, IMO.

But its more important that it is doced and easily findable from the place the security reporting email address is descibed, so I'll move it.

Can you give me an explicit location and title?

sam-github · 2017-08-30T16:12:35Z

@jasnell, gentle ping, I know you are juggling eggs ATM

mhdawson

LGTM

PR-URL: nodejs#14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: #20011 Refs: #14485 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: nodejs#20011 Refs: nodejs#14485 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jul 25, 2017

Trott reviewed Jul 25, 2017

View reviewed changes

drifkin reviewed Jul 25, 2017

View reviewed changes

mhdawson reviewed Aug 1, 2017

View reviewed changes

sam-github force-pushed the define-vulnerability branch from 613b0b3 to 0a0d060 Compare August 1, 2017 14:24

sam-github force-pushed the define-vulnerability branch 2 times, most recently from e646d3d to e46d161 Compare August 22, 2017 17:11

jasnell reviewed Aug 24, 2017

View reviewed changes

gibfahn approved these changes Aug 31, 2017

View reviewed changes

mhdawson approved these changes Sep 7, 2017

View reviewed changes

sam-github force-pushed the define-vulnerability branch 2 times, most recently from aa357de to cca378b Compare September 7, 2017 20:43

doc: describe what security issues are

7540821

PR-URL: nodejs#14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

sam-github force-pushed the define-vulnerability branch from cca378b to 7540821 Compare September 7, 2017 20:44

sam-github merged commit 7540821 into nodejs:master Sep 7, 2017

sam-github deleted the define-vulnerability branch September 7, 2017 20:50

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

doc: describe what security issues are

ca3c4a0

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins pushed a commit that referenced this pull request Sep 11, 2017

doc: describe what security issues are

d66a4eb

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

doc: describe what security issues are

ea86cb5

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017

doc: describe what security issues are

c6f3374

PR-URL: nodejs#14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins pushed a commit that referenced this pull request Sep 20, 2017

doc: describe what security issues are

17d0cf5

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins added the land-on-v6.x label Sep 20, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

MylesBorins pushed a commit that referenced this pull request Sep 21, 2017

doc: describe what security issues are

7dcce65

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

MylesBorins pushed a commit that referenced this pull request Sep 26, 2017

doc: describe what security issues are

33fdbb5

PR-URL: #14485 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

sam-github mentioned this pull request Oct 10, 2017

Threat model nodejs/security-wg#51

Closed

tniessen mentioned this pull request Apr 13, 2018

doc: fix typo in README #20011

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: describe what security issues are #14485

doc: describe what security issues are #14485

sam-github commented Jul 25, 2017

Trott Jul 25, 2017

Trott commented Jul 25, 2017

sam-github commented Jul 25, 2017

Trott Jul 25, 2017 •

edited

Loading

sam-github Jul 25, 2017

sam-github commented Jul 25, 2017

Trott Jul 25, 2017

sam-github Jul 25, 2017

Trott Jul 25, 2017

sam-github Jul 25, 2017

drifkin Jul 25, 2017

sam-github Jul 25, 2017

drifkin Jul 25, 2017

sam-github commented Jul 31, 2017

mhdawson Aug 1, 2017

sam-github commented Aug 1, 2017 •

edited

Loading

jasnell Aug 24, 2017

jasnell Aug 24, 2017

jasnell Aug 24, 2017

jasnell Aug 24, 2017

sam-github Aug 24, 2017

jasnell commented Aug 24, 2017

sam-github commented Aug 24, 2017

sam-github commented Aug 30, 2017

mhdawson left a comment

doc: describe what security issues are #14485

doc: describe what security issues are #14485

Conversation

sam-github commented Jul 25, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Trott commented Jul 25, 2017

sam-github commented Jul 25, 2017

Trott Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github commented Jul 31, 2017

Choose a reason for hiding this comment

sam-github commented Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 24, 2017

sam-github commented Aug 24, 2017

sam-github commented Aug 30, 2017

mhdawson left a comment

Choose a reason for hiding this comment

Trott Jul 25, 2017 •

edited

Loading

sam-github commented Aug 1, 2017 •

edited

Loading