[Feature]: Fail on whitelisted advisories that are not found

[dgjbge]

Hi, thanks for this great tool, here's an idea I've been thinking about:

The process for managing CVEs is typically to attempt to patch the affected libraries and if no patch is available, verify the risk of the CVE and if it is low, whitelist the CVE.

At some point the issue is fixed in an dependency, and the CVE no longer needs to be whitelisted and audit-ci helpfully gives a message about this, but since a CI build somewhere does not fail, this message is generally ignored.

This potentially becomes an issue in cases where the CVE reappears in the future, maybe in a different library, or maybe in the same one, but after the use of that library has materially changed. In these cases the CVE should be reassessed, but because the whitelist lingered in the config if is missed.

If there were a flag to give an error code .e.g. --fail-not-found then the above would be mitigated and it would also catch copy/paste mistakes where excessive advisories are whitelisted across multiple repos.

Any thoughts?

Dave

[quinnturner]

Hi there,

Thanks for this great writeup!

I wholeheartedly agree that we should at least a flag to enforce this behaviour. I will work on that soon 😄

I agree that --fail-not-found is an appropriate flag name. --fail-whitelist-not-found is more verbose, but it is more clear.

The other option we have is to introduce a breaking change v3.0.0 (perhaps release along-side #131) and enforce this behaviour by default with a flag to disable the behaviour. The flag could be named --pass-whitelist-not-found. We use the --pass-enoaudit flag currently, so --pass-* is appropriate, and --pass-fail-not-found is confusing.

Introducing this behaviour as default would most likely break some CIs, but this is appropriate for most environments IMO.

Would love some more feedback! CC: @andy-patt

[dgjbge]

Thanks Quinn.

Personally, I'd welcome the breaking change to have fail-by-default behaviour because
i) It means I won't forget to add the flag!
ii) I don't think anyone would intentionally whitelist items that are not relevant

[andy-patt]

I think not failing by default is reasonable as the impact of the original CVE, which was whitelisted, hasn't changed now that a fix has been made available. IMO keeping dependencies always up to date is out of scope for this tool and should be handled with other tooling. That being said, it's not a problem to use the default being discussed here - just providing my opinion.

This potentially becomes an issue in cases where the CVE reappears in the future, maybe in a different library, or maybe in the same one, but after the use of that library has materially changed. In these cases the CVE should be reassessed, but because the whitelist lingered in the config if is missed.

Can this actually happen? I would think a new CVE would have to be created. For example, CVEs always include the year. If this were to re-appear a year later, I would think the year would be updated.

I think the use case of re-assessing after the use of a library has changed is relevant, but not entirely solved by the change being discussed. Perhaps adding an expiry to the whitelist could also help.

[quinnturner]

I missed that we have the flag --show-not-found in the current project. An additional flag --pass-whitelist-not-found will introduce overlap that may lead to confusion regarding the mix of these flags.

I propose to merge them into a single flag that allows users to specify their desired behaviour. The configurable behaviours would be something along the lines of: ignore, warn (print to console in yellow), and error.

Some ideas for the flag name are:
--redundant-whitelist <option>
--excess-whitelist <option>

To exhaust all possibilities for this configuration, there's also the option of having per-package overrides for the ignore, warn, and error. For example, we frequently get the request to ignore devDependencies. I could see users wanting to warn/ignore for all devDependencies, but error on all dependencies. If they used the whitelist config option and listed all devDependencies to make it so that they don't break the build (not recommended, but it happens), any non-vulnerable packages with the error flag would break the build.

To me, it seems like more work as the end-user to handle manually configuring which not-found vulnerable dependencies to ignore/warn/error than to simply remove them from the whitelist. For this reason, I don't think it's appropriate to support the per-package behaviour.

[dgjbge]

Can this actually happen? I would think a new CVE would have to be created. For example, CVEs always include the year. If this were to re-appear a year later, I would think the year would be updated.

So, probably more into than anyone needs to read because I'm happy with the flag suggestions above, but just to clarify my edge case:

The CVE 'reappearance' would be if the 'unpatched' version of a transitive dependency that was previously 'fixed' for one library, appears as a transitive dependency in another library.

Say some-lib has a transitive dependency ropey-lib and ropey-lib v2.1 has a CVE against it. It's in our dev dependencies and not a big risk so we whitelist the CVE.

Time passes...

some-lib replaces it's dependency on ropey-lib with something else in its next major version. The CVE no longer applies to some-lib

Time passes

A develop adds cool-new-lib to the production dependencies, but cool-new-lib uses the old version of some-lib which has ropey-lib at v2.1, only now it's in production there's a potential vulnerability. Unfortunately, we don't spot it because we left the CVE in the whitelist.

And if you followed that, well done! It's niche, I admit.

[quinnturner]

TL;DR: the print options seems to be a reasonable default in all workflows if you use the various whitelist options mindfully.

Let's consider some workflows:

If a user is using a lock file (yarn.lock or package-lock.json), dependencies (including transitive) will not be updated until users (or a bot) manually perform a {npm|yarn} update. Upon an update, existing whitelists will be redundant. From a workflow perspective, it seems reasonable to address the redundant whitelist for cleanliness purposes, but keeping them there doesn't introduce a vulnerability unless they cover an unknown vulnerability (as per @dgjbge's use-case). Either the print option or the error option seem to be a reasonable default in this case.

If a user is not using a lock file, the transitive dependencies will be updated automatically. The whitelist will be redundant, but in an intrusive manner. The build will break because a vulnerability has been fixed automatically. IMO, the print option seems to be the most reasonable default in this case.

There are three types of whitelists that users can perform with this tool: --whitelist [packages], --advisories [advisory numbers], and --path-whitelist [paths].

In the case that @dgjbge provided, this would be a non-issue if the original whitelist was performed using --path-whitelist (e.g. 123|some-lib>ropey-lib). The newly introduced cool-new-lib would not be within the whitelisted paths (it would need to use 123|cool-new-lib>ropey-lib), so the audit will fail correctly. If a different prototype pollution existed in the future for ropey-lib, it would get a new advisory, so it's still "future-proof". If a whitelist is redundant with the usage of a path whitelist, there's no reason to error since it's inherently not an error, just outdated/redundant. Thus, print is desirable here.

However, if the author used either the advisories option or the whitelist option then a new vulnerability could slide through unnoticed. Using the whitelist option is inherently unsafe as you're saying ignore all advisories with this package, so I am not concerned with this workflow being "future-proofed".

Thus, this problem comes down to the usage of the advisories option. In general, there is greater risk of transitive vulnerabilities being introduced if the user decides to use advisories instead of path-whitelists. This risk should be clearly communicated to users. Both print and error are reasonable in this case (print if it's a devDependency probably, error if it's a production dependency).

This package doesn't make distinctions between dependencies and devDependencies for many reasons. All in all, the print options seems to be a reasonable default in all workflows if you use the various whitelist options mindfully.

[dgjbge]

Just a note on the module path whitelist as sometimes it's not practical.

Last month we many hundreds of reported vulns in per project due to a CVE in "acorn" that was included many times in the many dependencies of jest and webpack.

So, while path whitelists might be the 'right way', I don't think anyone is going to add 100's of paths over multiple repos!