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

[Feature]: Allow excluding vulnerabilities from output of dotnet list package --vulnerable #11926

Open
loop-evgeny opened this issue Jun 29, 2022 · 13 comments
Assignees
Labels
Functionality:ListPackage dotnet.exe list package help wanted Considered good issues for community contributions. Priority:2 Issues for the current backlog. Product:dotnet.exe Triage:NeedsDesignSpec Type:Feature

Comments

@loop-evgeny
Copy link

NuGet Product(s) Involved

dotnet.exe

The Elevator Pitch

Allow excluding vulnerabilities from the output of dotnet list package --vulnerable [--include-transitive] using another command line flag or config file.

Additional Context and Details

We scan for NuGet packages with vulnerabilities regularly on our build server by running dotnet list package --vulnerable --include-transitive. Sometimes it's not practical to upgrade a package version to properly fix the vulnerability, but we don't want to be notified of it anymore (because we have either determined that it doesn't apply to us, made a code change to mitigate it or accepted the risk). It would be great to be able to exclude specific vulnerabilities, e.g. by the advisory URL or perhaps just the ID part of it (CVE or GHSA ID), e.g.

dotnet list package --vulnerable --include-transitive --exclude-vulnerabilities GHSA-qpvx-gpqm-g98j,GHSA-mv2r-q4g5-j8q5
@loop-evgeny loop-evgeny changed the title [Feature]: Allow excluding vulnerabilities from output of `dotnet list package --vulnerable [Feature]: Allow excluding vulnerabilities from output of dotnet list package --vulnerable Jun 29, 2022
@nkolev92 nkolev92 added Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. Pipeline:Icebox and removed Triage:NeedsTriageDiscussion labels Jul 7, 2022
@loop-evgeny
Copy link
Author

This would really make the vulnerability scanning feature so much more useful! Right now we have to ignore a lot of false positives and this inevitably promotes complacency: "ah, it's just that vulnerability check failing as usual - ignore that".

@loop-evgeny
Copy link
Author

Can I contribute some code to help make this happen?

@erdembayar
Copy link
Contributor

Can I contribute some code to help make this happen?

Yes, you can make contributions. First you need to create a design spec where we can discuss what problem it's solving and how to solve it. Fyi I'm working on related issue, probably you can parse the json output and ignore some CVEs using bash/powershell scripts.

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Aug 18, 2023

When using dotnet list package --vulnerable to scan a solution, I'd like to suppress vulnerabilities per project, rather than solution-wide. For example, by defining an MSBuild item <NoWarnVulnerability Include="https://github.com/advisories/GHSA-cmhx-cq75-c4mj" /> in the project file or in a Directory.Build.props file. This way, the suppression would only apply to those projects in which the risk has been assessed. If a developer in the team starts using the same vulnerable package in another project, then we'd need to assess that separately.

Alternatively, I suppose my build system could convert the dotnet list package --format json output to SARIF and then run SARIF tools to suppress individual issues. I may have to implement that conversion anyway, in order to visualize the results in Jenkins.

@zivkan
Copy link
Member

zivkan commented Aug 18, 2023

We're definitely going to add something, especially with the NuGetAudit feature. But we haven't yet started seriously thinking about designs.

I wish there was an easy way for me to do a poll with interested people, to see how common the wish is for per-project suppressions of specific vulnerabilities/advisories. I also wish there was a way to get "useful in practise" vs "I think I'd use it, but if I check in 12 months from now, I won't actually use it".

Unfortunately, I'm prejudiced against MSBuild. I think it's slow. At least for the use cases that NuGet needs, which is basically a declarative file showing project configuration. I was trying to gather some simple evidence, but I found something super weird. dotnet msbuild -restore:false -t:DoesNotExist on NuGet.CLient's NuGet.sln takes 6-8 seconds on my machine, which I think is far too slow. However I used DotesNotExist because it was my first idea to measure evaluation performance. I didn't want to have a target's execution time count. But if instead I run the CollectPackageReferences target, then the same solution completes in 0.5s, which I think is great and would make me no longer concerned about MSBuild performance. But when I debug restore, I do see those multi-second evaluations before NuGet's RestoreTask gets executed. 😕

Anyway, if we put the ignored advisories in nuget.config, then it's loaded once, and shared across all projects. No concern about duplicated strings increasing memory usage and additional garbage collections. Great for performance, but doesn't have the flexibility to ignore per-project. We have some customers with 1000+ project repos, so 1ms per project is 1 second of CI time, per build, and restore related properties still get evaluated when build or publish targets are run.

If per-project is genuinely useful, then we should absolutely do it. I'm just trying to guess how common your scenario is across all .NET developers worldwide. And also what on earth the DoesNotExist vs CollectPackageRefernces target performance differences mean 😕

@KalleOlaviNiemitalo
Copy link

I was thinking the per-project suppressions could be scanned by dotnet restore, which evaluates the project anyway in order to find PackageReference items, and copied to project.assets.json.

@KalleOlaviNiemitalo
Copy link

And also what on earth the DoesNotExist vs CollectPackageRefernces target performance differences mean 😕

Perhaps related to how CollectPackageReferences is defined in "NuGet.targets" imported by "SolutionFile/ImportAfter/Microsoft.NuGet.ImportAfter.targets", but DoesNotExist is not defined and has to be generated for each ProjectInSolution by "src/Build/Construction/Solution/SolutionProjectGenerator.cs" in MSBuild.

@zivkan
Copy link
Member

zivkan commented Aug 21, 2023

Yes, after talking to the MSBuild team, I learned that when a target exists in the metaproj (a temporary "project" file that represents the sln file), then it only runs on the metaproj and is not repeated on every project individually. This is how restore manages to run on a solution, and not on every project individually.

Adding DoesNotExist as a target (which does nothing) in Directory.Build.targets, I confirmed the execution time is still 6-8 seconds on my work laptop.

Adding 100 additional items into Directory.Build.props, the MSBuild evaluation performance appears roughly the same (there's a lot of run-to-run variance, and I wasn't saving the durations to do a statistical analysis), so I guess that's evidence that a few more items won't have a meaningful impact on evaluation performance, which is one of my biggest concerns.

@KalleOlaviNiemitalo
Copy link

Re excluding vulnerabilities via MSBuild, I'd prefer doing that via an item type rather than via a property, for these reasons:

  • easy to add a comment (or metadata) to justify each exclusion
  • easy to remove an exclusion that was added by an outer Directory.Build.props

@KalleOlaviNiemitalo
Copy link

Having the exclusions in MSBuild rather than in NuGet config would also make it possible to set a Condition that checks $(TargetFramework)… except I suppose that would not work with exclusions of vulnerabilities of MSBuild SDKs that MSBuild loads from NuGet, as those have to be restored before the conditions are evaluated.

@zivkan zivkan changed the title [Feature]: Allow excluding vulnerabilities from output of dotnet list package --vulnerable [Feature]: Allow excluding vulnerabilities from output of dotnet list package --vulnerable and NuGetAudit Dec 22, 2023
@zivkan
Copy link
Member

zivkan commented Dec 22, 2023

FWIW, I renamed the title of this issue to include NuGetAudit.

There's also a design spec available:

@nkolev92 nkolev92 removed Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. Triage:NeedsTriageDiscussion labels Feb 22, 2024
@nkolev92 nkolev92 added Priority:2 Issues for the current backlog. and removed Pipeline:Icebox labels Feb 22, 2024
@zivkan zivkan changed the title [Feature]: Allow excluding vulnerabilities from output of dotnet list package --vulnerable and NuGetAudit [Feature]: Allow excluding vulnerabilities from output of dotnet list package --vulnerable Jun 25, 2024
@zivkan
Copy link
Member

zivkan commented Jun 25, 2024

This feature request was created before NuGetAudit was available as a feature.

NuGetAudit was added to the .NET 8.0.100 SDK, and can report packages with known vulnerabilities at restore time for both direct and transitive packages (configurable). NuGetAuditSuppress will be available for NuGetAudit starting from the .NET 8.0.400 SDK.

Therefore, in order to free up capacity to work on other things, we're deprioritizing dotnet list package --vulnerable and waiting to see if more customers upvote this issue.

@zivkan zivkan added Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. and removed Priority:2 Issues for the current backlog. labels Jun 25, 2024
@nkolev92 nkolev92 added help wanted Considered good issues for community contributions. Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. and removed Triage:NeedsTriageDiscussion Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. labels Jul 22, 2024
@loop-evgeny
Copy link
Author

Thanks! When is SDK 8.0.400 expected to be released?

I see https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1901-nu1904 links to this issue, saying "The initial release of NuGetAudit does not provide a way to suppress specific advisories (URLs). It is a feature we intend on adding based on prioritization of other improvements." It would be good to add an update there about NuGetAuditSuppress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Functionality:ListPackage dotnet.exe list package help wanted Considered good issues for community contributions. Priority:2 Issues for the current backlog. Product:dotnet.exe Triage:NeedsDesignSpec Type:Feature
Projects
None yet
Development

No branches or pull requests

7 participants