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

Build warning for dependency Newtonsoft.Json 11.0.1 #2468

Open
seanterry opened this issue Nov 15, 2024 · 12 comments
Open

Build warning for dependency Newtonsoft.Json 11.0.1 #2468

seanterry opened this issue Nov 15, 2024 · 12 comments

Comments

@seanterry
Copy link

seanterry commented Nov 15, 2024

.NET 9 now generates build warnings for known vulnerabilities in transitive dependencies. This causes build failures if you treat warnings as errors.

Warning NU1903 : Package 'Newtonsoft.Json' 11.0.1 has a known high severity vulnerability, GHSA-5crp-9r3c-p9vr

I have no reason to believe this vulnerability can be exploited via Hangfire.

dotnet nuget why output:

  [net9.0]
   │  
   └─ MyProject (v1.0.0)
      ├─ Hangfire.AspNetCore (v1.8.15)
      │  └─ Hangfire.NetCore (v1.8.15)
      │     └─ Hangfire.Core (v1.8.15)
      │        └─ Newtonsoft.Json (v11.0.1)
      ├─ Hangfire.PostgreSql (v1.20.10)
      │  └─ Hangfire.Core (v1.8.15)
      │     └─ Newtonsoft.Json (v11.0.1)
      ├─ Hangfire.Pro (v3.0.4)
      │  └─ Hangfire.Core (v1.8.15)
      │     └─ Newtonsoft.Json (v11.0.1)
      └─ Hangfire.Pro.Redis (v3.0.10)
         └─ Hangfire.Core (v1.8.15)
            └─ Newtonsoft.Json (v11.0.1)

Repro steps:

  • Install .NET 9 SDK.
  • Build a net9.0 or net8.0 project containing a reference to Hangfire 1.8.15.
@odinserj
Copy link
Member

This is a general issue with NuGet itself described in this issue on GitHub: NuGet/Home#5553, feel free to upvote it. The problem is that NuGet doesn't allow developers or package authors to specify the dependency resolution behavior when installing packages. The best possible solution here currently is to install the Newtonsoft.Json or other package explicitly in your application project:

<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />

Hangfire specifies minimum required version, and all the future versions are still compatible, there are even unit tests that point to Newtonsoft.Json 13.0.3 package. Every bump of a dependency package is a breaking change and completely unnecessary, since it will force to a never-ending race with version bumps.

Duplicates #2202.

@abarducci
Copy link

I totally agree with you concerning the sentence: "Every bump of a dependency package is a breaking change...", and also the fact that "...it will force to a never-ending race with version bumps."
At the same time, I think that when a security issue is found on a specific version of a referenced library (I assume that this is the case with Newtonsoft.Json 11.0.1), all the libraries which use that unsecure version should be updated to the next secure version, if the latest is not an option, resolving the potential breaking changes issues caused by the update; or at least, this is how I do in my projects that of course, are not so widely used as your, hence I got the complexity of applying this approach.

I understand that including that:
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />

will force the build process to use that version, but I'd prefer avoid that workaround because actually I don't use directly Newtonsoft.Json in any of my projects, since I switched them to use the native System.Text.Json a long time ago and I would remain like that.

I think that between a never-ending race with version bumps and leaving the versions of external references as is forever, there is a "middle way", that in my opinion is: update them when it is necessary; and in this way, especially because there should be no breaking changes issues for this specific case: you continue to guarantee a safe solution without introducing issues for your users.

@mikernet
Copy link

mikernet commented Nov 21, 2024

I agree with the above. While bumping too often can be annoying if someone also has a direct dependency on a transitive library from your project (and thus needs to bump when you bump), there is a happy middle-ground where you bump every so often to a more recent version when tangible benefits present themselves in your dependencies (performance, security, features, etc). I don't want to have to worry about what transitive versions of dependencies are referenced if it's not something I am using directly - the libs I am referencing should pick minimum versions that work well for them without causing any issues.

@odinserj
Copy link
Member

While I agree that bumping all the dependencies whenever new versions are available for them will make it easier for some to stay on the edge and have no automated security-related warnings, for others it can end in a dependency hell, when a new version of Hangfire.Core or other package can't be used together with other packages like in the following case:

Hangfire.Core: Newtonsoft.Json ≥ 13.0.3
Application.Library: 11.0.0 ≥ Newtonsoft.Json < 12.0.0

