Vulnerabilities

[nprhl]

Serilog.Sinks.MSSqlServer

Library Vulnerabilities (1)

Severity: Medium Top Fix: Upgrade to version microsoft.data.sqlclient 5.2.1

[cremor]

Which vulnerability do you mean exactly? SqlClient 5.1.5 is not marked as vulnerable on NuGet. (I know that a dependency of SqlClient is marked as vulnerable, but this is also true for the currently latest version 5.2.1.)

[MaddMugsy]

Piggybacking on this issue to report two new vulnerabilities reported by VS today:

Azure.Identity 1.1.3 and Microsoft.Identity.Client 4.60.3 are showing up as vulnerable dependencies of Microsoft.Data.SqlClient 5.2.1, which is a dependency of version 6.6.1 of this package.

[ckadluba]

Thank you for reporting this. I made a scan today using dotnet list package --vulnerable --include-transitive and found vulnerability reported by @MaddMugsy and other vulnerabilies. One even with high severity.

Project `Serilog.Sinks.MSSqlServer` has the following vulnerable packages
   [net462]:
   Transitive Package               Resolved   Severity   Advisory URL
   > Azure.Identity                 1.10.3     Moderate   https://github.com/advisories/GHSA-wvxc-855f-jvrv
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9
   > Microsoft.Identity.Client      4.56.0     Low        https://github.com/advisories/GHSA-x674-v45j-fwxw
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9

   [net472]:
   Transitive Package               Resolved   Severity   Advisory URL
   > Azure.Identity                 1.10.3     Moderate   https://github.com/advisories/GHSA-wvxc-855f-jvrv
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9
   > Microsoft.Identity.Client      4.56.0     Low        https://github.com/advisories/GHSA-x674-v45j-fwxw
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9

   [netstandard2.0]:
   Transitive Package               Resolved   Severity   Advisory URL
   > Azure.Identity                 1.10.3     Moderate   https://github.com/advisories/GHSA-wvxc-855f-jvrv
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9
   > Microsoft.Identity.Client      4.56.0     Low        https://github.com/advisories/GHSA-x674-v45j-fwxw
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9

   [net6.0]:
   Transitive Package               Resolved   Severity   Advisory URL
   > Azure.Identity                 1.10.3     Moderate   https://github.com/advisories/GHSA-wvxc-855f-jvrv
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9
   > Microsoft.Identity.Client      4.56.0     Low        https://github.com/advisories/GHSA-x674-v45j-fwxw
                                               Moderate   https://github.com/advisories/GHSA-m5vv-6r4h-3vj9
   > System.Formats.Asn1            5.0.0      High       https://github.com/advisories/GHSA-447r-wph3-92pm

After updating Microsoft.Data.SqlClient from 5.1.5 to the latest version 5.2.1, it came down to only the one vulnerability which was reported by @MaddMugsy: https://github.com/advisories/GHSA-m5vv-6r4h-3vj9.

It seems that the SqlClient have already merged a PR which fixes the remaining vulnerability (https://github.com/dotnet/SqlClient/pull/2577) on 23rd of June but they have not yet created a new 5.x release. So we will have to wait for that.

In the meantime, to increase the security posture of SQLServer sink, I will create a release with the SqlClient udated to 5.2.1 so we get rid of the high severity and other vulnerability and only https://github.com/advisories/GHSA-m5vv-6r4h-3vj9 is remaining. This we will fix as soon as SqlClient makes a new release.

[cremor]

only GHSA-m5vv-6r4h-3vj9 is remaining. This we will fix as soon as SqlClient makes a new release.

@ckadluba FYI, this will also be fixed in SqlClient version 5.1.6, see https://github.com/dotnet/SqlClient/pull/2649

SqlClient 5.1.x has LTS support, while 5.2x only has "current" support. See https://learn.microsoft.com/en-us/sql/connect/ado-net/sqlclient-driver-support-lifecycle I don't know if that affects this package or how you want to handle this. But I thought that I should mention it since it is the reason that Microsoft.EntityFrameworkCore.SqlServer v8 stays on SqlClient 5.1.x.

[ckadluba]

Thanks for mentioning this. Then probably we should switch back to a 5.1 version as soon as there is a fix.

[cremor]

@ckadluba FYI: https://github.com/dotnet/SqlClient/issues/2568#issuecomment-2313733614

[ckadluba]

@cremor Thank you for the heads up. I'll create a sink version 6.7.1 with SqlClient 5.1.6 soon. This should also fix #552.

[ckadluba]

Added NuGet audit and fixed different vulnerabilities by updating any affected dependencies. We created a dev release with the changed.

https://www.nuget.org/packages/Serilog.Sinks.MSSqlServer/6.7.1-dev-00084

Please try it out and give us some feedback. Thank you! :)

[cremor]

I'm not so sure that directly referencing indirect dependencies is a good idea, even if the indirect dependency has a security vulnerability. This leads to a situation where it looks like this package has more direct dependencies than it really has. And to more maintenance work.

