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

Microsoft.Extensions.Hosting has a transitive vulnerability (System.Text.Json) #107141

Closed
crone66 opened this issue Aug 29, 2024 · 15 comments
Closed

Comments

@crone66
Copy link

crone66 commented Aug 29, 2024

Description

As the title says Microsoft.Extensions.Hosting 8.0.0 has a transitive vulnerable dependcy of System.Text.Json.

Reproduction Steps

See warning in nuget explorer in visual studio with a reference to Microsoft.Extensions.Hosting 8.0.0

Expected behavior

A new nuget package without vulnerability is available

Actual behavior

The newest package contains a dependency to a vulnerable package

Regression?

No response

Known Workarounds

Package System.Text.Json can be upgraded independently

Configuration

No response

Other information

No response

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Aug 29, 2024
Copy link
Contributor

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

@julealgon
Copy link

Can't you just upgrade System.Text.Json independently?

Surely this should at least be in the "Known Workarounds" section.

@crone66
Copy link
Author

crone66 commented Aug 29, 2024

you are right

Can't you just upgrade System.Text.Json independently?

Surely this should at least be in the "Known Workarounds" section.

Yes forgot that to mention. Thank you for the hint. I will update the issue.

@colejohnson66
Copy link

colejohnson66 commented Aug 29, 2024

Microsoft has a policy of not releasing new versions if the only change is due to a vulnerable transitive dependencies. This has been discussed many times, and they refuse to change their policy. The current discussion is at #105028.

@julealgon
Copy link

If every single dependent library needed a version update when a vulnerability was found that would lead to a massive explosion in the number of package versions, and would be much harder to manage than just updating the underlying dependent package.

Imagine, especially for something as general purpose as System.Text.Json, that when a vulnerability was detected there, every single NuGet that depends on it was then also marked as "vulnerable" and a new version was released for those. Now suddenly instead of having to update just a single package (the problematic version of System.Text.Json) you'd have to update potentially dozens of packages that depend on it, while still potentially having to update STJ itself if you also reference it explicitly.

Additionally, imagine that one (or many) of those dozens of packages that you were forced to update hasn't been updated in your project for a while... now you are also forced to deal with any breaking changes and potential bugs introduced with that upgrade, dramatically increasing risk and QA effort.

It just doesn't scale, at all.

The new NuGet vulnerability warnings should be enough to trigger consumers to update the affected specific packages.

@crone66
Copy link
Author

crone66 commented Aug 30, 2024

First of all a runtime package should never be vulnerable by default. It just distrubing even think about it... no suprise that Microsoft has so many security issues with azure if thats there stand on security.

Regarding breaking changes if you want to stay on a older version nothing prevents you from updating just the transitive package but it doesn't change the fact that the most recent package shouldn't be vulnerable by default. Especially since it ignores different skill/knowledge levels of developers. A junior developer probably won't even notice possible security issues.

Additionally the nuget tooling and visual studio integration is not ready for bigger solution. If you have a solution with 100 Projects you have a big issue. Visual Studio shows on a solution level only warnings for top level packages not for transitives. Therefore, a developer has to check 100 projects manually... Now you might say just use the nuget cli (which we do in our pipeline). But the output of the nuget cli doesn't show you what the source package of the vulnerable transitive package is. You now have to go to the project hover over the transitive package to find the source package and first check the top level package for updates before manually updatingthe transitive package. Doing that on 100 projects is ridiculously time consuming.

Another problem is that with updating transitive packages you convert them to explicit references. Let's assume the top level package updates and completely removes the reference to the transitive package since it's now longer needed. The result is that you have over time a lot of dead/unsed nuget packages referenced in your solution and It will take a lot of time to check if they are still needed or not.

Additionally if you update the top level package that requires a higher version of your explicitly referenced transitive package you will get a version conflict and need to deal and think avout something that shouldn't exists in the first place.

Runtime packages shouldn't be vulnerable by default and there shouldn't be discussions about that in the first place. If Microsoft doesn't care about security, every company that cares about security should abandoned all Microsoft products as soon as possible.

@julealgon
Copy link

@crone66 to me, you are overreacting to the problem. There are native vulnerability warnings that will show up both on Visual Studio's UI as well as compiler warnings on build that will warn you whenever any direct or transitive dependency has been marked as vulnerable on NuGet.org, and you can immediately upgrade it then.

Forcing every single dependent package to also be marked as vulnerable and force them to have their versions bumped because of a dependent vulnerable package simply does not scale, IMHO.

Regarding breaking changes if you want to stay on a older version nothing prevents you from updating just the transitive package but it doesn't change the fact that the most recent package shouldn't be vulnerable by default.

That wouldn't work very well. I could upgrade the dependent package, but if the parent package is also marked as vulnerable because of the dependency, all existing warning systems would constantly keep bugging me about upgrading those as well even though I've already "fixed" the actual vulnerability by upgrading the transitive dependency.

