collecting info on node vulnerabilities by sam-github · Pull Request #35 · nodejs/security-wg

sam-github · 2017-08-01T18:00:56Z

These will be reformatted into JSON, just PRing early so what I'm
working on is visible.

Depends on #34

sam-github · 2017-08-01T18:02:52Z

So far I have just scraped the changelogs from 8.x back to and including 4.x, no further, now I have to read through in more detail and organize the issues into individual records and then into the JSON schema.

evilpacket · 2017-08-01T18:09:09Z

vuln/SCHEMA.md

+|-----|------|-------------|---------|
+|`id`|Integer > 0|Unique number identifying the record|349|
+|`created_at`|date-time|Record creation|"2017-05-19T22:45:59.16+00:00"|
+XXX do we need this, or is it just a mongo internal field?


This was our identifier. You don't have to keep this. It was a postgres integer incrementing when we added vulns.

evilpacket · 2017-08-01T18:09:45Z

vuln/SCHEMA.md

+XXX do we need this, or is it just a mongo internal field?
+
+|`updated_at`|date-time|Record update|"2017-06-05T13:01:57.071+00:00"|
+XXX do we need this, or is it just a mongo internal field?


These are date fields that are useful. I'd keep them but it's totally up to you.

evilpacket · 2017-08-01T18:10:53Z

vuln/SCHEMA.md

+
+|`title`|String|Short readable description|"Directory Traversal"|
+|`author`|String|Finder of vuln|"Liang Gong"|
+XXX what if reporter was not finder?


+1 to changing this name, we never did because of legacy support but yes I'd change it and possibly add other fields to give appropriate credit such as who authored the advisory, who found the issue, etc.

evilpacket · 2017-08-01T18:11:17Z

vuln/SCHEMA.md

+|`publish_date`|date-time|when made public|"2017-06-05T13:01:57.07+00:00"|
+|`cves`|Array[String]|Associated CVE IDs|[]|
+XXX only 22 and 7 have multiple CVEs, might be simpler to enforce a CVE-per-record rule
+XXX for both, all the CVEs are reserved but still empty, whats up?


it's CVE. We don't control that. /shrug

evilpacket · 2017-08-01T18:11:51Z

vuln/SCHEMA.md

+XXX see http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-3743 for example
+
+|`vulnerable_versions`|semver-spec|vulnerable versions?|"<99.999.9999"|
+XXX What is exact syntax of semver-spec? Is it https://www.npmjs.com/package/semver#ranges ?


yes it uses the semver package and supports the ranges it supported (loose option was set)

evilpacket · 2017-08-01T18:12:06Z

vuln/SCHEMA.md

+XXX What is exact syntax of semver-spec? Is it https://www.npmjs.com/package/semver#ranges ?
+
+|`patched_versions`|semver-spec|versions after fixed?|"<0.0.0"|
+XXX so vulnerable versions are `vulnerable_versions` MINUS `patched_versions`? And any other versions are assumed not vulnerable?


evilpacket · 2017-08-01T18:12:41Z

vuln/SCHEMA.md

+|`overview`|Multiline String|Long form description|"`badjs-sourcemap-server` recieves files sent by `badjs-sourcemap`.\n\n`badjs-sourcemap-server` is vulnerable to a directory traversal issue, giving an attacker access to the filesystem by placing \"../\" in the url.\n\nExample request:\n```\nGET /../../../../../../etc/passwd HTTP/1.1\nhost:localhost\n```\nand response:\n```\nHTTP/1.1 200 OK\nDate: Wed, 17 May 2017 22:59:49 GMT\nConnection: keep-alive\nTransfer-Encoding: chunked\n\n{content of /etc/passwd}\n```"|
+|`recommendation`|Multiline String|Recommendation for mitigation|"Because there is no fix for this module, we recommend using a different one."|
+|`references`|String, multiline markdown bullet list|URLs of interest?|"* [PoC by Liang Gong](https://github.com/JacksonGL/NPM-Vuln-PoC/tree/master/directory-traversal/badjs-sourcemap-server)"|
+XXX what is structure? do they have to be markdown URLs?


it's markdown formatted text.

evilpacket · 2017-08-01T18:13:24Z

vuln/SCHEMA.md

+
+|`cvss_vector`|CVSS vector|CVSS vector|"CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:H/I:N/A:N"|
+|`cvss_score`|CVSS score|Calculated from vector|7.5|
+XXX cvss v2? https://nvd.nist.gov/vuln-metrics/cvss/vector-v2 or https://www.first.org/cvss/v2/guide ?


I believe v2 - it uses this module https://www.npmjs.com/package/cvss

evilpacket · 2017-08-01T18:15:13Z

vuln/SCHEMA.md

+XXX cvss v2? https://nvd.nist.gov/vuln-metrics/cvss/vector-v2 or https://www.first.org/cvss/v2/guide ?
+
+|`coordinating_vendor`| "^Lift Security"
+XXX Company who reported? But different from the author? Is coordination a thing, if so, what is coordinated?


For example. why should other vendors contributor and help with this process. It's not required but giving them credit for who help the disclosure process (hand holding) would be nice. I have a feeling we'll be sending a lot of stuff your way in the future and if it's already all buttoned up and written up it would be nice to give credit. Could be some other field though.

sam-github · 2017-08-03T18:51:58Z

This is a first pass at finding the node security issues released, only 4.x and later. @evilpacket @grnd I'd be interested in opionions on the description and overview text, since I imagine that tools could be extended to detect the current version of node, and report and vulnerabilities associated with it from this information. I'm wondering about details such as active/passive voice, description of problem vs description of fix (node usually describes the fix, not the problem), and whether the description field should/should not be capitalized, be a full sentence. Or whether it doesn't matter.

The data is currently in a text file for easy of updating (much easier than JSON), but I have a script locally that explodes it into JSON.

I'll do one more pass on the data to check for quality and typos, but I'm a bit burnt out after 2 days of going through this, that will have to wait until next week when I'm back from vacation.

sam-github · 2017-08-03T18:54:08Z

vuln/core/README.txt

+
+--
+
+CVE: XXX assigned, @mhdawson, what is it?


sam-github · 2017-08-03T18:55:17Z

vuln/core/README.txt

+
+--
+
+XXX break into 4 records?


is breaking this into a record per CVE busy work, or does it make the data much easier to consume? I suspect the latter, I'll probably make sure that no record has more than a single CVE, that seems to be what rubysec does, but perhaps I misundertand the CVE purposes. Comments?

sam-github · 2017-08-03T18:57:21Z

vuln/core/README.txt

+code. This vulnerability would require an attacker to be able to execute
+arbitrary JavaScript code in a Node.js process.
+
+XXX This is not remote, why was it considered a security issue for node?


I will find the PR for this in a second pass and look for commentary by @nodejs/security

https://github.com/nodejs/security/issues/85 and https://github.com/nodejs/node-private/pull/80

@bnoordhuis looking at the conversations, it looks to me that this V8 bug alone would probably not have triggered a security release, but since a security release was already happening (https://nodejs.org/en/blog/vulnerability/october-2016-security-releases/) and it is a V8 bug, and 6.x was transitioning to LTS, it was easy to just bundle it in.

Yes, that sounds about right.

sam-github · 2017-08-03T18:57:53Z

vuln/core/README.txt

+
+--
+
+XXX split into two records?


this seems like two related vulns, better to be two records with one CVE per record.

sam-github · 2017-08-03T18:58:23Z

vuln/core/README.txt

+
+--
+
+XXX this doesn't appear to be remote, is it a node vulnerability?


need to find why @nodejs/security called this a vuln, seems like it needs code access to me, though perhaps data can be injected?

https://github.com/nodejs/security/issues/20 for context.

OK, looks like its CVSS score indicated it was a vuln, and its possible to imagine data being injected which when stringified and returned would leak memory?

sam-github · 2017-08-03T18:59:43Z

vuln/core/README.txt

+
+--
+
+XXX this was NOT called a security release?


since this breaks a security guarantee, seems like it would deserve a CVE @nodejs/security

this seems similarl to https://github.com/nodejs/security-wg/pull/35/files#r131361440, could be a vuln

sam-github · 2017-08-03T19:00:50Z

Also, once we get the CVE allocation process sorted, #33, I think we should apply for CVEs for all of these that don't have one. cc: @mhdawson

sam-github · 2017-08-11T21:43:29Z

@nodejs/security PTAL, does this reflect known vulnerabilities in node? The README.txt is probably easier to review, though I intend to delete it and the explode.js script and commit only the JSON vulnerabilities into the database.

@nodejs/security-wg is the format of this useful? In particular, will it be useful to tools to allow them to report vulnerabilities not only in module dependencies, but also in the version of the node runtime itself? Can you think of a better naming convention than sequential IDs? Eventually, I hope to be able to apply for CVEs for them all, but that won't happen very quickly.

gibfahn

Hard to say how useful it is, that depends on what tools target it. But I'd say it's fairly useful just as a list to manually parse.

gibfahn · 2017-08-23T00:34:13Z

vuln/core/1.json

+  ],
+  "ref": "https://nodejs.org/en/blog/vulnerability/july-2017-security-releases/",
+  "vulnerable": "8.x, 7.x, 4.x, 6.x, 5.x",
+  "patched": "^8.1.4, ^7.10.1, ^4.8.4, ^6.11.1",