And if every dependency should be installed with its latest version, I honestly don't understand why we need to specify versions when authoring NuGet packages.

Previously

Previously, with the packages.config-based implementation that existed before .NET Core days, it was possible to specify the dependency resolution behavior when installing a package, and with its "Highest" option we always received the highest supported versions for dependencies. Nowadays, we can't specify this when using PackageReference, and there's a long-standing issue in this in the NuGet repository – NuGet/Home#5553, and I would suggest to actively vote for it to restore the same semantics.

image

Paket manager

There's another package manager available in the .NET world, Paket. And while I don't encourage everyone switch to it, its behavior can also prove my point of view. Today I installed it and specified the following manifest file:

source https://api.nuget.org/v3/index.json

nuget Hangfire.Core
nuget Hangfire.InMemory

So when I'm installing packages, I get Newtonsoft.Json 13.0.3 version as expected. And if I specify Newtonsoft.Json 12.0.2 (for example) explicitly in the manifest (for some reason), it's installing that version instead, and Hangfire.Core still can work with it, since it supports all the preceding version.

PS C:\Users\SergeyOdinokov\RiderProjects\ConsoleApp3> dotnet paket install
Paket version 9.0.2+a9b12aaeb8d8d5e47a415a3442b7920ed04e98e0
Resolving dependency graph...
Updated packages:
  Group: Main
    - Hangfire.Core: 1.8.15 (added)
    - Hangfire.InMemory: 1.0.0 (added)
    - Microsoft.CSharp: 4.7.0 (added)
    - Microsoft.NETCore.Platforms: 7.0.4 (added)
    - Microsoft.NETCore.Targets: 5.0.0 (added)
    - Microsoft.Win32.Primitives: 4.3.0 (added)
    - NETStandard.Library: 2.0.3 (added)
    - Newtonsoft.Json: 13.0.3 (added)
    - Owin: 1.0.0 (added)
    - runtime.debian.8-x64.runtime.native.System.Security.Cryptography.OpenSsl: 4.3.3 (added)

What to do

Libraries don't have "specified" versions of their dependencies, they have "supported" versions. And they don't and shouldn't control them, since target applications may already have these dependencies for other reasons. And without full support from package managers that are responsible for resolving these dependencies, we'll always end with workarounds – either by installing highest versions manually in specific applications (causing local issues) or globally in library packages themselves (causing global issues).

I encourage everyone to vote for the referenced issue in the NuGet repository, putting all the energy there to solve the problem, instead of implementing more and more workarounds.

Otherwise we'll end with every library that specifies highest dependencies, and we'll be forced to update the whole project every time we just bump a patch version of a specific library.

@abarducci
Copy link

Hi @odinserj,
I'm totally agree with your previous post, that is crystal clear and it is valid in general.
What I would like to point out here, that is I think the reason why this thread has been opened, is a very specific issue: that is that the minimal version of Newton.Json (11.0.1), referenced by Hangfire.Core and only for who is using it with netstandard2.0 projects, has been deprecated because it contains known high severity vulnerability (GHSA-5crp-9r3c-p9vr).

As far as I can see from the Hangfire.Core.csproj file:

<ItemGroup Condition="'$(TargetFramework)'=='netstandard2.0'"> <PackageReference Include="Microsoft.CSharp" Version="4.4.0" /> <PackageReference Include="Newtonsoft.Json" Version="11.0.1" NoWarn="NU1903" /> </ItemGroup>

We could just update the version to 13.0.1, that should be the first version without known vulnerabilities:
image

What you suggest makes perfectly sense, but at the same time, imho a public library should at least declare its own references
ensuring their security when possible. Of course, maybe someone that is referencing directly Newton.Json with an old version, after upgrading Hangfire to the new version with inside an updated Newton.Json version will break, but that is caused not because we just trying to pursuit the latest version released, but to ensure the safety of all the libraries involved.

I hope I can be understood, what I mean is that there are situations where upgrading a library, potentially causing breaking changes to the users which use it, is somehow more acceptable than leaving everything as is, but keeping unsecure references that at least personally I try to avoid whenever possible. Even with a more suitable packet manager, which could "automagically" fix this issue, I would still prefer to have the library I use referencing exclusively not vulnerable versions.

@odinserj
Copy link
Member

