NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

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

Open loop-evgeny opened 2 years ago

loop-evgeny commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Can I contribute some code to help make this happen?

erdembayar commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

KalleOlaviNiemitalo commented 1 year ago

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 commented 11 months ago

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

There's also a design spec available:

zivkan commented 5 months ago

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.

loop-evgeny commented 3 months ago

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.