What upper vulnerable version should be set in reports?

[lirantal]

In reports that we triage, we have had it as a standard so far to specify the latest version that exists at the time of the report as the upper bound.

So if library coolmodule had a report for affecting versions <= 1.0.0 and its current latest release is 1.0.0, we'd set in the report:

vulnerable_versions: '<= 1.0.0'

Pros

forgiving approach which sets an upper limit to what the report has triaged

Cons

newer versions coming out won't be considered vulnerable even though this might actually be the case

See example use-case https://github.com/nodejs/security-wg/pull/513/files where harp is triaged for vulnerable_versions <=0.29.0 but soon after the report was triaged, versions 0.30.0 released, and is probably still vulnerable.

What should we do?

1. We can specify a vulnerable_versions: '*' to account for everything when no fixes that we know of are applied.
2. Keep the existing forgiving upper limit
3. ?

[sam-github]

I find option 1 attractive. It puts the onus on us to do the initial eval, but once the report is accepted, we shouldn't have to do a polling loop on every package with an open vuln, checking if a more recent one exists, and updating the vuln DB if it is still vulnerable. Setting an open-ended max puts the onus back on the package maintainers to notify us when a fix is made, preferably with a PR that updates the DB.

@lirantal do you have a preference among your options?

[dougwilson]

