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

dotnet publish -r win10-x64 restores vulnerable package #12240

Closed
mattiaskagstrom opened this issue Nov 11, 2022 · 19 comments · Fixed by dotnet/SqlClient#1849
Closed

dotnet publish -r win10-x64 restores vulnerable package #12240

mattiaskagstrom opened this issue Nov 11, 2022 · 19 comments · Fixed by dotnet/SqlClient#1849

Comments

@mattiaskagstrom
Copy link

mattiaskagstrom commented Nov 11, 2022

Issue copied from: dotnet/sdk#29028

Describe the bug
My organization blocks downloads of packages with known vulnerabilities.
The application is buildable and publishable, but as soon as you add -r win10-x64 it tries to restore runtime.win7.System.Private.Uri/4.3.0.
The package is blocked due to: dotnet/announcements#112

Running sdk 6.0.x on the build agents, and 7.0.0 locally

To Reproduce
The app has this csproj:

<PropertyGroup>
	<TargetFramework>net6.0</TargetFramework>
	<RollForward>LatestMinor</RollForward>
	<Nullable>enable</Nullable>
	<ImplicitUsings>enable</ImplicitUsings>
	<AspNetCoreHostingModel>InProcess</AspNetCoreHostingModel>
	<AspNetCoreModuleName>AspNetCoreModuleV2</AspNetCoreModuleName>
</PropertyGroup>

<ItemGroup>
	<PackageReference Include="Hangfire" Version="1.7.31" />
	<PackageReference Include="Microsoft.AspNetCore.Server.IIS" Version="2.2.6" />
	<PackageReference Include="Microsoft.EntityFrameworkCore" Version="7.0.0" />
	<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="7.0.0" />
	<PackageReference Include="Microsoft.EntityFrameworkCore.Tools" Version="7.0.0">
	  <PrivateAssets>all</PrivateAssets>
	  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
	</PackageReference>
	<PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
	<PackageReference Include="NLog.Web.AspNetCore" Version="5.1.5" />
	<PackageReference Include="System.Data.SqlClient" Version="4.8.5" />
	<PackageReference Include="System.DirectoryServices.Protocols" Version="7.0.0" />
	<PackageReference Include="Trafikverket.PMSCore.Lib" Version="1.8.0.547" PrivateAssets="All" />
	<PackageReference Include="System.Text.Encodings.Web" Version="7.0.0" />
</ItemGroup>
#> dotnet publish -r win10-x64 MSBuild version 17.4.0+18d5aef85 for .NET Determining projects to restore... Failed to download package 'runtime.win7.System.Private.Uri.4.3.0' from 'https://********/nuget/Defa ultSafe/package/runtime.win7.System.Private.Uri/4.3.0'. Response status code does not indicate success: 400 (Bad Request).

Exceptions (if any)
Failed to download package 'runtime.win7.System.Private.Uri.4.3.0'

Further technical details
dotnet --info
.NET SDK:
Version: 7.0.100
Commit: e12b7af219

Runtime Environment:
OS Name: Windows
OS Version: 10.0.19042
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\7.0.100\

Host:
Version: 7.0.0
Architecture: x64
Commit: d099f075e4

.NET SDKs installed:
5.0.102 [C:\Program Files\dotnet\sdk]
5.0.201 [C:\Program Files\dotnet\sdk]
5.0.202 [C:\Program Files\dotnet\sdk]
7.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
Microsoft.AspNetCore.App 5.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.1.22 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.4 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 6.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 6.0.11 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 7.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found:
x86 [C:\Program Files (x86)\dotnet]
registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]

Environment variables:
Not set

global.json file:
Not found

@ghost
Copy link

ghost commented Nov 12, 2022

Issue is missing Type label, remember to add a Type label

@ghost ghost added the missing-required-type The required type label is missing. label Nov 12, 2022
@erdembayar
Copy link
Contributor

erdembayar commented Nov 15, 2022

@mattiaskagstrom
I can repro this, vulnerable package coming in transitive dependency.
image

Could you please take a look at https://learn.microsoft.com/en-us/nuget/consume-packages/Central-Package-Management#transitive-pinning? You can set non-vulnerable versions to avoid downloading the vulnerable version. Let me know if works for you.

@erdembayar erdembayar added the WaitingForCustomer Applied when a NuGet triage person needs more info from the OP label Nov 15, 2022
@ghost
Copy link

ghost commented Nov 15, 2022

Issue is missing Type label, remember to add a Type label

