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

Enable NuGet Audit #15018

Merged
merged 7 commits into from
Aug 26, 2024
Merged

Enable NuGet Audit #15018

merged 7 commits into from
Aug 26, 2024

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Aug 21, 2024

See #15019

To double check:

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 21, 2024

First vulnerability warning:

error NU1903: (NETCORE_ENGINEERING_TELEMETRY=Restore) Warning As Error: Package 'Microsoft.IO.Redist' 6.0.0 has a known high severity vulnerability, GHSA-hq7w-xv5x-g34j

We don't directly reference that package so I did the following to find out where it's coming from:

@ViktorHofer ➜ /workspaces/arcade (ViktorHofer-patch-1) $ ./.dotnet/dotnet nuget why src/VersionTools/Microsoft.DotNet.VersionTools.Tasks/Microsoft.DotNet.VersionTools.Tasks.csproj Microsoft.IO.Redist
Project 'Microsoft.DotNet.VersionTools.Tasks' has the following dependency graph(s) for 'Microsoft.IO.Redist':

  [net472]
   │  
   ├─ Microsoft.Arcade.Common (v9.0.0-dev)
   │  └─ Microsoft.Build.Utilities.Core (v17.8.3)
   │     └─ Microsoft.IO.Redist (v6.0.0)
   ├─ Microsoft.Build.Tasks.Core (v17.8.3)
   │  ├─ Microsoft.Build.Utilities.Core (v17.8.3)
   │  │  └─ Microsoft.IO.Redist (v6.0.0)
   │  └─ Microsoft.IO.Redist (v6.0.0)
   └─ Microsoft.DotNet.VersionTools (v9.0.0-dev)
      └─ Microsoft.Arcade.Common (v9.0.0-dev)
         └─ Microsoft.Build.Utilities.Core (v17.8.3)
            └─ Microsoft.IO.Redist (v6.0.0)

  [net9.0]
   │  
   └─ No dependency graph(s) found for this target framework.

So that package is a dependency of Microsoft.Build.Utilities.Core/17.8.3 which we directly reference. Nuget.org tells us that Microsoft.IO.Redist/6.0.1 is already available but a newer Microsoft.Build.Utilities.Core package with that updated dependency isn't.

That means we need to pin the transitive dependency to the higher version. "Luckily (it was me) Arcade already uses Central Package Management (CPM) so we can easily do that. See commit 2.

@riarenas
Copy link
Member

riarenas commented Aug 21, 2024

First vulnerability warning:

error NU1903: (NETCORE_ENGINEERING_TELEMETRY=Restore) Warning As Error: Package 'Microsoft.IO.Redist' 6.0.0 has a known high severity vulnerability, GHSA-hq7w-xv5x-g34j

I think I got this one just now! a3756ed
This seems useful

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 21, 2024

Second vulnerability warning:

    /workspaces/arcade/src/VersionTools/Microsoft.DotNet.VersionTools.Tasks/Microsoft.DotNet.VersionTools.Tasks.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.XliffTasks/Microsoft.DotNet.XliffTasks.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.SharedFramework.Sdk/Microsoft.DotNet.SharedFramework.Sdk.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Helix/Sdk/Microsoft.DotNet.Helix.Sdk.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Helix/JobSender/Microsoft.DotNet.Helix.JobSender.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Helix/Client/CSharp/Microsoft.DotNet.Helix.Client.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Deployment.Tasks.Links/Microsoft.DotNet.Deployment.Tasks.Links.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Build.Tasks.Workloads/src/Microsoft.DotNet.Build.Tasks.Workloads.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Build.Tasks.Packaging/src/Microsoft.DotNet.Build.Tasks.Packaging.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Build.Tasks.Feed/Microsoft.DotNet.Build.Tasks.Feed.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Microsoft.DotNet.Arcade.Sdk/Microsoft.DotNet.Arcade.Sdk.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w
    /workspaces/arcade/src/Common/Microsoft.Arcade.Common/Microsoft.Arcade.Common.csproj : error NU1903: Warning As Error: Package 'System.Text.Json' 8.0.0 has a known high severity vulnerability, https://github.com/advisories/GHSA-hh2w-p6rv-4g7w

That warning only shows up for MSBuild task projects or dependencies of it.

