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.

Sign up for GitHub

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

Update dependencies to fix CVE warnings #369

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Update dependencies to fix CVE warnings #369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f17e8cf
Update Newtonsoft.Json to fix CVE-2024-21907
varon Sep 21, 2024
4a930c1
Update System.Formats.Asn1 to fix CVE-2024-38095
varon Sep 21, 2024
8fa58f9
Update System.Security.Cryptography.Pkcs to fix CVE-2023-29331
varon Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions paket.dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ nuget FSharp.Core >= 6.0.0
nuget Microsoft.AspNetCore.Server.Kestrel ~> 2.2
nuget Microsoft.AspNetCore.TestHost ~> 3.1
nuget Microsoft.IO.RecyclableMemoryStream ~> 3
nuget Newtonsoft.Json >= 12.0
nuget Newtonsoft.Json >= 13.0
nuget NUnit >= 3.12
nuget PuppeteerSharp >= 7.1
nuget Suave >= 2.6.0
nuget Giraffe >= 4.1
nuget System.Formats.Asn1 >= 6.0.1
nuget System.Security.Cryptography.Pkcs >= 6.0.4
Comment on lines +17 to +18
Copy link
Collaborator

@kerams kerams Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is going to have any effect (as these are transitive dependencies and you have not added any references in this PR) or that this is the right approach in the first place.

Imagine you're developing application A and have the following dependency chain

A
  \
  B 1.0
     \
     C 1.0
        \
        D 1.0
           \
           E 1.0

What if E 1.0 is declared as vulnerable? Should D release a new version bumping E to 1.0.1, C bumping D, B bumping C, and finally you referencing new B? Or should just B add a reference to E 1.0.1 (our scenario)? Why would it be B's responsibility? Just because it happens to be your direct dependency? What about users who don't use B, but reference C directly - they'd want the authors of C to release an update anyway. In other words, a vulnerability fix in E would trigger an update wave across the entire dependency chain.

See what I am trying to say? Should everyone adhere to this approach, every time a new vulnerability in a System.* package is discovered, millions and millions of Nuget packages would have to release new versions. As far as I am concerned, libraries should only list their direct dependencies and the minimal required versions (above, Newtonsoft.Json has also been bumped to 13, yet it works perfectly fine with 12), and should not concern themselves with transitive dependencies.

I know you're trying to solve a real pain point of the Nuget client, but this is not the way to go about it. As the end consumer you should simply use a sane package manager that has better version resolution strategies for transitive dependencies (or do what you have done in this PR, but on the level of your application). I am happy with Zaid merging anyway, so consider this just me pointing out a sustainability problem.

Copy link
Collaborator

@kerams kerams Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I've just checked the lock file of one of my applications which uses Remoting, and there is no mention of Asn1 or Pkcs. Giraffe v4 used to transitively depend on these, but this is no longer the case with v6, so this change would force me and others to download packages we no longer need.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kerams - Appreciate the long explanation. Agree with your reasoning, but there are confounding factors at play beyond just pure transitivity and engineering concerns.

As to your point, yes, all package vendors should be incentivised to update all packages. If this is painful, hopefully people will be more cautious and introduce less vulnerabilities with bad code and sprawling dependencies. Any 'BIG BAD CVE' should imply that all packages in the dependency chain have an incentive to update, as none of them would 'want to be vulnerable' - in fact, Microsoft is expected to start doing this with .NET 9 and slap some very scary warnings on package restore for anything with a transitive CVE as part of their security efforts, which would render the entire thing required by corporate dictate.

You can see this happening here: https://learn.microsoft.com/en-us/nuget/concepts/auditing-packages#running-a-security-audit-with-restore - Note that .NET 9 is defaulting to include transitive in that table.

More practically and for today, we have a compliance requirement to run dotnet list package --vulnerable --include-transitive. This needs to pass. These packages are included as transitive dependencies of Fable.Remoting, so need to be updated or removed from that dependency chain.

While I agree with you and totally understand that these changes don't do much, especially because they're only used in a testing context here, they're still triggering the warning system.

This is further complicated by the fact that we're using Snowflaqe to generate code, which means we can't just go and update the project A in the dependency chain to begin with.

If we don't want to merge this PR, then please can we ensure that projects using this correctly pass dotnet list package --vulnerable --include-transitive)

