Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow ignoring/exclusions of certain CPAN security advisories #5

Closed
robinsmidsrod opened this issue Jun 20, 2022 · 13 comments
Closed
Assignees
Labels
Type: enhancement improve a feature that already exists v2 Features for version 2

Comments

@robinsmidsrod
Copy link

I'm having a problem that the File::Temp vulnerability (CPANSA-File-Temp-2011-4116) doesn't have a fix, so it will always generate a failure when scanning for vulnerabilities.

Is it possible to add an option to exclude certain named vulnerabilities, or exclude vulnerabilities that have no fixed version?

@briandfoy
Copy link
Owner

It's possible. What would be a convenient way for you to do that?

@briandfoy
Copy link
Owner

Oh, there is a --no-corelist option too.

@briandfoy briandfoy added Type: enhancement improve a feature that already exists Status: needs help needs outside expertise or capacity labels Jun 21, 2022
@robinsmidsrod
Copy link
Author

robinsmidsrod commented Jun 21, 2022

Maybe --no-unfixed or --ignore-unfixed.

For ignoring specific advisories, My first thought is --ignore CPANSA-XXXX-XXXX, called multiple times if you want to ignore multiple advisories. Or possibly the option to specify a filename with one line per advisory identifier that should be ignored.

If an ignored advisory is detected, it should still be shown, but not in red/highlighted color. In addition, it should not increase the counter for the exit code. If only ignored advisories are shown, the command should exit with success.

@briandfoy
Copy link
Owner

Since this is an audit tool, ignoring unfixed issues isn't what you want to do. However, ignoring particular things may be interesting since you have to affirmatively select the things that you want to ignore. --ignore or --ignore-file might be a way to go. Let me think about that.

However, it might be easier for you to capture the output and filter it however you like. I've been thinking about adding JSON output to make it easier for another program to deal with the output. That wrapper program could then do anything that matters to you in any way that you want to do it.

Another part of the problem is that I've been thinking that the issue identifiers shouldn't necessarily be stable. In the new issues, I've been using the CVE values, like Foo-2022-1234. Earlier reports did not do that, which adds another layer of identifiers when you want to remember what connects to what. If I changed some of those that you had ignored, they'd show up again.

Also, now that I'm adding lots more reports, the exit code feature becomes suspect. Eventually it will try to exit with too large a value, so I need to think about that too.

@briandfoy briandfoy added the v2 Features for version 2 label Jun 21, 2022
@briandfoy
Copy link
Owner

briandfoy commented Jun 21, 2022

I'm tagging this as "v2". We'll start collecting a set of features that people want, sketch out how the next major version might work, and then plan how all of that will happen. I've opened an ideas discussion for this.

@robinsmidsrod
Copy link
Author

@briandfoy I think it would be wise that the security advisories would have stable identifiers, as people will refer to them by identifier in other locations when discussing the issues (e.g. release notes). And if we'd split the tool and the database at some point, it would be nice to be able to refer to a single advisory by stable URL.

My initial thought is that we could use MetaCPAN in some way to host the advisories, and make it possible to look them up per dist and/or package on MetaCPAN. If the advisory already have a CVE number then we might not need to create our own CPANSA numbering scheme, but I'm not sure if all of the advisories actually have CVE numbers.

In terms of exit code, I'd probably want to differentiate between fixable and unfixable advisories, and possibly those bundled with perl. I've noticed that in my own security scanner, I've had to use --no-corelist to avoid security issues in the scanner itself, and not in the code I'm scanning.

@briandfoy
Copy link
Owner

Yes, I want stable identifiers, but not at the expense of another layer that disconnects the report from its source. I'd like to convert identifiers to use the CVE number if it's a CVE, and otherwise just increment year and something else (or maybe YYYYMMDD for release, and increment). I haven't got that far yet, and it's not a priority. It merely helps keep track of things since I can easily connect a CVE to a report. It's only recently that we've been inserting lots of non-CVE reports.

For the --no-corelist issue, if we want to avoid issues that are only there because you are using cpan-audit, then a better approach would be to ignore cpan-audit—some way to run it from a different perl and module path to have it look at an external set of modules unconnected to what you are using to run the audit. Even then, a problem like File::Temp is still going to show up, so it's still a problem because it's installed and available. A proper security solution may be to delete the offending modules so you can't use them (and then not use stuff that uses it).

How do you run it now? Are you doing cpan-audit installed?

@robinsmidsrod
Copy link
Author

With regards to the identifiers I don't really have any strong opinions, except that I want them stable. If renaming the non-CVE ones is the way to go, I don't have a lot of problems with that.

Personally I wouldn't want a security solution to delete stuff I depend on. I'd want it to report the state of the situation and let me make a decision on what to do.

Currently I'm creating a docker container based on alpine:edge (which has perl 5.36.0), then I install the latest version of CPAN::Audit, then I mount in the directory of the repo I'm scanning, and run cpan-audit --no-corelist deps <repo-dir>. So technically I'm not using anything from the repo for the cpan-audit runtime, so issues in the cpan-audit container I wouldn't want reported.

And just for reference, I'm setting up automation to run this on more than 300 repos, which is why I want to reduce irrelevant reports.

@bleargh45
Copy link
Contributor

While I would agree that a proper solution may be to delete the offending modules, in the case of File::Temp, that list ends up including all manner of important downstream packages, including Perlbrew, CPAN, DZil, DBIx::Class, LWP, and more. Yes, I agree that the underlying issue needs to be addressed, but also feel that "just don't use it" isn't really a viable option in all cases. However, one could also argue that "has lots of important downstream packages" should be seen as sufficient to warrant someone addressing the underlying issue, and that without that being addressed all of the downstream packages are suspect regardless.

For the case of CPANSA-App-cpanminus-2020-01... while cpanm may still allow for Signature Verification Bypass, it is also noted in the referenced links that using a trusted mirror would limit exposure. For my use case, I have an internally hosted and trusted partial mirror of CPAN, containing just the Distributions that we have vetted for internal use. Knowing that we are only ever going to be connecting to said internal mirror, having the ability to skip/ignore a particular advisory would be helpful.

@briandfoy
Copy link
Owner

@bleargh45 Thanks. I'm coming around to this feature. I don't have time right away to add it right away. If someone else wants to take a stab at it, go for it.

A quick fix might be like the rsync options --exclude and --exclude-file that simply take the report file names and don't report anything from those files.

bleargh45 added a commit to bleargh45/cpan-audit that referenced this issue Jul 8, 2022
Allows for Advisories/CVEs to be excluded and ignored from the results.  May be
provided multiple times on the command line when you wish to exclude more than
one Advisory/CVE.
@bleargh45
Copy link
Contributor

The above is a first stab at an --exclude option, allowing for individual CVEs/Advisories to be excluded/ignored.

Am not expecting it to be immediately mergeable, but am looking for feedback/comments on the approach and style (was kinda hard to figure out exactly which indentation style was preferred, as there are several different styles and indentations taken across the codebase).

briandfoy pushed a commit that referenced this issue Jul 13, 2022
Allows for Advisories/CVEs to be excluded and ignored from the results.  May be
provided multiple times on the command line when you wish to exclude more than
one Advisory/CVE.
@briandfoy
Copy link
Owner

@bleargh45 understood. I played with it for a bit and changed some stuff around

I've released 20220713.001_001 so people can play with it. We can still change things around.

It's basically what you put together. I changed it to CPAN::Audit::Filter since I'm anticipating other sorts of things it might do (say, like --no-core or something similar).

As for the style, yeah, that's a mess right now. perltidy can always clean that up later, so I prefer code in any style over no code at all. In general, go with the style inside a particular file if it's clear what it's doing, and otherwise do your best.

@briandfoy briandfoy self-assigned this Jul 15, 2022
@briandfoy briandfoy removed the Status: needs help needs outside expertise or capacity label Jul 29, 2022
@briandfoy
Copy link
Owner

This is in 20220729.001. Thanks,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: enhancement improve a feature that already exists v2 Features for version 2
Projects
None yet
Development

No branches or pull requests

3 participants