@ViktorHofer ➜ /workspaces/arcade (ViktorHofer-patch-1) $ ./.dotnet/dotnet nuget why src/Microsoft.DotNet.Helix/Client/CSharp/Microsoft.DotNet.Helix.Client.csproj System.Text.Json
Project 'Microsoft.DotNet.Helix.Client' has the following dependency graph(s) for 'System.Text.Json':

  [net472]
   │  
   └─ Azure.Core (v1.41.0)
      ├─ System.ClientModel (v1.0.0)
      │  ├─ System.Memory.Data (v1.0.2)
      │  │  └─ System.Text.Json (v8.0.0)
      │  └─ System.Text.Json (v8.0.0)
      ├─ System.Memory.Data (v1.0.2)
      │  └─ System.Text.Json (v8.0.0)
      └─ System.Text.Json (v8.0.0)

  [net9.0]
  [netstandard2.0]
   │  
   └─ Azure.Core (v1.41.0)
      ├─ System.ClientModel (v1.0.0)
      │  ├─ System.Memory.Data (v1.0.2)
      │  │  └─ System.Text.Json (v9.0.0-preview.6.24327.7)
      │  └─ System.Text.Json (v9.0.0-preview.6.24327.7)
      ├─ System.Memory.Data (v1.0.2)
      │  └─ System.Text.Json (v9.0.0-preview.6.24327.7)
      └─ System.Text.Json (v9.0.0-preview.6.24327.7)

So, only the .NET Framework side is impacted, interesting. This is because MSBuild .NET Framework tasks can't go higher than what's defined by the binding redirects. See

<PackageVersion Update="System.Text.Json" Version="8.0.0" />

Looking at https://github.com/dotnet/msbuild/blob/b5fcc152e1df4023af8b8e7317e29d263beeeaa8/src/MSBuild/app.config#L97 we can see that binding redirects for the non-vulnerable version (8.0.4) already exist. What is unfortunate here is that this change happened only a month ago and so we can't yet rely on it as by doing so, we would require a very recent version of MSBuild (probably a dogfood build of 17.12).

That effectively means that we can't yet upgrade to the non-vulnerable 8.0.4 version. So we need to suppress this advisory (and later in Component Governance again) and defer the upgrade by ~1 month when a newer version of VS with the updated .NET Framework MSBuild is consumed. See commit 3.

cc @rainersigwald

mmitche
mmitche previously approved these changes Aug 21, 2024
@rainersigwald
Copy link
Member

This is because MSBuild .NET Framework tasks can't go higher than what's defined by the binding redirects.

They can if you're willing to redist the assembly next to your task, and in fact it looks like Microsoft.DotNet.VersionTools.Tasks (I checked 9.0.0-beta.24421.2) is doing this--so you should be ok to bump the ref--the only problem with doing so would be if you need internal binding redirects within your task dependencies. Those may have low references unified to the MSBuild-provided version but current references to the version you compile against. The SDK has hit that a couple of times and unfortunately I don't know of a great way to scan for it.

@ericstj
Copy link
Member

ericstj commented Aug 21, 2024

They can if you're willing to redist the assembly next to your task, and in fact it looks like Microsoft.DotNet.VersionTools.Tasks (I checked 9.0.0-beta.24421.2) is doing this--so you should be ok to bump the ref--the only problem with doing so would be if you need internal binding redirects within your task dependencies. Those may have low references unified to the MSBuild-provided version but current references to the version you compile against. The SDK has hit that a couple of times and unfortunately I don't know of a great way to scan for it.

No we should not do this. Tasks should not upgrade the host. We should not carry a new assembly that we don't even use.

One trick we could use for this now, in lieu of an actual MSBuild task SDK would be to reference the MSBuild assemblies via PackageDownload. That would minimize the dependency closure of theirs that we bring in, while still getting the references to MSBuild we actually need.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 21, 2024

and in fact it looks like Microsoft.DotNet.VersionTools.Tasks (I checked 9.0.0-beta.24421.2) is doing this

Yes, you are correct. That's a bug in the msbuild tasks package authoring here in Arcade:

<!-- Don't include assemblies that MSBuild ships with. -->
<ItemGroup>
<PackageReference Update="Microsoft.Build" Publish="false" />
<PackageReference Update="Microsoft.Build.Framework" Publish="false" />
<PackageReference Update="Microsoft.Build.Tasks.Core" Publish="false" />
<PackageReference Update="Microsoft.Build.Utilities.Core" Publish="false" />
<PackageReference Update="Microsoft.NET.StringTools" Publish="false" />
<PackageReference Update="System.Resources.Extensions" Publish="false" />
</ItemGroup>
<!-- Don't include assemblies that are provided by the SDK, next to MSBuild. -->
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' and '$(SkipSDKInboxPublishExcludes)' != 'true'">
<PackageReference Update="Newtonsoft.Json" Publish="false" />
<PackageReference Update="NuGet.Commands" Publish="false" />
<PackageReference Update="NuGet.Common" Publish="false" />
<PackageReference Update="NuGet.Configuration" Publish="false" />
<PackageReference Update="NuGet.Frameworks" Publish="false" />
<PackageReference Update="NuGet.Packaging" Publish="false" />
<PackageReference Update="NuGet.ProjectModel" Publish="false" />
<PackageReference Update="NuGet.Versioning" Publish="false" />
<PackageReference Update="System.CodeDom" Publish="false" />
<PackageReference Update="System.Security.Cryptography.ProtectedData" Publish="false" />
<PackageReference Update="System.Text.Encoding.CodePages" Publish="false" />
</ItemGroup>
<!-- Don't include assemblies that are inbox in Desktop MSBuild -->
<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
<PackageReference Update="Microsoft.Bcl.AsyncInterfaces" Publish="false" />
<PackageReference Update="System.Buffers" Publish="false" />
<PackageReference Update="System.Collections.Immutable" Publish="false" />
<PackageReference Update="System.Memory" Publish="false" />
<PackageReference Update="System.Numerics.Vectors" Publish="false" />
<PackageReference Update="System.Reflection.Metadata" Publish="false" />
<PackageReference Update="System.Reflection.MetadataLoadContext" Publish="false" />
<PackageReference Update="System.Runtime.CompilerServices.Unsafe" Publish="false" />
<PackageReference Update="System.Text.Encodings.Web" Publish="false" />
<PackageReference Update="System.Text.Json" Publish="false" />
<PackageReference Update="System.Threading.Channels" Publish="false" />
<PackageReference Update="System.Threading.Tasks.Dataflow" Publish="false" />
<PackageReference Update="System.Threading.Tasks.Extensions" Publish="false" />
<PackageReference Update="System.ValueTuple" Publish="false" />
</ItemGroup>
<ItemGroup>
<!--
Update all PackageReference items to default Publish to true.
This forces the publish output to contain the dlls.
-->
<PackageReference Update="@(PackageReference)">
<Publish Condition="'%(PackageReference.Publish)' == ''">true</Publish>
<ExcludeAssets Condition="'%(PackageReference.Publish)' == 'false'">runtime</ExcludeAssets>
</PackageReference>

It only excludes direct package dependencies, not transitive ones. Unsure how to fix this, though.

Filed #15022

@ericstj
Copy link
Member

ericstj commented Aug 21, 2024

Should we add WarningsNotAsErrors like @zivkan mentioned in #15019 (comment) so that the build doesn't break on patch-Tuesday? I know we used to have that characteristic with harvesting (build breaks when new packages are published) and it had a lot dissatisfaction from developers. It's tough though, because we also want high-visibility of these.

@ViktorHofer
Copy link
Member Author

Let's not add WarningsNotAsErrors initially and try this out. I like that when there's a new vulnerability (even if it's a false positive), the build starts to break and requires a response. That keeps us clean on warnings but also on post build reports like with Component Governance.

If this starts to significantly disturb our product composition, we can always go back and add <WarningsNotAsErrors>$(WarningsNotAsErrors);NU1901;NU1902;NU1903</WarningsNotAsErrors to Arcade's Directory.Build.props.

cc @mmitche for opinions

@ViktorHofer
Copy link
Member Author

Before I merge this PR I would like to get an explicit ack that this strategy is ok. As mentioned we can always change direction if necessary.

@ViktorHofer
Copy link
Member Author

I had a chat with @akoeplinger and he convinced me that promoting NuGetAudit warnings to errors by default is too noisy and could halt productivity. I adjusted the code path to only fail official builds to guarantee that we aren't producing official bits without NuGetAudit enabled. In all other cases (dev & PR) NuGetAudit will emit warnings.

@ViktorHofer
Copy link
Member Author

Filed #15031 to track upgrading STJ to 8.0.4

@ViktorHofer ViktorHofer merged commit e3bdd9a into main Aug 26, 2024
11 checks passed
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-1 branch August 26, 2024 15:45
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.

5 participants