dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.87k stars 4.63k forks source link

System.Security.Cryptography.Pkcs 8.0.1 package needed to resolve System.Formats.Asn1 vulnerability #105028

Open KirkMunroSagent opened 1 month ago

KirkMunroSagent commented 1 month ago

Description

If you take a dependency on System.Security.Cryptography.Pkcs 8.0, you will receive a package vulnerability warning due to the vulnerability identified in #104622. That warning occurs because System.Security.Cryptography.Pkcs 8.0 has a dependency on System.Formats.Asn1 >= 8.0.0, and System.Formats.Asn1 8.0.0 is the vulnerable package. System.Formats.Asn1 8.0.1 is available, and no longer contains the vulnerability.

To fix this without requiring customers to take a dependency on a nested package that isn't directly referenced in their code (customers should not have to do that), Microsoft should publish a System.Security.Cryptography.Pkcs 8.0.1 package that has a dependency on System.Formats.Asn1 >= 8.0.1.

Reproduction Steps

  1. Create a .NET 8 project that takes a dependency on the latest System.Security.Cryptography.Pkcs 8.x package.
  2. Compile your project in Visual Studio.

Expected behavior

The project will compile without any package vulnerability warnings.

Actual behavior

The project compiles, and Visual Studio identifies that you have vulnerable packages.

Regression?

No

Known Workarounds

Take a hard dependency on System.Formats.Asn1 8.0.1.

NOTE: Taking a dependency on a nested package that you do not reference in your code really isn't a viable workaround, because it adds something else to manage that may very well not be cleaned up later, and it's a transient dependency, so really the package that does reference it and use it directly needs to be updated. That's the proper fix.

Configuration

.NET 8.0 Windows 11 x64

This issue is not specific to Windows or x86/x64.

Other information

  1. It would be really nice if the NuGet Package Manager would properly identify packages that have internal dependencies that contain known vulnerabilities so that customers have someone to reach out to when a package like System.Security.Cryptography.Pkcs needs an update. Finding this so that I could come here to log the bug required opening up the obj/project.assets.json file and inspecting the contents to determine where the vulnerability warning was really coming from.
vcsjones commented 1 month ago

/cc @ViktorHofer and @ericstj. This looks like duplicate of https://github.com/dotnet/runtime/issues/98827 but with different packages.

KirkMunroSagent commented 1 month ago

@vcsjones: Thank you for drawing my attention to #98827. I had missed that when I searched for an existing issue.

Having read that issue, and the pushback against updating these packages, I thought I should share some additional perspective.

When a customer like myself takes a dependency on one of the packages that internally takes a dependency on System.Formats.Asn1, and when you use Visual Studio to build your solution, here is what the end-user experience currently looks like:

  1. Once the vulnerability in System.Formats.Asn1 is published, you are presented with the following warning in Visual Studio:

    image

  2. If you click on the Manage Nuget Packages link that is presented in the UI, you're taken to the NuGet Package Manager in Visual Studio. The NuGet Package Manager does not identify any package that you have as vulnerable. You need to look in the error log to see what warnings appear that highlight which package is vulnerable.

  3. Once you see which package is vulnerable, since the vulnerable package is a transitive package that you didn't directly add to your project, you need to figure out where that vulnerability comes from using some other means. In my case, I had to open the obj/project.assets.json file to figure it out.

    In this case, it's from the System.Security.Cryptography.Pkcs 8.0.0 package, because that package lists System.Formats.Asn1 >= 8.0.0 as a dependency, so anyone that needs System.Security.Cryptography.Pkcs in .NET 8 will currently take on a package that is vulnerable to a DDOS attack, by default (as was highlighted in the other issue).

The proposed solution seems to be for customers to take on an additional dependency, even though they don't use it directly. This is taking something that could be fixed in one location, at the root, by simply publishing a System.Security.Cryptography.Pkcs 8.0.1 package with a System.Format.Asn1 >= 8.0.1 dependency, and pushing that work out to every project, current and new, that uses this package. How that is acceptable as a proposed solution is baffling. You're literally telling your customers to fix your problem for you, individually, per project. It's a remarkably irresponsible way to handle a security vulnerability.

I'm going to wait for a reply to see if this can be changed, but if the policy is to do nothing in this scenario, that policy needs to be revisited, full stop. Your tooling (Visual Studio and the Nuget Package Manager within it) doesn't properly help customers understand the problem, and your proposed solution doesn't address the problem in such a way that customers can easily get the vulnerability repaired. How is doing nothing in this scenario, with inadequate tooling leaving customers to have to dig around to figure out what is really going on, considered an acceptable solution?

WGroenestein commented 1 month ago

Over the past couple of years, our csproj files have become littered with comments like these (it is our solution to say, we don't really want this direct dependency, but we have too, so don't delete it)