But what to do if there's a company that has an application that's using Newtonsoft.Json of a specific outdated version, and upgrading to the latest version will cause compile-time (that's even ok) or run-time errors (more difficult to detect) for the application itself? So they updated Hangfire.Core and get an unwanted side-effect of changing their application behavior.

For example, a similar problem was with the Microsoft.Data.SqlClient (it's a dependency of Hangfire.SqlServer) package that introduced a breaking change in version 4.0.0 that ended with connectivity errors for many. When we update such package explicitly, we at least can track the source of the problem, but with a side-effect update it's not so clear. Also, some newest versions of packages simply have bugs, and it sometimes takes time to fix them.

The problem is that it's not rare for packages to fix security vulnerabilities only in their latest versions, leaving all the previous versions unfixed. And it's not clear when to bump such dependencies if we decide to have always non-vulnerable dependencies. Is it allowed in patch versions? No one expects big changes in patch versions. Is it allowed only in minor or major versions? It will take time, and workaround is still to manage transitive dependencies, until the new version is released.

image

Hangfire also have some internal dependencies that are merged and internalized, like Microsoft.Owin (for .NET Framework packages) or Dapper for Hangfire.SqlServer. They all have non-vulnerable versions, and updated under the scenes, since they are used internally and don't affect the target application.

But for external dependencies that can be already installed in a target application, I'm not sure that a framework can dictate package updates. Transitive dependencies management is required anyway to stay secure, and it perfectly solves the problem of outdated and unsecure dependencies, and is fully available. And at the same time we are sure that we can solve bugs in one package, leaving others untouched.

And with something like the following we can have advantages of both worlds, when our package manager supports this:

<PackageReference Include="Hangfire.Core" Version="1.8.15" DependencyBehavior="Highest" />

@abarducci
Copy link

Well,
I think that when a developer decides to update its projects and find something that is changed in the references, it should have all the tools to update and/or fix what could happen then; maybe I'm too optimistic, but in my experience if you are not able to update a project because of an updated version of a referenced library (and in this case we are talking about "nothing"), maybe that project has several issues and it should remain untouched at all.

Anyway, while I can agree with you if we are talking about "major systems", in this specific case we are dealing with an update that fixes known vulnerabilities, which I'm confident that won't introduce breaking changes, since as you wrote previously: "there are even unit tests that point to Newtonsoft.Json 13.0.3 package".
That's said, in my project I have just this references:

<PackageReference Include="Hangfire.AspNetCore" Version="1.8.15" />
<PackageReference Include="Hangfire.SqlServer" Version="1.8.15" />

Again, I wouldn't like to force my csproj to include:
<PackageReference Include="Hangfire.Core" Version="1.8.15" DependencyBehavior="Highest" />

I could accept to have the "DependencyBehavior" property in my direct references, but still I think it's like "cheating" in this scenario because yes: I eventually fix my vulnerability issue, but the real thing is that a library that I use is declaring to accept a vulnerable library instead of update it.

From my point of view (and of course, it is just my thought), with this approach you worry more about maintaining dated systems, rather than supporting projects that are always up to date and maintained to the state of the art. I understand this is only my issue (maybe someone else is in a situation like mine), but after upgrading to net core 9, I was forced to set to false the "TreatWarningsAsErrors" property in a solution of 76 projects only for a project that is referencing Hangfire, due to that Newtonsoft.Json vulnerability...and this is really frustrating for me, especially because I really can't see a drawback updating that reference; that's it.

@odinserj
Copy link
Member

I think that when a developer decides to update its projects and find something that is changed in the references, it should have all the tools to update and/or fix what could happen then; maybe I'm too optimistic, but in my experience if you are not able to update a project because of an updated version of a referenced library (and in this case we are talking about "nothing"), maybe that project has several issues and it should remain untouched at all.

Yes, but there are legacy projects (with almost no time budget) that you prefer to leave untouched, except one library that solves 1 bug that started to appear after running this project smoothly after 10 years. Unfortunately this happens sometimes.

Anyway, while I can agree with you if we are talking about "major systems", in this specific case we are dealing with an update that fixes known vulnerabilities, which I'm confident that won't introduce breaking changes, since as you wrote previously: "there are even unit tests that point to Newtonsoft.Json 13.0.3 package".

There are no breaking changes for Hangfire itself, and there are no breaking changes to your project, but this doesn't mean there are no breaking changes for someone else's project.

From my point of view (and of course, it is just my thought), with this approach you worry more about maintaining dated systems, rather than supporting projects that are always up to date and maintained to the state of the art. I understand this is only my issue (maybe someone else is in a situation like mine), but after upgrading to net core 9, I was forced to set to false the "TreatWarningsAsErrors" property in a solution of 76 projects only for a project that is referencing Hangfire, due to that Newtonsoft.Json vulnerability...and this is really frustrating for me, especially because I really can't see a drawback updating that reference; that's it.

I understand, my friend. But what if include the Newtonsoft.Json 13.0.3 package to your project instead of disabling the TreatWarningsAsErrors (that's completely unacceptable, I agree), until there are changes in the NuGet client itself that install/restore highest dependency versions by default when you have no transitive dependency installed explicitly in your project, or at least gives some way to specify this behavior manually?

To be honest, I'd be happy to reference only the newest package versions and stop supporting older frameworks, but there are still companies and projects that use them. Once that legacy prevents from moving forward, I'd drop such support, but this problem itself is bigger than Hangfire, and relate to other packages as well.

So I'm still suggesting to upvote the referenced feature for NuGet. Once we understand it will never be implemented, I would be happy to join the bumping race, but in my opinion it would be irresponsibly towards the NuGet ecosystem.

NuGet/Home#5553

@mikernet
Copy link

We are talking about a 6 year old package with security vulnerabilities here. There is no reason for new hangfire packages to support that. If they really need to use 6 year old vulnerable packages they can use 6 year old hangfire packages too 😆

@abarducci
Copy link

First of all, thank you @odinserj for your patience and support.
Concerning your suggestion:

I understand, my friend. But what if include the Newtonsoft.Json 13.0.3 package to your project instead of disabling the TreatWarningsAsErrors (that's completely unacceptable, I agree), until there are changes in the NuGet client itself that install/restore highest dependency versions by default when you have no transitive dependency installed explicitly in your project, or at least gives some way to specify this behavior manually?

Of course, it will be my less worse option to workaround my issue and, for sure I'll upvote the referenced feature for NuGet (NuGet/Home#5553), because I'm convinced that is a good thing. Nevertheless, as I've already written and I hope to be crystal clear and understood: even with the new package manager behavior, imho having a reference to a vulnerable library is a mistake, especially if we consider that (as @mikernet pointed out), we are talking about a 6 years old library.

Finally and again, between "reference only the newest package versions and stop supporting older frameworks" and leave all the references as is forever and ever, there should be a middle way where vulnerable libraries should be deprecated and updated, for a greater good.

@StevenQuickGS1
Copy link

StevenQuickGS1 commented Nov 22, 2024

We are talking about a 6 year old package with security vulnerabilities here. There is no reason for new hangfire packages to support that. If they really need to use 6 year old vulnerable packages they can use 6 year old hangfire packages too 😆

Yes, it seems a very niche case that someone would update project A to latest Hangfire version but then never update project B's Newtonsoft.Json in 6 years when project B depends on project A, but I guess it's possible

Also, there are not many breaking changes between 11.x and 13.x
https://github.com/JamesNK/Newtonsoft.Json/releases

I understand the rationale behind wanting to wait until Hangfire 2.x, myself I would not be surprised by if going from Hangfire 1.8 to 1.9 it increased a very stable dependency like Newtonsoft.Json from 11.x to 13.x

tldr; Hangfire 2.x lets go

odinserj added a commit that referenced this issue Nov 26, 2024
Setting `JsonSerializerSettings.MaxDepth` manually for custom serializing options to avoid requiring to upgrade Newtonsoft.Json to 13.0.1 version where GHSA-5crp-9r3c-p9vr is not present.
Relates to #2468
@odinserj
Copy link
Member

I have just committed a change to Hangfire.Core itself to set the MaxDepth property to its JsonSeriarlizerOptions objects, so it's safe to use whatever version of Newtonsoft.Json we have. This "unsafe defaults" is the only vulnerability in Newtonsoft.Json, and it's related to StackOverflowException exception when deeply nested structures are serialized, please see GHSA-5crp-9r3c-p9vr for details.

I will also bump Newtonsoft.Json to version 13.0.1 in the upcoming Hangfire 1.9.0 release (which will be started soon) as a dependency for a new platform, either net6.0 or net8.0 as other bumps already performed with this library. So both backward compatibility + safe defaults will be persisted, and we'll get rid of transient package warning on the .NET 9.0 platform.

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

No branches or pull requests

5 participants