Skip to content

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Aug 26, 2024

Contributes to dotnet/arcade#15019 Fixes dotnet/source-build#4577

Also delete unused feeds.

Before merging, we should add this line to the repo's Directory.Build.props file:

    <!-- Only upgrade NuGetAudit warnings to errors for official builds. -->
    <WarningsNotAsErrors Condition="'$(OfficialBuild)' != 'true'">$(WarningsNotAsErrors);NU1901;NU1902;NU1903;NU1904</WarningsNotAsErrors>

Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be backported to 9.0?

@ViktorHofer
Copy link
Member Author

The PR is throwing a lot of errors for old packages. I'm not sure how to respond to these. I.e.

__w/1/s/artifacts/sb/src/src/referencePackages/src/system.security.cryptography.xml/7.0.1/System.Security.Cryptography.Xml.7.0.1.csproj : error NU1903: Package 'System.Formats.Asn1' 7.0.0 has a known high severity vulnerability, GHSA-447r-wph3-92pm [/__w/1/s/.dotnet/sdk/9.0.100-preview.7.24407.12/NuGet.targets]

There is no patched System.Formats.Asn1 in the 7.0.x version range.

@ViktorHofer
Copy link
Member Author

@MichaelSimons how do you deal with CG reports for SBRP?

@MichaelSimons
Copy link
Member

@MichaelSimons how do you deal with CG reports for SBRP?

These are not being reported by CG - the report is clean and these weren't dismissed.

In the past whenever CGs are reported, we manually upgrade the references as illustrated by this PR. I don't recall a situation where CG reports an vulnerability where a package is not available.

In general I view these vulnerabilities as not applicable in the context of reference packages. It would be nice to be able to disable them. The product repos are what are vulnerable and this repo is just a reflection of what the product repos reference. The caveat would be the infra structure within this repo should still have vulnerabilities flagged.

@ViktorHofer
Copy link
Member Author

These are not being reported by CG - the report is clean and these weren't dismissed.

Interesting. Any idea why NuGetAudit reports those vulnerabilities but CG doesn't? cc @zivkan

In general I view these vulnerabilities as not applicable in the context of reference packages. It would be nice to be able to disable them. The product repos are what are vulnerable and this repo is just a reflection of what the product repos reference. The caveat would be the infra structure within this repo should still have vulnerabilities flagged.

That should be possible by setting the <NoWarn>$(NoWarn)NU1901;NU1902;NU1903;NU1904</NoWarn> property under src/referencePacakges so that these warnings don't show up but only for anything under that folder. This will silence NuGetAudit but not CG.

@ellahathaway
Copy link
Member

That should be possible by setting the <NoWarn>$(NoWarn)NU1901;NU1902;NU1903;NU1904</NoWarn> property under src/referencePacakges so that these warnings don't show up but only for anything under that folder. This will silence NuGetAudit but not CG.

I understand the reasoning for not wanting to alert for packages in src/referencePackages, but doing this will mean that dotnet/source-build#4577 does not get resolved with this PR. I still think that it's important to add some sort of safeguard from adding packages with vulnerabilities to SBRP.

@zivkan
Copy link
Member

zivkan commented Aug 26, 2024

Any idea why NuGetAudit reports those vulnerabilities but CG doesn't?

I don't know how CG works. In the past they used to look at NuGet's Global Packages Directory, but since NuGet's dependency resolver algorithm downloads packages that are culled from the final graph, I think they might have got so many complaints that they stopped doing that. So, my guess is that whatever technique they're using now might have gaps.

NuGetAudit, on the other hand, looks at the final, resolved package graph (though it currently doesn't consider PackageDownload).

There might also be a difference in which advisories the two products consider. NuGetAudit uses whatever the audit source tells us, and nuget.org uses GitHub's Advisory Database. Maybe CG curate their vulnerability database differently. For example, System.* reference assembly only packages, maybe CG don't warn about. Just a guess, as some people call these packages "false positives", despite the fact that GitHub Advisory Database have defined those packages as vulnerable, and neither NuGet client, nor nuget.org try to hide.

@ViktorHofer
Copy link
Member Author

Got it, thanks for the input. Given that the build currently fails with 642 reported vulnerabilities I'm not sure how to make progress here. Also, I will be out for the next six weeks so I'm inclined closing this PR unless someone else would like to take it over.

@MichaelSimons
Copy link
Member

Got it, thanks for the input. Given that the build currently fails with 642 reported vulnerabilities I'm not sure how to make progress here. Also, I will be out for the next six weeks so I'm inclined closing this PR unless someone else would like to take it over.

I don't see my team having bandwidth at the moment to help with this. Given there is no clear solution, I lean towards closing as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider adding vulnerability detection to PR checks in SBRP

5 participants