<!-- We take a direct dependency on System.Formats.Asn1 because the transitive version used by other dependencies is marked as vulnerable #{issue-number} -->

In terms of addressing these errors, we have found that the dotnet cli is the easiest to figure things out (though it will not report vulnerable versions which have been unlisted (which is another issue))

dotnet list package --vulnerable --include-transitive

This is how we deal with the tools givens (though not ideal indeed)

KirkMunroSagent commented 1 month ago

Over the past couple of years, our csproj files have become littered with comments like these

I find that so unfortunate, for something that could be so easily resolved at the root via a simple NuGet package update.

dotnet list package --vulnerable --include-transitive

I use the same command, but that just tells me which packages are vulnerable. It doesn't identify why I'm seeing the vulnerability (i.e. if it's transitive, what is the source of the problem so that I can ping the source if they're not addressing it properly). Running the same command without --include-transitive isn't any more helpful either. So far, the best way I've found to identify the source of the vulnerability is by reviewing the obj/projects.assets.json file. I'm going to write a script to automate that, because I'm simply not willing to blindly convert transitive dependencies to direct dependencies without knowing exactly which packages are the real culprit. If in the future we drop a dependency that was the true source of the problem, I don't want any dependencies that should be unnecessary left kicking around, so I need to include those packages in the comments as well so that I can keep my dependency list as short as possible over time.

KirkMunroSagent commented 1 month ago

Maybe the proper solution for Microsoft is to use floating version dependencies for Microsoft-managed packages so that the latest appropriate version is installed instead of the minimum version, which may contain vulnerabilities.

i.e. Instead of having a dependency in System.Security.Cryptography.Pkcs on System.Format.Asn1 >= 8.0.0, set that dependency up as System.Format.Asn1 >= 8.*. If you take that approach, the Nuget Package resolver should pull down the latest 8.* version that is available, allowing for packages containing vulnerabilities to be flagged accordingly and automatically superseded as those vulnerabilities are fixed.

That means you would need to publish a single System.Security.Cryptography.Pkcs package update, bringing it up to 8.0.1, but from that point forward if you don't need to make direct updates, indirect updates would automatically be picked up. As long as you limit your floating version updates to the packages that you (Microsoft) controls, and as long as no breaking changes are published within the same major version number, that would solve this problem. You could use >= 8.0.* instead of >= 8.* if you are more risk averse, because there should definitely not be any breaking changes in a 8.0.* update.

If all of that makes you too uncomfortable due to no direct testing of the updates, then I don't see another solution other than to stand behind your packages by doing the right thing and updating them when there are vulnerabilities in your dependencies. The testing argument doesn't stand up though because by not updating you're telling customers to take on these updates themselves, and expecting that to work for them, so floating version dependencies seems like the right way to go since that's essentially what you're counting on working in this situation anyway.

WGroenestein commented 1 month ago

I suspect this is what would be very useful to have: NuGet/Home#11553 (or something similar to npm ls)

JonDouglas commented 1 month ago

hey folks,

we recently blogged on this topic and how nuget can help you further on understanding transitive vulnerabilities:

https://devblogs.microsoft.com/nuget/nugetaudit-2-0-elevating-security-and-trust-in-package-management/

KirkMunroSagent commented 1 month ago

Thanks @JonDouglas, sounds like some great improvements are on their way that will make this much easier.

Based on what I read, it seems like the Supplied By Platform would make for smart elimination of some of the recent package vulnerabilities (System.Text.Json, System.Format.Asn1), as long as you have the appropriate SDK version with the fixes installed, correct? It looks like that's been in progress since 2018, so maybe there's still some opportunity to address vulnerability issues in the meantime by using floating version dependencies if that future update won't see the light of day for some time yet? It's great if it will be resolved in the future, but it seems like a bad decision to leave things the way they are in the meantime.

idontsov commented 1 month ago

@jeffhandley added this to the Future milestone 2 days ago Work on this milestone may or may not occur; it is not confirmed.

Updates to packages are not released unless there are changes within the packages themselves.

Instead of issuing a minor update to the package, you are requiring your customers to find a workaround and hope for the eventual delivery of the Supplied By Platform feature.

Could you please explain why it is challenging for you to release a minor package update?

KirkMunroSagent commented 1 month ago

@jeffhandley: I'd like to know more about the decision to tag this with the Future milestone as well.

You have a package that you offer to customers. Your package is dependent on another package that has a known vulnerability/exploit. Any of your customers that use your package automatically pick up that exploit.

Solutions already exist to solve this at the lowest level, so that your customers get the exploit fixed/corrected without needing to take on additional package dependencies that they shouldn't have to take on. One of those solutions (floating version dependencies) also allows for future exploits that show up in the packages that you are dependent on to be fixed, such that a customer would simply need to rebuild their project to get fixes to those packages as well.

