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

[Bug]: How msbuild tasks should handle CVE-2024-38095? #10656

Open
jaredpar opened this issue Sep 12, 2024 · 7 comments
Open

[Bug]: How msbuild tasks should handle CVE-2024-38095? #10656

jaredpar opened this issue Sep 12, 2024 · 7 comments
Labels

Comments

@jaredpar
Copy link
Member

Issue Description

When building with NuGet audit enabled, MSBuild tasks targeting .NET Core will receive the following error:

Package 'System.Formats.Asn1' 7.0.0 has a known high severity vulnerability, GHSA-447r-wph3-92pm

Looking at a dotnet nuget why of the package and will end up with the following:

Project 'SemanticSearch.BuildTask' has the following dependency graph(s) for 'System.Formats.Asn1':

  [net8.0]
   │
   └─ Microsoft.Build.Tasks.Core (v17.7.2)
      ├─ System.Security.Cryptography.Pkcs (v7.0.2)
      │  └─ System.Formats.Asn1 (v7.0.0)
      └─ System.Security.Cryptography.Xml (v7.0.1)
         └─ System.Security.Cryptography.Pkcs (v7.0.2)
            └─ System.Formats.Asn1 (v7.0.0)

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

From the perspective of a MSBuild task I'm not sure how we can address this. This package comes from the .NET runtime. There is no way for us to really fix this at the msbuild task level given that the MSBuild host controls this dependency. Not sure how we can proceed here as we can't suppress this warning over the long term.

Steps to Reproduce

Create a new console project and add the following NuGet.config file

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
     <clear />
    <add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
  </packageSources>
  <auditSources>
    <clear />
    <add key="nuget.org" value="https://api.nuget.org/v3/index.json" />
  </auditSources>
</configuration>

Then run the following:

> dotnet new classlib
> cp ../NuGet.config .
> dotnet add package Microsoft.Build.Tasks.Core
> dotnet build

That will produce the following warning:

Package 'System.Formats.Asn1' 7.0.0 has a known high severity vulnerability, GHSA-447r-wph3-92pm

Expected Behavior

No warning or a method of correctly addressing the warning in the library

Actual Behavior

That will produce the following warning:

Package 'System.Formats.Asn1' 7.0.0 has a known high severity vulnerability, GHSA-447r-wph3-92pm

Analysis

N/A

Versions & Configurations

No response

@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

What I've done in runtime is cherry-pick the MSBuild assemblies when building tasks. This allows us to reference them without bringing in the closure. https://github.com/dotnet/runtime/pull/107639/files#diff-31e8e6d63141c9dcbc8ed1c2b019b92b199f2902d1856108c7effe8805542789R4

It's a temporary solution until we can get a better one like an MSBuild Task SDK, or some dedicated MSBuild task package. microsoft/MSBuildSdks#551.

I think it's possible for MSBuild to produce separate packages just for tasks that might even be version agnostic as the surface area needed by a task rarely changes.

@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

@jaredpar most folks don't actually use Asn1 and tasks tend to mark all of MSBuild references ExcludeAssets="Runtime". I think the cheapest workaround here is to do

  <PackageReference Include="System.Formats.Asn1" Version="$(SystemFormatsAsn1Version)" ExcludeAssets="All" PrivateAssets="All" />

That upgrades the version to one that's not vulnerable and prevents the task from seeing the higher version and potentially breaking out of the version from VS / MSBuild. Since the package is entirely excluded the project upgrading it cannot witness the upgrade (for example as an assembly reference) - but it does prevent the vulnerable version from being pulled by NuGet.

@jaredpar
Copy link
Member Author

Will the package still be downloaded? If so won't that still cause CG alerts that look at disk assets to be triggered?

@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

The new package would be downloaded, avoiding the CVE alert. In the above sample, I expect SystemFormatsAsn1Version to be set to the fixed version.

Typically folks don't persist the MSBuild reference for tasks (using PrivateAssets="All") so consumers of the task aren't impacted regardless. Of course for a component that actually persists the MSBuild reference this doesn't work - for example Microsoft.CodeAnalysis.Workspaces.MSBuild - but we have a different solution for that.

@ericstj
Copy link
Member

ericstj commented Sep 13, 2024

Another recommendation here -- try to remove your dependency on Microsoft.Build.Tasks.Core completely.

I suspect most people don't need this package reference. It's mostly implementation Tasks - that's why it has heavy / highly-serviced dependencies. There are a couple of non task types in here. I wonder if we could push those down to make it easier on folks?

Looking through all the types, these ones seem to be the types that aren't actual tasks and could be useful to push down.

  • CommandLineBuilderExtension
  • TaskExtension
  • ToolTaskExtension

Not generally useful nor exchange:

  • ExtractedClassName
  • IFixedTypeInfo
  • Microsoft.Build.Tasks.Deployment.Bootstrapper namespace
  • Microsoft.Build.Tasks.Deployment.ManifestUtilities namespace
  • Microsoft.Build.Tasks.Hosting namespace
  • System.Deployment.Internal.CodeSigning namespace

For reference, here's the API surface of Microsoft.Build.Tasks.Core.

cc @rainersigwald

@jaredpar
Copy link
Member Author

  • try to remove your dependency on Microsoft.Build.Tasks.Core completely.

Not realistic for roslyn. That assembly contains the ICscHost* and IVbcHost* interfaces which we have to use.

@ericstj
Copy link
Member

ericstj commented Sep 13, 2024

Yeah, and even so - that doesn't help with CVE-2024-38081 (Microsoft.IO.Redist) which is similar. I think the best option right now is ExcludeAssets="All" PrivateAssets="All". I bet that works for most people.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants