zivkan
commented
2 months ago I don't agree with this feature request. In my opinion, the point of vulnerability scanning (NuGet Audit) is to inform developers that their projects are using packages with known vulnerabilities, so they can make an informed decision on what actions to take. It doesn't seem right to me to allow a package author to hide relevant details from the package consumer. I understand it's well intentioned, but there are security risks when it goes wrong.
In some cases, the transitive dependency with a vulnerability cannot be updated because there's no fixed version yet or it contains breaking changes, and the users of the package aren't exposed to the vulnerability due to the way the transitive dependency is used.
That "and" is doing a lot of heavy lifting. Without it, I feel like it would be just inappropriate to allow package authors to suppress advisories of known vulnerabilities.
project referencing the packages could use the vulnerable package directly
Even with that additional condition, that the package author really has done a sufficiently detailed security analysis of the known vulnerability and ensured that the way that the use the package does not meet the vulnerable scenario, since packages are transitive by default, the package consumer might directly use the vulnerable package's APIs in a way that is vulnerable. Therefore, I don't think that this is acceptable unless the parent package's nuspec also has exclude="compile" metadata on the dependency (the project should use PrivateAssets="compile" on the PackageReference). If the vulnerable package is a diamond dependency, then every reference to the package must have both exclude="compile" and also the suppression. If even a single package lists the vulnerable package without both the exclude compile, or the suppression, then NuGet must warn the package consumer about the vulnerability.
ensuring that the project referencing the packages can't directly use the vulnerable package is non-trivial
However, PrivateAssets="compile" might not even work for packages that ship their own MSBuild props/targets. So if we want to be cautious about it, then the requirement would be for the package to have exclude="compile,build,buildTransitive", and now it's impossible for a package author to suppress an advisory on a transitive dependency that requires buildTransitive assets. But if
There's still a technical risk that a project using reflection could call the vulnerable package's APIs without compile time assets. I guess it's up for debate if someone using reflection to use a vulnerable package in a risky way is something that NuGet and build tools in general should warn about.
incorrect attestations
All of this also relies on the package author being correct that their package does not use the package with a known vulnerability in a way that exposes the vulnerability. Ignore malicious packages, because they could just do bad stuff directly, if we consider an innocent example where a developer believes they're using the vulnerable package in a safe way, but they're wrong, then package consumers of the parent package are still at risk and now NuGetAudit won't warn them.
inconsistent reporting
Another thing to consider is that some security tools will scan the global packages folder and just check if packages with known vulnerabilities are there. Other security tools might have a file hash of the dll with known vulnerabilities. If these tools report known vulnerabilities, and NuGetAudit does not, customers are going to question why, which might increase the NuGet team's support workload, but a worse case is if the customer loses confidence that NuGetAudit works and stops using it or encourages other people to stop using it. One could argue that NuGetAudit is doing a better job at suppressing "false" positives, but trust doesn't always depend on being technically or factually correct.
warn the package consumer that a package author is suppressing a security advisory
I think I'd feel better if instead of suppressing the NuGetAudit warning, it instead changed the warning code to a different value (NU1906 instead of NU1901-4), so then package consumers can make the choice to trust package authors who claim their package uses the vulnerable package in an acceptable way.
summary
TL;DR I don't think this feature proposal can go ahead without a design spec that carefully considers as many edge cases as possible, so that we can go through a security review and ensure that the feature compiles with the principals of Microsoft's Secure Future Initiative.
AndriySvyryd
JonDouglas
nkolev92
NuGet Product(s) Involved
NuGet.exe, MSBuild.exe, dotnet.exe, NuGet SDK
The Elevator Pitch
In some cases, the transitive dependency with a vulnerability cannot be updated because there's no fixed version yet or it contains breaking changes, and the users of the package aren't exposed to the vulnerability due to the way the transitive dependency is used. For such cases something like
<NuGetAuditSuppress Include="https://github.com/advisories/XXXX" Justification="" />should be flowed through in the package to the users but apply only to the dependencies brought by this package.Additional Context and Details
There should be a way to see the warnings suppressed in this way, related to https://github.com/NuGet/Home/issues/13518