Yet instead of addressing this fixable issue with an update to your package, Microsoft's recommended solution is for each customer to fix this issue themselves, because of an internal policy that seems like it should be revisited and revised, with a plan in place to fix this at some time in the future once some new functionality that has been in progress since 2018 is released and adopted.

Is that the outcome of this issue? And that's ok?

If so, I'll deal with it simply because I don't have a choice, but it certainly doesn't seem like a very responsible way to handle vulnerabilities in packages that have fixes available.

Matheos96 commented 3 weeks ago

We just stumbled upon this now recently too. I am quite honestly baffled with what I am reading, at least according to my current understanding.

I understand that the fix for us (the customer) is in theory super simple, add a direct dependency on a version where the vulnerability is fixed. However, to re-iterate what other's have said, I don't like this solution one bit as in our case we are referencing Microsoft.AspNetCore.DataProtection.Extensions which transitively does reference System.Formats.Asn1 but oh boy is that path far from short: Microsoft.AspNetCore.DataProtection.Extensions:8.0.7 --> Microsoft.AspNetCore.DataProtection:8.0.7 --> System.Security.Cryptography.Xml:8.0.1 --> System.Security.Cryptography.Pkcs:8.0.0 --> System.Formats.Asn1:8.0.0 😮 In this case, bloating our .csproj files with direct dependencies to work around this issue makes little sense and just creates complications as we will need to stay alert to remove these direct dependencies when the issue is fixed at the root OR we need to also maintain this version ourselves, making sure not to accidentally bump it too far, making it incompatible with any other direct dependency (this package may not be likely to cause such a scenario, but in theory for another package, it could happen).

I do realize though that in our case, quite a few packages in the chain need to react and update their dependency before we "automatically" are covered by the fix. The maintainer for all however, is Microsoft, so I don't see it as a big challenge...

Lastly, I agree that some kind of floating version system or "roll forward on patch" would be better for dependency resolution as this would mean that this issue, at least in our case, would not even exist. After the new 8.0.1 release of Asn1, we would just have had to re-run dotnet restore. Roll forward on patch is the default behaviour for SDK resolution, why couldn't it be for NuGet dependency resolution? Is it perhaps due to non-restrictive versioning schemas?

martincostello commented 3 weeks ago

FWIW, if you use NuGet Central Package Version Management and CentralPackageTransitivePinningEnabled=true is specified, then you can update the package in most cases with one line of XML in Directory.Packages.Props (that's what I did, e.g. https://github.com/martincostello/wait-for-nuget-package/commit/d6be89e0b41521526145691130f73a8e260d670c):

<Project>
  <ItemGroup>
    <PackageVersion Include="Foo" Version="1.2.3" />
+   <PackageVersion Include="System.Formats.Asn1" Version="8.0.1" />
    <PackageVersion Include="Bar" Version="4.5.6" />
  </ItemGroup>
</Project>
Matheos96 commented 3 weeks ago

@martincostello Thanks for the tip! Unfortunately I don't see the benefit in the case where you would either way only reference the package directly once (if only needed in a single project). The problem is still that you need to maintain the version yourself and/or remember to remove it once you have confirmed the direct dependency chain no longer contains the vulnerable package.

I do like the idea of the CPM though and I see how it does make the situation more managable in large projects where multiple direct references could be needed (on top of the benefit of managing all dependencies in one place of course 😃 ).

martincostello commented 3 weeks ago

I don't see the benefit in the case where you would either way only reference the package directly once

It's great for dependabot because it only ever touches one file, regardless of how many times a package is used or what project(s).

Matheos96 commented 3 weeks ago

Yes, certainly. But in the case where I, either way, need to manually reference a transitive dependency, in a single .NET project in a solution, it does not relieve me of the "burden" of manually maintaining the reference. If anything, in that case I need to remember I have it defined both in Directory.Packages.Props and in the .csproj itself (if I correctly recall how CPM works - 2 places vs. 1). But nevertheless, this issue's particular annoyance aside, CPM does indeed look intriguing.

EDIT: I believe my 2v1 statement above is probably incorrect when it comes to overriding transitive dependencies with CentralPackageTransitivePinningEnabled=true. In that case it is 1v1 as the override only needs to be defined in the .props file. Sorry!

KirkMunroSagent commented 3 weeks ago

FWIW, if you use NuGet Central Package Version Management and CentralPackageTransitivePinningEnabled=true is specified, then you can update the package in most cases with one line of XML in Directory.Packages.Props (that's what I did, e.g. martincostello/wait-for-nuget-package@d6be89e):

<Project>
  <ItemGroup>
    <PackageVersion Include="Foo" Version="1.2.3" />
+   <PackageVersion Include="System.Formats.Asn1" Version="8.0.1" />
    <PackageVersion Include="Bar" Version="4.5.6" />
  </ItemGroup>
</Project>

The problem with CPM is twofold:

  1. It isn't really fully baked when it comes to using the Nuget Package Manager in Visual Studio, and this is especially true if your solution includes a bunch of projects that are published into multiple nupkgs for different versions of .NET.

  2. If you work on many solutions, you still have to deal with this issue multiple times, at least once per solution that is impacted, and then keep track of that for cleanup later.

Saying that, we use CPM in some of our solutions, and it does reduce the burden a little once you learn the shortcomings of the Nuget Package Manager UI when it comes to CPM, but this issue is still something that shouldn't be pushed downstream so I still question the stance Microsoft has taken on this.

hoerup commented 3 weeks ago

I also ran into this and had quite a few of our images getting flagged by trivy scan, and with the stance microsoft has on this matter i doubt it will be resolved in a timely manner.

I agree with the other participants that CPM might not be ideal - in my case I would need to set it up on 30 projs in as many repositories.

So, i took a quite different approach: I made a small tool that I injected into my pipeline template used across all my repos:

This tool does performs a dotnet list package --include-transitive --format json, parses and iterates the transitive packages, which is compared to a hardcoded list of replacements:

record PackageReplacement(string package, string oldVersion, string newVersion, string reason, string severity); static List replacements = [ new PackageReplacement("System.Formats.Asn1", "6.0.0", "6.0.1", "CVE-2024-38095", "high"), new PackageReplacement("System.Formats.Asn1", "8.0.0", "8.0.1", "CVE-2024-38095", "high"), ];

If a replacement is found i just do a dotnet add package so it's added as a direct dependency - but only during buildtime and nothing is commited back to repo.

So now all our builds are fixed in the trivy scans - but I don't have added asn1 as a permanent dependency that I need to maintain.

martincostello commented 3 weeks ago

in my case I would need to set it up on 30 projs in as many repositories.

This project should be able to automate it for you: https://github.com/Webreaper/CentralisedPackageConverter

Matheos96 commented 2 weeks ago

I also ran into this and had quite a few of our images getting flagged by trivy scan, and with the stance microsoft has on this matter i doubt it will be resolved in a timely manner.

I agree with the other participants that CPM might not be ideal - in my case I would need to set it up on 30 projs in as many repositories.

So, i took a quite different approach: I made a small tool that I injected into my pipeline template used across all my repos:

This tool does performs a dotnet list package --include-transitive --format json, parses and iterates the transitive packages, which is compared to a hardcoded list of replacements:

record PackageReplacement(string package, string oldVersion, string newVersion, string reason, string severity); static List replacements = [ new PackageReplacement("System.Formats.Asn1", "6.0.0", "6.0.1", "CVE-2024-38095", "high"), new PackageReplacement("System.Formats.Asn1", "8.0.0", "8.0.1", "CVE-2024-38095", "high"), ];

If a replacement is found i just do a dotnet add package so it's added as a direct dependency - but only during buildtime and nothing is commited back to repo.

So now all our builds are fixed in the trivy scans - but I don't have added asn1 as a permanent dependency that I need to maintain.

I like the idea behind this. Do you have the tool publicly available? I guess replicating it myself would not be too much work either way, but no work is less than something :) Could potentially be improved with accepting a range of versions? The non-restrictiveness of version strings is a problem though, so maybe a list of old versions instead of a single one?

hoerup commented 2 weeks ago

I like the idea behind this. Do you have the tool publicly available? I guess replicating it myself would not be too much work either way, but no work is less than something :) Could potentially be improved with accepting a range of versions? The non-restrictiveness of version strings is a problem though, so maybe a list of old versions instead of a single one?

it's on our internal git server, but here are the main parts https://gist.github.com/hoerup/92e2f4d1bb1e16fedf354ec981abcf35

Matheos96 commented 2 weeks ago

it's on our internal git server, but here are the main parts https://gist.github.com/hoerup/92e2f4d1bb1e16fedf354ec981abcf35

Cool! I had some free time and interest so I decided to implement my own version before even looking at your gist. I hope you don't mind. My version (probably full of issues) is available here: https://github.com/Matheos96/dependency-overrider

One of the biggest differences is that I make use of a separate config.json file for defining overrides, rather than having it directly in code. But of course, the principle is the same.

michaeldaw commented 1 week ago

This is kind of low effort and doesn't add to the issue, but I just want to express appreciation for this discussion. It helped me get around this problem using the less-than-satisfactory methods suggested by MS.

As is mentioned, this issue is not easy to sort out if you're not an expert at package management, which I am not. I write code... I don't care about Nuget and I feel like I shouldn't have to.