implement nodejs fetcher

[liangchenye]

Implement nodejs fetcher by parsing api.nodesecurity.io/advisories.
Cache the last 'updated_at' time of advisories to send a valid 'update' notification.
(#40)

One problem is: there might be more than one fixed/affected package versions in a nodejs advisory.
For example: https://nodesecurity.io/advisories/77

Vulnerable: < 3.1.3 || >= 4.0.0 <4.1.1
Patched: >=3.1.3 < 4.0.0 || >=4.1.1

The current Feature seems not be able to handle this.

Signed-off-by: liang chenye liangchenye@huawei.com

[liangchenye]

The implementation does the following things:

1. using data from api.nodesecurity.io/advisories
2. drop advisory which doesn't have CVE fields
3. convert cvss score to priority (https://www.first.org/cvss/specification-document)
4. add node_version just as my previous comment
   Now I pick up the max fixed version. If we want to solve it throughly, we need to change the comparing logic (https://github.com/coreos/clair/blob/master/database/pgsql/feature.go#L218).
5. add fetcher_node_test.json which is a part of api.nodesecurity.io/advisories
   It has 5 advisories. One of them has no CVE, two of them share a same CVE, one of them has complicate 'version' and the remain one is 'normal'. I think they are representative.

@jzelinskie @Quentin-M PTAL

[jzelinskie]

defer r.Body.Close()

[liangchenye]

Thanks @jzelinskie for the review, commit 'e0a450d' fixes all the issues mentioned in your comment.

The commit 'e4b50f1' fixes a similar 'io reader close' issue in other fetchers.

[Quentin-M]

Could/Should we give a link back to nodesecurity.io?

[Quentin-M]

Especially if we could see the specific version ranges then!

[liangchenye]

I use 'cve.mitres.org' because one CVE may links to multiple advisories.

[liangchenye]

ping @jzelinskie

[jzelinskie]

Does this still need to be fixed?

[liangchenye]

I'll remove the 'FIXME'. I added 'FIXME' just to remind it needs a better way to check if a package is vulnerable.

In current fetchers, if a package's version is recorded as '1.0.1', it means the security issue will be fixed when the package version '>=' 1.0.1. But in nodejs, it is more complicated. Sometimes, it should '>=' 1.0.1, sometimes it should '>' 1.0.1.

[liangchenye]

updated

[jzelinskie]

I'm not sure how I feel about this. If we invent this version and people query the API, won't the FixedIn package always be $version-1, which isn't a real package, but one we made up? That's pretty confusing for end users.

[liangchenye]

I think a thorough solution is changing the current 'version' from types.NewVersion to a full description string like ">=2.5.0 <= 3.0.0 || >=3.1.0". It could solve both this issue and false-positive issue.

It may not necessary to System package managers, but useful for 'Application updaters' and 'Library package managers'.

[liangchenye]

Add a type 'fixedInVersions' to FeatureVersion. WIP

[liangchenye]

The commit 'cb42892: add FixedInVersions to check affected packages' made the following changes:

• add a FixedInVersions to FeatureVersion struct

  'Version' was used both as the detected package version and the patched version of fetched vulnerability. After this change, 'Version' will be used only as the detected package version.
  'FixedInVersions' is a more complicate but more user friendly patched version ( > 1.0 || <0.8 for example)

• add a FixedInVersions struct to utils/types

  In order to be compatible with previous Clair, the default operation is '>=' if no operation was assigned.

• reuse the 'version' key of the 'vulnerability_fixedIn_feature' table

  It is not changed to 'fixed_in_versions' because I don't want to add too many sql migration files. If it should be modified, I'll do it in support multiple namespaces in one layer #193.

@jzelinskie

[guypod]

If I may chime in here, WDYT about supporting Snyk with this integration?

Can use the open source VulnDB (https://github.com/snyk/vulndb) as a source.
Here's the auto-generated JSON feed that combines all the vulns: https://github.com/Snyk/vulndb/blob/snapshots/master/snapshot.json

Using Snyk will open up the possibility of supporting patches for the (frequent) case of vulnerabilities you can't upgrade away. More on that here: https://snyk.io/docs/security/#snyk-s-process-for-creating-patches

It's online, but can also consider using Snyk's API when finding issues, to get more detailed remediation advice.
For instance this API call: https://snyk.io/api/v1/vuln/github/snyk/goof
Will provide the details from this page: https://snyk.io/test/github/snyk/goof

Snyk's DB is AGPL licensed, which makes it fine for internal use. Either way, it's no more restrictive than Node Security's terms of use: https://nodesecurity.io/tos

[guypod]

One more note - you pointed out you drop all the issues with no CVE.
In Snyk's DB (nsp is roughly the same), only about a third of the vulns have a CVE.
I'd strongly encourage not to drop those without a CVE, but rather use our IDs for them.

[jzelinskie]

@liangchenye what do you think about @guypod's proposal? We've discussed the topic a little bit with NodeSecurity and they aren't 100% clear on how they want their data to be licensed in scenarios like this.

[Quentin-M]

Here what's nsp's attorney would like us to add to Clair's license if we were to use their database. tl;dr Use of Clair in a paid product would require a separate, paid licensing with nsp.

SEPARATE LICENSING NEEDED FOR COMMERCIAL USE: Clair is licensed under the [insert license], but leverages security advisories drawn from the Node Security Platform ("nsp") API. These advisories are licensed under a separate terms of service available here: https://nodesecurity.io/tos. By accessing the advisories through Clair you agree to be bound by these terms of service. nsp may only be used for your own personal or internal business purposes. You may not reproduce, duplicate, copy, sell, trade, lease, rent or provide to any other third-party (i) any data retrieved through the nsp, (ii) any report or analysis developed using nsp, (iii) use of nsp, or (iv) access to nsp. Separate licensing must be arranged for these commercial uses by contacting contact@nodesecurity.io.

[liangchenye]

I never think of the license issue :( From my point of view, SnykDB is a better choose to be the 'default nodejs fetcher' in this case.

@jzelinskie I agree with 'not to drop non-CVE' vulns, it helps users to better understand their security status. We can add this to README.md to mention about it: Clair is not only a CVE scanner and ID might be a problem in the future if we keep adding different security types, maybe we can use 'snyk#123456'.

[liangchenye]

@Quentin-M @jzelinskie is SnykDB OK for you, I'll change to that implementation.

Another question is do we plan to provide a document ('implementation.md' for example) to list third-party fetchers?

[johanneswuerbach]

Looks like the nsp licensing issue will soon be resolved upstream https://nodejs.org/en/blog/announcements/nodejs-security-project

[jzelinskie]

This PR is very out of date. I'm going to close it.
The groundwork provided by #432 should enable both layers supporting multiple namespaces and version ranges. If work was to resume on nodejs vulnerability detection, it should be done through these new APIs.