Concrete example: You added System.Private.Uri v4.3.2 as a direct dependency. I don't know why, because now my solution additionally requires this package. The only reference to it is from Serilog.Sinks.MSSqlServer:

> dotnet nuget why . System.Private.Uri
Project 'WebApi' has the following dependency graph(s) for 'System.Private.Uri':

  [net8.0]
   │
   └─ Serilog.Sinks.MSSqlServer (v6.7.1-dev-00084)
      └─ System.Private.Uri (v4.3.2)

Hypothetical example for future maintenance work: Microsoft.Data.SqlClient is (finally) starting the work on removing the Azure.Identity dependency from their package, see https://github.com/dotnet/SqlClient/issues/1108 Assume you'd also had added Azure.Identity as a direct dependeny to Serilog.Sinks.MSSqlServer because of a security vulnerability. When SqlClient then releases a new version that doesn't have the Azure dependency any more, Serilog.Sinks.MSSqlServer would still add it as a dependency, even though it isn't needed.

Ideally, the direct dependency should update their dependency and then just the direct dependency should be updated here. But of course that requries work from other teams.

End users can alway update indirect dependencies themselves, e.g with transitive pinning in central package management.

[ckadluba]

@cremor I see your point and agree to some degree. Pulling a transient dependency into the sink project to fix a vulnerability isn't quite elegant. Especially since it could easily be forgotten to be removed again once the package where it is referenced originally gets updated and we no longer need the reference. But on the other hand, fixing a high severity vulnerability fast is seems important than having less maintenance work in the future or unused dependencies. If we leave the vulnerability open, people will start creating issues and complain about the vulnerability (which I can understand because they usually do not care if it is caused by the a dependency of the sink or a transitive dependency of it).

By the way, the example with Azure.Identity in SqlClient is a very interesting pick when it comes to the MSSQL sink because the sink can also be used for logging to Azure SQL. This means, if they restructure SqlClient and remove Azure.Identity from the their core package, we still need to add the SqlClient.Azure (or whatever it will be called) package because otherwise things like managed identity authentication will not work anymore with the sink.

[cremor]

But on the other hand, fixing a high severity vulnerability fast is seems important than having less maintenance work in the future or unused dependencies.

Ok, that's fine for me. I just wanted to mention it.

This means, if they restructure SqlClient and remove Azure.Identity from the their core package, we still need to add the SqlClient.Azure (or whatever it will be called) package because otherwise things like managed identity authentication will not work anymore with the sink.

Do you actually need classes from Azure.Identity at compile time? If not, I think you should not reference it in the future. I think you should then take the same approach that Microsoft.Data.SqlClient seems to follow now, which is to throw at runtime if Microsoft.Data.SqlClient.Azure.Authentication (or however it will be called) is not available. Otherwise you'll do the same that Microsoft.Data.SqlClient does now, which is forcing a Azure dependency (with possible security vulnerabilities) to all of your users, even if they don't use Azure.

Finally, could you please explain why you now need to reference System.Private.Uri? This dependency wasn't installed in my project with Serilog.Sinks.MSSqlServer 6.6.1 or 6.7.0, so it is new in 6.7.1. How could there have been a security vulnerability for it, if it wasn't even used before? Maybe it is only referenced in some target frameworks, but not in .NET 8?

[ckadluba]

Finally, could you please explain why you now need to reference System.Private.Uri? This dependency wasn't installed in my project with Serilog.Sinks.MSSqlServer 6.6.1 or 6.7.0, so it is new in 6.7.1. How could there have been a security vulnerability for it, if it wasn't even used before? Maybe it is only referenced in some target frameworks, but not in .NET 8?

This was the warning I got mentioning System.Private.Uri.

In the NuGet UI it did not appear as a transitive, so I suppose your assumption is correct and it is referenced by the frameworks.

Possibly the advisories can help us to locate the source of that dependency. https://github.com/advisories/GHSA-5f2m-466j-3848 https://github.com/advisories/GHSA-xhfc-gr8f-ffwc

[ckadluba]

Yes, it seems to be only an SDK issue. https://github.com/dotnet/announcements/issues/113. A quite old one even.

I'll check and update it on my machine. It that removes the warning, we can remove the System.Private.Uri dependency again in the next release.

[ckadluba]

I get the same behavior on a different machine with latest VS 2022 Preview (17.12.0 Preview 1) and .NET SDK 9.0.100-preview.7.24407.12. When I remove the reference to System.Private.Uri from the sink project, the warnings about the two vulnerabilities occur.

I will leave the reference in and we will watch how this behaves with the next SDK updates.

[cremor]

Yeah, this looks like a false positive. Here is a similar issue where it's mentioned that this will not be fixed in .NET 9 😞 https://github.com/unoplatform/uno/issues/18019

It looks like all reports of this issue are closed as duplicates of https://github.com/NuGet/Home/issues/7344

[ckadluba]

Thanks for the research. If this affects lots of users giving a false postive about a high severity vulnerability, I hope there will be fix anytime soon.