Our most likely solution if this is not merged (or we don't find some way of unfuckulating this nuget warning) is to move away from Fable.Remoting entirely as we only use a tiny subset of this library - something we really don't want to do.

See also - a blog post from microsoft about best practices: https://devblogs.microsoft.com/nuget/nugetaudit-2-0-elevating-security-and-trust-in-package-management/

To summarise - let's do whatever we can to make the warnings go away - we'll need to do that for .Net 9 anyway.

Copy link
Collaborator

@kerams kerams Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I fundamentally disagree with is auditing class libraries (even though that's what's showcased in the linked blog post).

image

This package is vulnerable in the sense that it is compatible with a version of System.Text.Json which has a CVE. It does not require such a version, merely supports it.

If you reference FSharp.SystemTextJson in your class library and run dotnet list package --vulnerable --include-transitive, you will indeed see a warning (sorry, this is not true, nuget will resolve the lowest dependency by default, v6, which isn't vulnerable - v7 is). However, why would you want to audit your internal class library? After all, it doesn't run on its own.

If you then create a net9.0 console application and reference the class library, auditing isn't going to uncover any warnings, because System.Text.Json v9 will have been resolved and no vulnerable code is going to be executing in production.

Are you required to audit every single project? In my opinion, outside of some dynamic loading scenarios, auditing class libraries is irrelevant. Returning to the example above, one might think you're at risk, but at the end of the day, you're not actually using any vulnerable versions.

I am not familiar with Snowflaqe, but if I understood the code correctly, it's generating project files, which you don't want to edit manually, right? If you really need the audit to pass for such projects (again, rather than just testing the executable projects), then I reckon it would be best that Snowflaqe inject direct references to safe versions of the packages that have CVEs. At the risk of sounding like a broken record, there will be new CVEs tomorrow, next week or next month, and we'll need to keep releasing new Snowlaqe/Remoting versions. I seriously doubt any nuget package author is interested in doing this either.

I personally use Paket, which uses highest version resulution strategy by default, making all of this a non-issue, but if you're stuck with the nuget client, maybe you could also give CPM a try? It's supposed to solve these problems by the looks of it.

Copy link
Contributor Author

@varon varon Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed answer - definitely think we're getting somewhere!

Are you required to audit every single project?

Effectively yes - we have compliance requirements that mandate a secure lifecycle + static application security testing every build. Woo hoo bureaucracy.

At the risk of sounding like a broken record, there will be new CVEs tomorrow, next week or next month, and we'll need to keep releasing new Snowlaqe/Remoting versions. I seriously doubt any nuget package author is interested in doing this either.

Most of this way of thinking is coming from the JS ecosystem, I think, where all of these practices such as ten million package updates every week are extremely common - obviously the solution is more bloat and more automation... sigh. (see nonsense like DependaBot)

And yes - we have to ensure we address those continuously. We have meticulously reduced our surface area and dependencies, and continue to shrink as we go to minimize this.

The root cause of this seems to be SnowFlaqe preventing a workaround by adding non-vulnerable packages - you're correct in asserting that - which we're really not inclined to drop as a dependency due to its general utility.

Perhaps @Zaid-Ajaj can chip in here with some comments on how he suggests we resolve this, as it's going to impact more in .NET 9 for the code-gen library - otherwise - what do you suggest to as simply as possible 'make the warnings go away' for the security audit?

Appreciating the fruitful and constructive discussion here.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there will be new CVEs tomorrow, next week or next month, and we'll need to keep releasing new Snowlaqe/Remoting versions. I seriously doubt any nuget package author is interested in doing this either.

In general I agree with @kerams on this one but I can understand where @varon is coming from. Strictly it shouldn't be needed for us to go and update every time a vulnerability is discovered but I don't mind doing it as long as it doesn't happen too often (hopefully) and I can find some time between my day job / life to look into it and do the publishing.


group AwsLambda
source https://api.nuget.org/v3/index.json
Expand Down Expand Up @@ -50,4 +52,4 @@ group Client
nuget Fable.Mocha >= 2.5
nuget Fable.SimpleJson >= 3.24.0
nuget Fable.Browser.XMLHttpRequest
nuget Fable.Core
nuget Fable.Core
Loading