Update dependencies to fix CVE warnings

[varon]

These warnings were cropping up on build.

Untested, but should work given that they are minor updates aside from Newtonsoft, which should be okay according to this: #363 (comment)

• Update Newtonsoft.Json to fix CVE-2024-21907
• Update System.Formats.Asn1 to fix CVE-2024-38095
• Update System.Security.Cryptography.Pkcs to fix CVE-2023-29331

This is required because Fable.Remoting.Json is a dependency of Snowflaqe, which is code-gen based (so we cannot patch the newtonsoft version unless we work our way back up the dependency tree here).

[varon]

@Zaid-Ajaj - bump!

[kerams]

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.

[kerams]

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.

[varon]

@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.

[kerams]

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

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.

[varon]

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.

[Zaid-Ajaj]

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.

[Zaid-Ajaj]

@varon @kerams will be checking out PRs soon, just low on time

[Zaid-Ajaj]

Merged and published @varon