@ghost ghost added the missing-required-type The required type label is missing. label Nov 15, 2022
@mattiaskagstrom
Copy link
Author

@erdembayar I'm not working on the project that experienced the issue, but if you want it tested I can try to make that happen next week.
However the package that needs to be pinned to a higher version has the following description: "Internal implementation package not meant for direct consumption. Please do not reference directly."

@ghost ghost added WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. and removed WaitingForCustomer Applied when a NuGet triage person needs more info from the OP labels Nov 15, 2022
@erdembayar erdembayar added Type:Bug Triage:NeedsTriageDiscussion and removed missing-required-type The required type label is missing. labels Nov 15, 2022
@nkolev92
Copy link
Member

@mattiaskagstrom

What is your ask from the tooling in this case?

I understand the motivation with the vulnerable packages, but the source should not be returning a 400.
If a certain is made unavailable then it must not appear in the content resource which is usually used before NuGet tries to download a package.

@nkolev92 nkolev92 added WaitingForCustomer Applied when a NuGet triage person needs more info from the OP and removed Triage:NeedsTriageDiscussion WaitingForClientTeam Customer replied, needs attention from client team. Do not apply this label manually. labels Nov 16, 2022
@erdembayar
Copy link
Contributor

erdembayar commented Nov 16, 2022