A junior developer probably won't even notice possible security issues.

They would basically have to be blind not to notice the obvious warning every time they loaded the solution:
image

But regardless, that's also probably why it would be a good reason to avoid having your entire engineering team be junior devs. Our process would immediately capture these problems just by the nature of using TreatWarningsAsErrors during build. I'd recommend using that, or at least considering just the specific warning codes for vulnerability as errors and you should also be able to capture the problem instantly when it first happens.

@crone66
Copy link
Author

crone66 commented Aug 30, 2024

They would basically have to be blind not to notice the obvious warning every time they loaded the solution:

You completely ignore that fact that the nuget explorer on solution level doesn't show the transitive vulnerabilities. It works for small solutions or especially single project solutions great but not on big solutions.

Another fact that you ignore that other IDEs such as vs code exist... how should I Identify the source (top level package) of a vulnerable transitive package? The nuget cli doesn't output such information

Forcing every single dependent package to also be marked as vulnerable and force them to have their versions bumped because of a dependent vulnerable package simply does not scale, IMHO.

Instead of the source fixes the issue every developer has to fix it themself? What a waste of time. It is even possible to automate it.

That wouldn't work very well. I could upgrade the dependent package, but if the parent package is also marked as vulnerable because of the dependency, all existing warning systems would constantly keep bugging me about upgrading those as well even though I've already "fixed" the actual vulnerability by upgrading the transitive dependency.

What are you talking about? Updating the transitive packages is the only workaround currently! Now your arguing that this doesn't work... doesn't make sense at all. I never said that that packages that have vulnerabilities need to be marked as vulnerable.

But regardless, that's also probably why it would be a good reason to avoid having your entire engineering team be junior devs.

Not only big companies with big teams develop software. Never heared about these open source software developers that maintain alone packages that everyone depends on 😆 ? They might not be senior because even very simple packages such as leftPad (different ecosystem but still true for .net) exist.

@RenderMichael
Copy link
Contributor

Putting aside whether a package should be marked as vulnerable if its dependency is vulnerable, isn’t it standard practice for packages to bump the version of a vulnerable dependency? The statement about “you'd have to update potentially dozens of packages that depend on it” doesn’t make sense to me. Adding an explicit reference should be a workaround in cases like the one mentioned, about avoiding breaking changes because the intermediate dependency hasn’t been updated in a while.

@julealgon
Copy link

@RenderMichael

... isn’t it standard practice for packages to bump the version of a vulnerable dependency?

Not sure about "standard". I believe it depends. Like @colejohnson66 mentioned above, Microsoft doesn't do it everywhere (I know they do it in places such as the Azure libraries though. There were some vulnerabilities found in Azure.Identity recently that caused a substantial ripple effect there causing every package to be upgraded basically).

The statement about “you'd have to update potentially dozens of packages that depend on it” doesn’t make sense to me.

Why not? If all libraries that depend on STJ suddenly bump their versions because the version of STJ they use had a vulnerability found, this could very well mean dozens of packages on a large project need updating. Something like STJ is used by a huge number of vendor and Microsoft-owned libraries.

Adding an explicit reference should be a workaround in cases like the one mentioned, about avoiding breaking changes because the intermediate dependency hasn’t been updated in a while.

Sure, but only if the parent package is not marked as vulnerable. So, you have to take this into account:

Putting aside whether a package should be marked as vulnerable if its dependency is vulnerable,

You can't put it aside... if the parent is marked as vulnerable, and you decide not to update them (and only update the child dependency) you'd still get vulnerability warnings for that package, wouldn't you? And that would also be misleading... since you have just fixed the vulnerability by upgrading the vulnerable child dependency.

@colejohnson66
Copy link

There is an argument that a vulnerable dependency makes your own library/application vulnerable as well, but whether that rises to the level of forcing a new version is where the debate is.

@RenderMichael
Copy link
Contributor

The main sticking point has been whether to mark a library as vulnerable over vulnerable dependencies, whether it’s theoretically vulnerable or not.

I think the best solution is to bump versions on the dependency, without marking the current version as vulnerable. That way, people can “just update” the canonical way to get past vulnerabilities, without being forced to do so if they’re not able to update for whatever reason.

Copy link
Contributor

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

@steveharter
Copy link
Member

Changed location; this is not an issue specific to Extensions-Hosting.

This is also likely a duplicate of #105120.

@ericstj
Copy link
Member

ericstj commented Sep 4, 2024

We understand that it makes it harder to update when you don't have the entire dependency chain updated. We're doing work to improve that. #105120 tracks a change that will avoid a package dependency here.

Check out https://devblogs.microsoft.com/nuget/nugetaudit-2-0-elevating-security-and-trust-in-package-management/#recommended-way-to-resolve-warnings2 for information about how to resolve transitive dependency update warnings.

@ericstj ericstj closed this as completed Sep 4, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Sep 4, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants