nswg-reporter: initial proposition for hackerone reporter #234

lirantal · 2018-04-22T20:17:06Z

Relates to #146
Status: WIP

Usage

$ npm run reporter -- --reportId <h1-report-id>

See more information in the new README.md file for the tools/ directory.

TODO

Fix linting
Update report format to recent changes and Suggestion for new vulnerability report format #200
How do we get the vulnerable/patched versions information? we had an idea once to update it ourselves in the summary text and then parse it. For now we're interactively requiring this information in the CLI.
Tests
API keys mgmt (we should be issuing API keys for triage team)

vdeturckheim · 2018-04-23T08:35:34Z

package.json

  },
  "keywords": [],
  "author": "",
  "license": "MIT",
  "dependencies": {
+    "axios": "^0.18.0",
+    "hackerone": "^1.1.0",


Is this depedency used in the code?

good catch, I tried it first but found it to be overkill for what we need
I'll remove it.

vdeturckheim · 2018-04-23T11:48:51Z

package.json

@@ -2,13 +2,16 @@
  "name": "security-wg",
  "version": "1.0.0",
  "scripts": {
-    "test": "node tools/vuln_valid/index.js"
+    "test": "node tools/vuln_valid",
+    "report": "node tools/reporter"


can you add a README with usage instructions?

@vdeturckheim added initial readme

dgonzalez · 2018-04-23T14:16:41Z

tools/reporter/config.js

@@ -0,0 +1,6 @@
+'use strict'
+
+module.exports = {


I use convict to manage configurations. It has a nice feature that allows you to override values per environment but also override them via env vars and I assume that this is going to end up running in a docker container. Just an idea.

this config uses env vars too, not sure if convict will be an overkill or not

yeah, if it uses env vars should be fine.

ChALkeR · 2018-04-30T06:39:44Z

tools/reporter/index.js

+const NswgReporter = require('./NswgReporter')
+
+const USER = process.env.NSWG_BB_API_USERNAME
+const PASS = process.env.NSWG_BB_API_PASSWORD


Do not store credentials in environment variables.

@ChALkeR:

would we want to source a config file from the system this is running on?
a-la ~/.aws/credentials ?

if we will run this on CI, which I know you're not too big fan of atm due to the security implications for that, how would we provide the relevant creds?

The problem with environment variables is that they are not treated as secure, and on-disk file storage with right permissions is better.

Some personal evidence:

npm passwords and api tokens ended up being published in the registry packages because they were leaking to the environment variables and another tool did not treat env vars as secure and saved them in config.gypi, which ended up being published. For publish access to express this happened more than once.

Including the tokens on a CI, in any form, also has risks — for example, I was able to trick Travis CI (and some others) into exposing encrypted credentials.

Re: reading from fs and CI — what would be the usecases for this tool? I assumed manual execution of this tool with explicit confirmation of the action taken for each module. Because even the «automatically open a generic security issue invite on the related module's repo» action without human confirmation sounds highly dubious. I do not think that not-previously-public things should be published automatically from CI.

So, perhaps just ask the credentials each time from the CLI, optionally password manager integration?

About the CI part — could we securely limit it's access to only the public reports somehow? Automatically opening pull requests to this repo based on published reports should be fine.

Thanks for the context and elaborate information, insightful as always! I actually did have your notes gist starred already from a previous read :-)

Having an interactive prompt on the CLI sounds good.
I'll make appropriate changes.

About CI and automation around tooling - we're not there yet. I haven't finalized requirements and actually think we'd benefit more from having a webhook originating from H1 platform to us (some service somewhere) but since H1 aren't supporting a custom webhook then this isn't relevant atm. Anyway I think we'll take this gradually and securely to see how the process works and eases human intervention so we can more easily handle the load.

lirantal · 2018-05-22T23:33:07Z

As discussed in the WG meeting last week I'll resume work on this soon. Stay tuned.

lirantal · 2018-06-15T21:27:42Z

@dgonzalez @vdeturckheim @ChALkeR appreciate another review now that comments have been addressed. Some of the TODOs will still be unaddressed for example grabbing the versions info from H1.

lirantal · 2018-07-06T10:14:31Z

@nodejs/ecosystem-security we've been testing it for a while both me and @vdeturckheim and if we merge it then others can also start using the tool to speed up the whole report triage process. I can address remaining items in follow-up PR later and other formatting changes that will be made.

Specific triage team members who use the tool need to ping us for an API key.

dgonzalez · 2018-07-06T10:29:07Z

Big +1 on this. Thanks a lot for the work. This makes the life much easier.

lirantal · 2018-07-14T20:17:24Z

Another ping before I merge if anyone wants to chime in on this.

old comment, updated code for interactiveness as discussed

lirantal added the enhancement label Apr 22, 2018

lirantal self-assigned this Apr 22, 2018

lirantal requested review from ChALkeR and vdeturckheim April 22, 2018 20:17

vdeturckheim requested changes Apr 23, 2018

View reviewed changes

dgonzalez approved these changes Apr 23, 2018

View reviewed changes

ChALkeR previously requested changes Apr 30, 2018

View reviewed changes

lirantal force-pushed the feat/nswg-reporter branch from bf2b7d9 to 9277eb9 Compare June 1, 2018 16:42

lirantal force-pushed the feat/nswg-reporter branch from 9277eb9 to 9c166bb Compare June 13, 2018 19:58

feat(nswg-reporter): initial proposition for hackerone reporter

2d643be

lirantal force-pushed the feat/nswg-reporter branch from 9112d16 to 2d643be Compare June 15, 2018 22:00

fix: coordinating vendor cant be empty string

e08b06e

vdeturckheim approved these changes Jun 18, 2018

View reviewed changes

This was referenced Jun 20, 2018

refactor(npm-db): update author field to be more verbose #314

Merged

Suggestion for new vulnerability report format #200

Closed

fix references field to comply with new format

45794e1

sam-github mentioned this pull request Jun 30, 2018

Implement process for management of thirdparty vulnerabilities #54

Closed

7 tasks

Merge branch 'master' into feat/nswg-reporter

4925642

lirantal merged commit 4212109 into nodejs:master Aug 1, 2018

sam-github mentioned this pull request Apr 16, 2019

Automating triage work around HackerOne #146

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nswg-reporter: initial proposition for hackerone reporter #234

nswg-reporter: initial proposition for hackerone reporter #234

lirantal commented Apr 22, 2018 •

edited

vdeturckheim Apr 23, 2018

lirantal May 3, 2018

vdeturckheim Apr 23, 2018

lirantal May 3, 2018

lirantal Jun 13, 2018

dgonzalez Apr 23, 2018

lirantal May 3, 2018

dgonzalez Jul 6, 2018

ChALkeR Apr 30, 2018

lirantal May 3, 2018

ChALkeR May 5, 2018 •

edited

lirantal May 6, 2018

lirantal commented May 22, 2018

lirantal commented Jun 15, 2018

lirantal commented Jul 6, 2018

dgonzalez commented Jul 6, 2018

lirantal commented Jul 14, 2018

nswg-reporter: initial proposition for hackerone reporter #234

nswg-reporter: initial proposition for hackerone reporter #234

Conversation

lirantal commented Apr 22, 2018 • edited

Usage

TODO

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChALkeR May 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lirantal commented May 22, 2018

lirantal commented Jun 15, 2018

lirantal commented Jul 6, 2018

dgonzalez commented Jul 6, 2018

lirantal commented Jul 14, 2018

lirantal commented Apr 22, 2018 •

edited

ChALkeR May 5, 2018 •

edited