Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some considerations regarding security #615

Closed
ademchenko opened this issue Jul 28, 2022 · 5 comments
Closed

Some considerations regarding security #615

ademchenko opened this issue Jul 28, 2022 · 5 comments
Assignees
Labels

Comments

@ademchenko
Copy link

ademchenko commented Jul 28, 2022

We are going to provide the dynamic query language to our end-users. That causes us to double-check potential pitfalls in security.
Since the string presenting the query translated into the code which can be executed inside the context of our system we have to be sure the user is very restricted in their usage of types, operators, etc., etc.

After the research, we have found some issues I would like to discuss.

  1. Enums are used without any restriction. It might seem to be safe, but I wouldn't like my end-user could potentially analyze what enums with what values are used in our code which is irrelevant to his query. Consider the example:
namespace IrrelevantNamespace
{
    public enum SecurityLevels
    {
        Weak = 1,
        Strong = 10
    } 
}  
  
[Test]
        public void TestIrrelevantEnums()
        {
            var entities = new Entity[]
            {
                new("KnownName1", 1),
                new("KnownName2", 10),
            }.AsQueryable();

           var query = entities.Where("ExperienceInYears = IrrelevantNamespace.SecurityLevels.Strong").Select(e => e.Name);
            var result = query.ToList();

            TestContext.WriteLine(string.Join(",", result));
        }
  1. Parameterless constructors of any entity (at least of the same namespace) can be called. It might be useful but, obviously, constructor calls could have side effects so there should be an ability to turn that feature off. Even in regards to the 'it' type the possibility to use its properties doesn't cause the safety of calling its constructor in all circumstances. Consider the example:
namespace MyNamespace
{
    public class IrrelevantDataClass
    {
        public IrrelevantDataClass()
        {
            TestContext.WriteLine($"{typeof(IrrelevantDataClass)} unexpected ctor is called");
        }
    }

    public class Entity
    {
        public string Name { get; }

        public int ExperienceInYears { get; }

        public Entity()
        {
            TestContext.WriteLine($"{typeof(Entity)} unexpected ctor is called");
        }

        public Entity(string name, int experienceInYears)
        {
            Name = name;
            ExperienceInYears = experienceInYears;
        }
    }

    public class DynamicLinqTests
    {        
        [Test]
        public void TestCtor()
        {
            var entities = new Entity[]
            {
                new("KnownName1", 1),
                new("KnownName2", 10),
            }.AsQueryable();

            var query = entities.Where(new ParsingConfig { AllowNewToEvaluateAnyType = false, ResolveTypesBySimpleName = true },
                "it eq object(new IrrelevantDataClass())").Select(e => e.Name);

            var result = query.ToList();

            //Even Entity ctor call might be not good in some circumstances
            query = entities.Where(new ParsingConfig { AllowNewToEvaluateAnyType = false }, "it eq new Entity()").Select(e => e.Name);
            result = query.ToList();
        }
}
  1. And the last question is not about the security but about addressing. Why, in the following case, cannot I use a full type name (i.e. with namespaces)? It is important when several types with the same name but from different namespaces are added as custom.
namespace IrrelevantNamespace
{
    public class SameNameClass
    {
        public static int UserExperience() => 10;
    }
}

 [Test]
        public void TestAddressing()
        {
            var entities = new Entity[]
            {
                new("KnownName1", 1),
                new("KnownName2", 10),
            }.AsQueryable();

            var query = entities.Where(new ParsingConfig
            {
                ResolveTypesBySimpleName = false,
                CustomTypeProvider = new TestCustomTypesProvider()
            }, "ExperienceInYears == IrrelevantNamespace.SameNameClass.UserExperience()").Select(e => e.Name);

            var result = query.ToList();

            TestContext.WriteLine(string.Join(",", result));
        }

        public class TestCustomTypesProvider : DefaultDynamicLinqCustomTypeProvider
        {
            public override HashSet<Type> GetCustomTypes()
            {
                var customTypes = base.GetCustomTypes();
                customTypes.Add(typeof(IrrelevantNamespace.SameNameClass));
                return customTypes;
            }
        }

causes the exception:
System.Linq.Dynamic.Core.Exceptions.ParseException : Enum value 'UserExperience' is not defined in enum type 'IrrelevantNamespace.SameNameClass'

@ademchenko ademchenko changed the title Some consideration regarding security Some considerations regarding security Jul 28, 2022
@StefH StefH self-assigned this Aug 8, 2023
@StefH
Copy link
Collaborator

StefH commented Aug 8, 2023

1]
If you expose it to end-users, all linq queries on all your database entities are possible, so indeed also using enums if you use enums.

Can you add a simpler example where you write the code and explain in detail what could go wrong?

2]
Full contructor support is only made possible when using linq-to-objects.

Adding a configuration setting to disallow all constructors could be added, I'll investigate this.

3]
Thanks for the question, indeed it seems that full namespace enums cannot be handled, I'll take a look.

@StefH
Copy link
Collaborator

StefH commented Aug 9, 2023

Issue 2: see #732

@StefH
Copy link
Collaborator

StefH commented Dec 5, 2023

@ademchenko
Is the answer enough for you? Or do you need more clarification?

@StefH
Copy link
Collaborator

StefH commented Feb 10, 2024

@ademchenko
Is the answer enough for you? Or do you need more clarification?

@StefH StefH added the question label Apr 26, 2024
@StefH
Copy link
Collaborator

StefH commented Apr 26, 2024

Closing

@StefH StefH closed this as completed Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants