Dependency on System.Drawing.Common that has a vulnerability

[trampster]

StackExchange.Redis 2.6.70

• System.Diagnostics.PerformanceCounter >= 5.0.0
  • System.Configuration.ConfigurationManager >= 5.0.0
    • System.Security.Permissions >= 5.0.0
      • System.Windows.Extensions >= 5.0.0
        • System.Drawing.Common >= 5.0.0

Because nuget uses the lowest version that meets the requirements, by default you end up with a System.Drawing.Common 5.0.0 which as the following security vulnerability: GHSA-rxg9-xrhp-64gj

This can be worked around by explicitly referencing the version with the fix:

<PackageReference Inculde="System.Drawing.Common" Version="5.0.3" />

[trampster]

It should be noted that System.Drawing.Common is no longer cross platform (windows only as of .net 6.0) so you may want to drop this dependency all together going forward.

I tracked down the usage of PerformanceCounter and it's only used in PerfCounterHelper.cs TryGetSystemCPU method which already has a [SupportedOSPlatform("Windows")] attribute.

So it's not used on linux (we are on linux) but that doesn't stop it from being referenced, ending out in our output folder and making

dotnet list package --vulnerable --include-transitive

report an issue.

[trampster]

Interestingly PerfCounterHelper is an internal class, So TryGetSystemCPU is not publicly callable and is also not called internally.

Unless there is some good reason to have it (do you use it for debugging, by adding calls to it that you don't commit?) Then I would suggest deleting PerfCounterHelper and getting rid of your dependency on System.Diagnostic.PerformanceCounter (and therefore System.Drawing.Common) entirely.

[NickCraver]

It's for reporting CPU in exception messages which lets us know the user is under extreme load or not. In this case though we're not using a sensitive path in the library or are subject to the CVE, so I don't see us changing dependencies especially 4 steps away here. If users want to alert on any other path to exploitable code and use a newer version they are welcome to do so, but us forcing an upgrade comes with a lot of friction so it's not something we do with every downstream issue especially if we're sure not to be a factor in it.

[NickCraver]

I've raised this to dotnet/runtime#64592 because I admit the dependency chain is...weird, for sure. It wouldn't change our current behavior (as there's no other decent way to get this counter we're aware of - see #1924). If there is a viable alternative, definitely all ears.

[trampster]

Thanks Nick,

We have already added an explicate reference to the patched version:

<!-- We don't use this directly, but without this StackExchange.Redis pulls in a vulnerable version (5.0.0) -->
<PackageReference Include="System.Drawing.Common" Version="5.0.3" />

We are on linux so that performance counter doesn't benefit us. We will look at striping our output because we don't need all these extra windows only dependencies in our deployment.

[NickCraver]

What are you targeting, specifically? net6.0?

I'm asking because we don't have a net6.0 TFM because there hasn't been a benefit thus far, but if for example we had a net6.0 (without the dependency) and a net6.0-windows (with the dependency)...or something, it might make sense to add.

[trampster]

Yes we are targeting net6.0

[AraHaan]

Technically S.S.P can be eliminated entirely from S.C.CM because it uses CAS (Code access security or w/e) which do not work in .NET Core (aka 6.0+) anyway.

Which makes it one less of a problem as well.

[NickCraver]

I'm looking through this and how many error reports actually have permissions to render the counter. It was really nice to have and has been a saver in the past, but going back over the past year I see almost all exceptions couldn't access the counter anyway, so the reason for having this dependency in the first place I agree is no longer a net win.

Working on dropping this off for the next release now. Thanks for poking and discussion here!

[NickCraver]

@trampster heads up: #2285 :)

[NickCraver]

2.6.80+ will drop this dependency, available on MyGet now if you want to grab it early: https://www.myget.org/feed/stackoverflow/package/nuget/StackExchange.Redis/2.6.80

[mataness]

@NickCraver when 2.6.80 will be released to the public nuget feed ? we need that fix too.
Thanks.

[NickCraver]

It's actually on NuGet right now hidden for other testing but I don't know if we'll publish it - getting a few more changes in for a release with release notes so what you'll see is a higher version. If you're okay using a release without release notes in-between (e.g. expect a 2.6.9x or something soon with all that), you can pull it immediately.

[guylevy]

great change, thanks! would great if you can please list 2.6.80 version on nuget.org even without release notes. makes it easier to find and communicate to our teams.

[NickCraver]

@guylevy I...thought I had :) Listed now!