Identifiers for vulnerabilities are not always unique

[DBaakman]

Type: Bug/Question

Description: In order to whitelist certain vulnerabilities, our security team wants to use unique identifiers, opposed to just use name/versions. Unfortunately not all vulnerabilities use a CVE identifier (or some other numeric identifier), but resort to a string like 'XSS' or 'SQL injection' (and in some cases, there is no identifier at all), so at first glance it is not really clear whether we can actually use those identifiers.

I did verify manually whether all duplicate identifiers (which is possible, when the same vulnerability is applicable in more than one version / branch) are pointing to the same vulnerability. (by updating the validate script to export all non-unique identifiers per name and then manually go through that list)

I found a few cases where this was not the case: mapbox.js sanitize-html sequelize i18next serve localhost-now

I think it makes sense to update those identifiers? (if wanted, I can create a pull-request for that)

Another question: is it indeed the case that each vulnerability should have a unique (within the name) identifier? This is not written down explicitly, and as validate doesn't check for this (and it is not easy to add this either) I'm not sure whether we can even safely whitelist on name/version/identifier.

[DBaakman]

(edited because just after I created the issue, I found out that my current verification-change wasn't looking for non-unique identifiers, but for non-unique info attributes (as part of my investigation was whether it would make sense to add the possibility to whitelist on info attributes as well); when looking at non-unique info attributes, there is one case: mapbox.js)

For reference, my local change to validate is:

for (var i in npmRepo) {
	var identifiersForModule = [];
	for (var j in npmRepo[i].vulnerabilities) {
		for (var k in npmRepo[i].vulnerabilities[j].identifiers) {
			for (var l in identifiersForModule) {
				if (identifiersForModule[l] === npmRepo[i].vulnerabilities[j].identifiers[k]) {
					console.log('Identifiers are not unique for ' + i)
				}
			}
			identifiersForModule.push(npmRepo[i].vulnerabilities[j].identifiers[k])
		}
	}
}

[eoftedal]

We could add unique identifiers to all of them. Currently "summary" feels a bit misplaced, but I am hesitant to move it, because previous versions are now using the same repo.

CVE should work for all vulns that have CVE, but we could also add a NSP-nn identifiers for all vulnerabilities coming in from the node security project.

[eoftedal]

There is another problem here also. For jQuery, to of the vulns in the list is actually the same vuln, but with different version ranges. This is also a weakness with the format. See the two items in the list with info: https://nodesecurity.io/advisories/328

[DBaakman]

Adding CVE and NSP identifiers (and also HO, for HackerOne?) seems like a good idea. And I totally feel your hesitance to move 'summary' - you have no idea what will break...

And yeah, in hindsight, the format could have been 'better' where the version was an array attribute of the vulnerability, then we could automatically check for duplicate identifiers; but in hindsight everything is easier :) For the whitelisting this is not really an issue.