I think option 1 would be a bad idea, as evidenced by the recent finalhandler incident. For example, in that incident the hackerOne report (https://hackerone.com/reports/504931) was published prior to a fix being released and without the knowledge of the maintainer.

Since reports are made public in this way, I'm not sure it would be fair to set the versions to * by default, since, after discovering the report is suddenly public, the maintainer would no be able to correct it by simply releasing a new version. Instead the maintainer will need to make a pull request (or something?) to the security WG to get the range updated, then likely manually contact every vendor who picked up the report to get the version numbers corrected there as well (ex npm security, Snyk, SourceClear, etc.).

This seems like a large burden on maintaining an npm module that seems unnecessary.

npm security currently sets * for any module where there is no published fix at the time of the HackerOne report publication. The issues described already occur with npm and many module maintainers have no idea it's happening and how to correct it. For example take the case of express-basic-auth (https://www.npmjs.com/advisories/827). Since npm marked the range as * since no fix was available at the time of publication, the versions 1.1.7 and 1.2.0 which have the fix are still marked as vulnerable. Both these versions are identical; the reason for the minor bump was trying to figure out how to get npm audit to stop saying it was a vulnerable version. npm security has still not corrected the report.

If there is a public SLA for how long a PR will sit before being merged when a module maintainer releases a patch, that would help mitigate some of the issues here, since there will be a known amount of time for the entry to be corrected after a fix. But I still think the issues around the maintainer discovering that they should open said PR and tracking down all the vendors who already copied the version range, still seems daunting.

[yonjah]

@lirantal agreeing with @sam-github option 1 sounds a lot better and moves the responsibility to the maintainer.
My only concern is if npm audit and other common tools are actually updating the advisories accordingly.
As @dougwilson mentioned npm is reporting express-basic-auth 1.2.0 to be vulnerable even though the relevant snyk report (https://snyk.io/vuln/SNYK-JS-EXPRESSBASICAUTH-174345) was updated to versions below 1.1.7

[sam-github]

I'm pretty sympathetic with @dougwilson after hearing about the internet bile that was undeservedly pouring his way over the weekend.

However, the general pattern of vulns (which I'm absolutly not saying express falls into) is that they don't get fixed unless they are reported in such a way that it causes users to complain. Its a frustrating aspect of open source that the complaints are sometimes so shockingly uncourteous.

I think there are a few issues coming up here:

1. its not clear where vendors are pulling vuln data from, npm is apparently pulling directly from H1 as well as the sec-wg's vuln DB.
   1.a. Or if we update information on H1 or the vuln-db, how long (if at all) it would take for consumers to notice the updates.
2. https://hackerone.com/reports/504931 was a mistake. I've some sympathy for @ronperris on that one, becuase it was not at all obvious what the fallout would be from closing it the way he did. It wasn't intended to be published as a vulnerability.
3. The intention is that a vulns publication would be coordinated, so a project like express would have patched releases out on all vulnerable release lines (or, at least the ones they intend to release on) before the vuln is published, so the publication would be able to state the fixed version ranges. Hopefully, that would be picked up then by npm, etc, and there would never be false positives.
4. https://hackerone.com/reports/504931 is probably a bad example (because it wasn't intended to be a vuln report), but I can't tell where in the H1 template that the vulnerable and fixed versions would go, or if they can be updated after the fact (or if consumers would notice if we updated).
5. Its not clear how much control we have over how vuln reports are consumed, but I'd think we should at the very least be able to coordinate more effectively with npm.
6. Even with good sec-wg&npm coordination, that won't help everything. https://www.npmjs.com/advisories/827 didn't come through the sec wg, it looks like it came from snyk. They currently say only < 1.1.7 are vulnerable, so they do recognize the fix was pusblished -- but I've no idea how long it took for that update to reach them, or how much longer it took to get back to npm audit.
7. While sympathetic about maintainers not knowing how to convince audit tools that a problem has been fixed (or in the case of the ongoing node-gyp problem, ever existed in the first place), I'm not convinced the solution is for the sec-wg to, for every vuln for doesn't have specific "fixed in" versions, somehow get notified of every upstream release and reevaluate whether they fixed the issue, and if they didn't, update the vuln-db (and H1?) with the new info.

It seems to me that the better solution is for npm to make very clear what the source of vuln data is, so it can be updated at source, and to ensure that there is a clearly documented process for packages who have fixed vulns to get that info into npm audit.

[lirantal]

Addressing all points since quite a few have been raised on this thread.

Which option to go for and why?

I am very much in favor of option 1 of setting a * on packages that are triaged and has no fix which is why I re-opened this issue. The reasoning is mostly because if we set an upper bound of the current version the report has been triaged then we are effectively a "point-in-time" security program and maintaining a vulnerability database makes no sense at all. Moreover, if we do keep the upper bound to said version any maintainer can "abuse" the security program by just releasing a newer version that has no fix and workaround the CVE for their tool.

Vendors and the Security WG

As for vendors keeping their vuln reports on their side synced - that will be something that is outside of control and immediate influence circle. While we can do our best to reach out in some cases that won't scale with the amount of reports we get, and the fact that the WG is volunteer-based and our resources and time are limited.

Maintainer contact

@dougwilson

For example, in that incident the hackerOne report (https://hackerone.com/reports/504931) was published prior to a fix being released and without the knowledge of the maintainer

Indeed this happened but that's not per our processes and IIRC this is literally the only case that I can remember in the WG history. In fact, I can point out tens if not module, of modules where we found a vulnerability and waited out on the report for weeks or months beyond the 45 disclosure days time just to get a hold of the maintainer. You might also be surprised how many times it is hard for us to even get in contact with a maintainer.

My point being - the Security WG works in a push-method. We actively get reports of possible vulnerabilities and part of the process is to triage them (and obviously contact the maintainer and work on a fix, see https://github.com/nodejs/security-wg/blob/master/processes/third_party_vuln_process.md for more details). We certainly do not perform security research in order to flush vulnerabilities. However some of the members on the WG are obviously affiliated with security research and may disclose vulnerabilities they found but those should go under the same process and I'm deeply sorry we missed on that in the finalhandler module case.

If there is a public SLA for how long a PR will sit before being merged when a module maintainer releases a patch, that would help mitigate some of the issues here

Definitely! Our processes are working with the maintainers on co-ordinating the fix and even just to confirm and discuss the vulnerability.

Moreover, and with regards to the * default for vulnerabilities where we weren't able to contact the maintainer or no fix is available - if at a future time you as the maintainer will release a fix you really aren't expected to create a PR yourself. We'll be happy to do it for you, just open an issue in the repo and notify us of this and we'll be able to update the report. This however only goes as far as the Security WG database and the HackerOne report and doesn't extend to other vendors and it's indeed up to them to provide good quality of reports and stay up to date with us as the source of truth.

@dougwilson my ❤️ is genuinely with you and apologies I wasn't available to pick the incident sooner, while it was happening to help remedy. Seems like with Vladimir and Ron contacting vendors we are already remedying the effects of this. Please let me know if there are any more ways we can help and take the burden off of you.

[lirantal]

Got an agreement during the agenda call so I'll update the triage process and we can move forward with this.

[sam-github]

Discussed at #541 , removing from agenda (but please add back if it should be discussed again).