search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.92k stars 4.64k forks source link

After creating a Worker service project, "System.text.json" 8.0.0 was referenced and identified as a vulnerable package #105120

Open v-bennettyue opened 1 month ago

v-bennettyue commented 1 month ago

Repro steps

  1. dotnet new worker
  2. Open project in Visual Studio
  3. Right-client project > Manage NuGet Packages

Expected results

The project shouldn't have vulnerability warnings

Actual results

Check 'Show only vulnerable' checkbox, then you can see that the warning is because the following packages have a dependency on System.text.json 8.0.0, which has been detected as the vulnerable package

Original issue

INSTALL STEPS

  1. Clean machine: Win11 x64 23h2 ENU
  2. Install Dev17.10.4 (Include Aspire 8.0.0) latest release build
    • Web workload

REPRO STEPS

  1. File > New project > .NET Aspire App Host > .NET 8.0 > Create
  2. Right-check project > Manage NuGet Packages

ACTUAL Check 'Show only vulnerable' checkbox, then you can see that the warning is because the following packages have a dependency on System.text.json 8.0.0, which has been detected as the vulnerable package Aspire.Hosting.AppHost "Version=" 8.0.0 image

NOTE:

  1. This issue can be repro in any aspire project or in a project with aspire This issue also repro on Dev17.10 + Aspire 8.0.2/8.1 and Dev17.11 + Aspire 8.0.2/8.1

EXPECTED The packages should be updated to depend on a newer version of System.text.json that is not vulnerable.

balachir commented 1 month ago

@mitchdenny is this a known issue? Note that this also reproduces using Aspire 8.0, so this may not be a blocking issue for Aspire 8.1. But if it's a straight-forward dependency update, perhaps we should go ahead and get it done.

eerhardt commented 1 month ago

The System.Text.Json v8.0.0 dependency is coming from the following dependency graph:

image

@ericstj @eiriktsarpalis @ViktorHofer - are we planning on updating Microsoft.Extensions.Hosting to uplift the System.Text.Json dependency to a non-vulnerable version? I know in this case the NuGet package isn't going to be used (since the System.Text.Json used will come from the shared framework). But I would guess every Worker app will have this vulnerability warning now. I am able to repro it by just creating a Worker service project in VS. This makes me think this issue should be transfered to dotnet/runtime.

Thoughts?

eiriktsarpalis commented 1 month ago

I would say this is a duplicate of https://github.com/dotnet/runtime/issues/104669. The current policy is that we don't update because of transitive dependencies being vulnerable.

ericstj commented 1 month ago

Correct, it will ship with updated dependencies if it has a reason to ship.

eerhardt commented 1 month ago

The current policy is that we don't update because of transitive dependencies being vulnerable.

I agree with this policy from a library perspective - so we don't have an explosion of new versions of all libraries in the world that depend on System.Text.Json.

However, from an application perspective we are causing an issue for any application using our templates. dotnet new worker is now setting off these kinds of vulnerability warnings due to a transitive dependency on the STJ 8.0.0 nuget package. Options to address this that I can see are:

  1. We do nothing and tell customers they are responsible for "fixing up" our templates so they don't get warnings.
  2. We teach the vulnerability warnings about package conflict resolution, so they don't warn about the 8.0.0 nuget package being referenced transitively when the real System.Text.Json assembly will be loaded from the shared framework.
  3. We are super stringent about TFM dependencies - in this case Microsoft.Extensions.Configuration.Json and Microsoft.Extensions.Logging.Console shouldn't have a nuget dependency on System.Text.Json for the net8.0 TFM. It should come from the TFM net8.0.
  4. We update a targeted set of packages that are used directly in applications - in this case Microsoft.Extensions.Hosting v8.0.1 gets shipped that "lifts up" the System.Text.Json version to a non-vulnerable one. That way I just need to reference the latest Microsoft.Extensions.Hosting version in my worker app, and I no longer get security alerts.
  5. We update our templates (ex. worker and aspire) to manually "lift up" the dependency to a non-vulnerable version.

Of these options, I think (3) might be the best. I'm not sure if we could make that kind of change in a servicing release, but we should think about that policy going forward so we don't get into these kinds of situations in the future.

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @dotnet/area-extensions-hosting See info in area-owners.md if you want to be subscribed.

steveharter commented 1 month ago

The proposed fix from @eerhardt is:

We are super stringent about TFM dependencies - in this case Microsoft.Extensions.Configuration.Json and Microsoft.Extensions.Logging.Console shouldn't have a nuget dependency on System.Text.Json for the net8.0 TFM. It should come from the TFM net8.0.

Thoughts @ericstj @eiriktsarpalis?

ericstj commented 1 month ago

I think that's reasonable to avoid dependencies when they're present in the framework. I see that as a nice improvement to avoid package downloads.

ericstj commented 1 month ago

So there's a complication here. If a package needs a reference for NETStandard2.0 we may not be able to remove the package reference for older .NET versions. Consider the following package shape:

A (netstandard2.0) -> B (package - 9.0.0.0) A (net8.0) -> B (framework - 8.0.0.0 | package - 9.0.0.0) A (net9.0) -> B (framework - 9.0.0.0)

We can't remove the package reference for net8.0 because that would cause a ref-def mismatch for folks compiling on netstandard2.0 then running on net8.0 - since net8.0 only contains the 8.0.0.0 version. Even if we're OK using the API that's in 8.0 when compiling for it, we must deliver the 9.0 assembly since that's what we expose on netstandard2.0.

We can still remove the package reference for the current framework, but not past ones in this case.

v-elenafeng commented 3 weeks ago

Hi @ericstj, just one question: where (on which build) can I verify the fix for this issue? With VS 17.12 P2 (contains 9.0 P7), I was able to repro this, should it be issue 3047?

ericstj commented 3 weeks ago

We only made this change for net9.0 in RC1. If you are using targeting net9.0 and an RC1 build you should see the fix.

v-elenafeng commented 2 weeks ago

We only made this change for net9.0 in RC1. If you are using targeting net9.0 and an RC1 build you should see the fix.

Thanks! With 9.0 RC1, the dependency has been removed. For 8.0 templates in VS 17.11 and below, the package warning is only shown when checking for vulnerable packages from the nuget package manager. For 8.0 templates in VS 17.12 (where the default SDK is 9.0), we're tracking an issue https://github.com/dotnet/AspNetCore-ManualTests/issues/3047