dotnet / runtime

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

Indirect dependency on vulnerable System.Text.Json 8.0.0 through Microsoft.Extensions.Configuration.Json #104705

Closed xC0dex closed 1 month ago

xC0dex commented 1 month ago

Description

I have <NuGetAuditMode>all</NuGetAuditMode> enabled in my Blazor authentication project. I encountered a high severity vulnerability warning for System.Text.Json version 8.0.0 (Announcement). This package is indirectly installed through Microsoft.Extensions.Configuration.Json version 8.0.0 which is installed by using Microsoft.AspNetCore.Components.WebAssembly version 8.0.7. I'm not sure if that's the way it should be.

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.

gregsdennis commented 1 month ago

8.0.4 is available. Do you get the warning if you explicitly install that package?

xC0dex commented 1 month ago

8.0.4 is available. Do you get the warning if you explicitly install that package?

I forgot to mention this. The warning is no longer there when I install 8.0.4 directly. I wondered why vulnerable packages are indirectly installed and whether this is intentional. I just wanted to bring this issue to your attention.

xC0dex commented 1 month ago

Ups, I just saw that https://github.com/dotnet/runtime/issues/104619 would be the right place for mentioning this, sorry!

eiriktsarpalis commented 1 month ago

Duplicate of #104669. We don't typically update packages because of a vulnerable dependency. The recommendation is to explicitly reference the patched STJ version.

julealgon commented 1 month ago

The recommendation is to explicitly reference the patched STJ version.

@eiriktsarpalis shouldn't NuGet give lower priority to vulnerable packages when auto-resolving a transitive dependency though?

Why would it explicitly pick a vulnerable version if there is a newer patch version that is not vulnerable that fits the version range criteria for the parent?

If the user doesn't explicitly install the transitive package (which the vast majority of people won't, especially if it's various levels of transitivity deep) then NuGet should just have a safer default.

eiriktsarpalis commented 1 month ago

shouldn't NuGet give lower priority to vulnerable packages when auto-resolving a transitive dependency though?

I don't think this should be done automatically, primarily because NuGet wouldn't be able to determine if the next version contains breaking changes. It should be up to the user to either suppress the warning or explicitly bump the transitive dependency.

Clockwork-Muse commented 1 month ago

if there is a newer patch version that is not vulnerable that fits the version range criteria for the parent?

This should be viable, though, if NuGet had some way to tell the package was vulnerable (or the repository didn't return the vulnerable versions for range searches)? If we excluded the affected versions from the list the normal resolution should take over?

julealgon commented 1 month ago

Another thing to keep in mind here is that maintaining the indirect dependencies explicitly adds a ton of maintenance burden on consumers.

I just went through a process where we shifted from the old packages.config way of handling dependencies (where you needed to explicitly reference the entire dependency graph), to the modern PackageReference approach where transitive dependencies can be inferred. We painstakingly streamlined our list of dependencies to only have what we really needed directly while allowing NuGet to automatically resolve transitive ones.

I'd rather not go back and have to maintain transitive dependencies again explicitly. I think that defeats a significant purpose of using PackageReferences in the first place.

If a package version is vulnerable, NuGet should remove that from the pool of "available choices" when selecting transitive dependencies automatically, and only in the case that there is no non-vulnerable replacement should it fallback to adding the vulnerable package with a warning.

eiriktsarpalis commented 1 month ago

I think this would be an issue to bring up with the NuGet team. My main concern is that versioning semantics are not consistent for all maintainers. NuGet should not implicitly assume that a patch version increment is compatible.

I'd rather not go back and have to maintain transitive dependencies again explicitly.

That's understandable but to be fair central package management does alleviate that burden to an extent.