Would an array make more sense than a , separated list here?

I don't think so, this is consistent with the version spec formats in the package.json engine field, and also with usage of https://github.com/npm/node-semver#usage, which is how it will probably be used. at least, its intended to be compatible with node-semver, I'll do a check.

It wasn't correct, a logical OR needs ||, not , . Fixed.

gibfahn · 2017-08-23T00:37:49Z

vuln/core/11.json

+  "vulnerable": "6.x",
+  "patched": "^6.9.0",
+  "ref": "https://nodejs.org/en/blog/vulnerability/october-2016-security-releases/ for",
+  "author": "Jann Horn",


Ref #35 (comment), changing author to something else seems like a good idea.

Maybe reporter?

Reasonably, but that should happen first in #34, and then applied to vuln/core as well as vuln/npm

PR-URL: #35 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

sam-github · 2017-08-23T18:55:46Z

landed in c7041cd

PR-URL: nodejs/security-wg#35 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

evilpacket reviewed Aug 1, 2017

View reviewed changes

sam-github commented Aug 3, 2017

View reviewed changes

vuln/core/README.txt Outdated

--

CVE: XXX assigned, @mhdawson, what is it?

Copy link

Contributor Author

sam-github Aug 3, 2017

Choose a reason for hiding this comment

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

@mhdawson

sam-github commented Aug 3, 2017

View reviewed changes

sam-github mentioned this pull request Aug 11, 2017

doc: describe what security issues are nodejs/node#14485

Merged

2 tasks

sam-github changed the title ~~WIP collecting info on node vulnerabilities~~ collecting info on node vulnerabilities Aug 21, 2017

gibfahn approved these changes Aug 23, 2017

View reviewed changes

vuln/core: add known vulnerabilty JSON for node

3352d12

sam-github added a commit that referenced this pull request Aug 23, 2017

vuln/core: add known vulnerabilty JSON for node

c7041cd

PR-URL: #35 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

sam-github closed this Aug 23, 2017

sam-github deleted the add-node-vulns branch August 23, 2017 18:55

sam-github mentioned this pull request Sep 18, 2017

Who should be in @nodejs/security? Who should have access to the private repo? nodejs/TSC#358

Closed

sam-github mentioned this pull request Oct 11, 2017

node vulnerabilities are not listed anywhere machine readable #27

Closed

patrickm68 added a commit to patrickm68/security-wg-process that referenced this pull request Sep 14, 2023

vuln/core: add known vulnerabilty JSON for node

465639a

PR-URL: nodejs/security-wg#35 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

mattstern31 added a commit to mattstern31/security-wg-process that referenced this pull request Nov 11, 2023

vuln/core: add known vulnerabilty JSON for node

071898d

PR-URL: nodejs/security-wg#35 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>


		--

		XXX this doesn't appear to be remote, is it a node vulnerability?

Uh oh!

Conversation

sam-github commented Aug 1, 2017

Uh oh!

sam-github commented Aug 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Aug 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Aug 11, 2017

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sam-github Aug 3, 2017 •

edited

Loading

sam-github commented Aug 3, 2017 •

edited

Loading