@nkolev92 @zivkan
How does runtime dependency pull in for dotnet publish -r win10-x64? (I believe it's doing dotnet restore -r win10-x64 behind the scenes). I found it's quite hard to override the version for runtime.win7.System.Private.Uri here, because we shouldn't have direct dependency on runtime dependency. Then I tried to override a parent package bringing it.
<PackageVersion Include="runtime.any.System.Runtime" Version="4.3.1" /> or <PackageReference Include="System.Runtime" Version="4.3.1" /> still asset file contains runtime.win7.System.Private.Uri/4.3.0.

I used below csproj for repro, I removed <PackageReference Include="Trafikverket.PMSCore.Lib" Version="1.8.0.547" PrivateAssets="All" /> which is on private source.

<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
	<TargetFramework>net6.0</TargetFramework>
	<RollForward>LatestMinor</RollForward>
	<Nullable>enable</Nullable>
	<ImplicitUsings>enable</ImplicitUsings>
	<AspNetCoreHostingModel>InProcess</AspNetCoreHostingModel>
	<AspNetCoreModuleName>AspNetCoreModuleV2</AspNetCoreModuleName>
</PropertyGroup>

<ItemGroup>
	<PackageReference Include="Hangfire" Version="1.7.31" />
	<PackageReference Include="Microsoft.AspNetCore.Server.IIS" Version="2.2.6" />
	<PackageReference Include="Microsoft.EntityFrameworkCore" Version="7.0.0" />
	<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="7.0.0" />
	<PackageReference Include="Microsoft.EntityFrameworkCore.Tools" Version="7.0.0">
	  <PrivateAssets>all</PrivateAssets>
	  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
	</PackageReference>
	<PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
	<PackageReference Include="NLog.Web.AspNetCore" Version="5.1.5" />
	<PackageReference Include="System.Data.SqlClient" Version="4.8.5" />
	<PackageReference Include="System.DirectoryServices.Protocols" Version="7.0.0" />
	<PackageReference Include="System.Text.Encodings.Web" Version="7.0.0" />
        <PackageReference Include="System.Runtime" Version="4.3.1" />
</ItemGroup>
</Project>

@nkolev92
Copy link
Member

nkolev92 commented Nov 16, 2022

@erdembayar
You're mixing CPM constructs with standard PackageReference, you're using PackageVersion which is CPM only.

Runtime dependencies have evolved over the years. They used to be provided by a runtime.json, but nowadays they are provided by the SDK as PackageDownload(3.x projects and later), and should not affect the package graph.

It's possible that the project in question has a graph pulls those dependencies in.

@erdembayar
Copy link
Contributor

@erdembayar You're mixing CPM constructs with standard PackageReference, you're using PackageVersion which is CPM only.

My bad, just corrected it. End result is still same runtime dependency is resolved to vulnerable one.

@nkolev92
Copy link
Member

Yep, that's expect, as the runtime.any.System.Runtime is not a standard dependency. It appears to be brought in by Microsoft.NETCore.Targets.

To my knowledge, this is not how runtime assets are provided in .NET Core App 3.1 and later. It is a PackageReference that brings in that package, which itself requests runtime dependencies.

The guidance from the SDK team normally is to specify:

    <PackageReference Include="Microsoft.NETCore.Targets" Version="5.0.0" ExcludeAssets="all" PrivateAssets="all" />

@erdembayar
Copy link
Contributor

The guidance from the SDK team normally is to specify:

    <PackageReference Include="Microsoft.NETCore.Targets" Version="5.0.0" ExcludeAssets="all" PrivateAssets="all" />

@mattiaskagstrom
Could you try this please? On my local it resolved the problem.

Fyi @jeffkl

@zivkan
Copy link
Member

zivkan commented Nov 16, 2022

@erdembayar that's not going to work. Have a look at the explanation of NuGet's graph resolution algorithm, particularly the part about Direct Dependency Wins. NuGet first resolves the entire graph using versions specified as package dependencies, and only once the full graph is evaluated, then it resolves which version to use, for example direct dependencies. That first part of the process, evaluating the entire graph using the dependency versions, is still going to download the vulnerable version.

I'm not 100% sure, but I think the implementation of CPVM is that pinned transitive packages get represented as direct dependencies in the (internal) graph, but it still needs to see the unpinned version to know that the pinned version needs to be added as a direct dependency.

Lock files (packages.lock.json) partially resolve the issue. When a lock file is used in a restore, it doesn't check dependency versions, it only uses the lock file versions. However, when anything requires the lock file to be updated (package added, removed, or a version changed, or --force-evaulate is used), then the lock file is not used, a "normal" restore happens, and then the result gets saved into the lock file. So, again, downloading the vulnerable package will happen as part of the package graph evaulation.

@mattiaskagstrom In case you didn't follow my explanation above, unfortunately the way your organization has implemented blocking packages with known vulnerabilities is incompatible with the way that NuGet's PackageReference works. A proxy that blocks nupkg downloads via HTTP 400 responses makes Nuget think the server is broken, but it doesn't prevent the client from attempting to download that version.

A different approach that would cause fewer problems would be to use a server that does not upstream to nuget.org, instead republish packages needed to your own private feed. When a known vulnerability is discovered, delete that package version from your private feed, so that https://server/path/to/package/index.json doesn't even list it as a known version. That will cause NuGet to raise NU1605 warnings, but that will be by design of the technique used to block the vulnerable packages.

Otherwise, any package that has a dependency on a vulnerable package needs to be updated to a new version that does not depend on a vulnerable package version, and that needs to happen all the way up the package chain. That's the only way to get NuGet's "first pass" graph evaluation to avoid packages with known vulnerabilities.

@nkolev92
Copy link
Member

NuGet first resolves the entire graph using versions specified as package dependencies, and only once the full graph is evaluated, then it resolves which version to use, for example direct dependencies

It's not 100% correct. There are situations in which a transitive dependency will be pruned and not even downloaded, described within the same rule.

I think your point is the same as what I mentioned in #12240 (comment).
The server should not return 400 for a package that can't be found.

That will cause NuGet to raise NU1605 warnings, but that will be by design of the technique used to block the vulnerable packages

Probably a typo, as NU1603 will be raised instead, https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1603.

@teo-tsirpanis
Copy link

The problem is that Microsoft.Data.SqlClient is referencing some really old packages that transitively bring the vulnerable package, and these dependencies can be just removed. I opened dotnet/SqlClient#1849 that will fix this.

@ghost ghost added the Status:No recent activity No recent activity. label Dec 9, 2022
@ghost
Copy link

ghost commented Dec 9, 2022

This issue has been automatically marked as stale because we have not received a response in 14 days. It will be closed if no further activity occurs within another 14 days of this comment.

@teo-tsirpanis
Copy link

Keep open.

@nkolev92
Copy link
Member

@teo-tsirpanis I'm not sure I see the action for NuGet here.
The root cause is really about a misbehaving source, so we don't think there's a product bug here.

@teo-tsirpanis
Copy link

Hmm, true. OK, feel free to close.

@nkolev92
Copy link
Member

Closing as we don't think there's a NuGet action needed here.

cc @mattiaskagstrom

@ghost ghost removed the WaitingForCustomer Applied when a NuGet triage person needs more info from the OP label Dec 13, 2022
@nkolev92 nkolev92 added the Resolution:NotABug This issue appears to not be a bug label Dec 13, 2022
@teo-tsirpanis
Copy link

@mattiaskagstrom version 5.1.0 of Microsoft.Data.SqlClient was just released, which should fix your problem.

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

Successfully merging a pull request may close this issue.

5 participants