CODEWITHSUNDEEP
c#entity-framework-coreormexpressionsql-injection
Myliak
Myliak

Reputation: 59

Entity Framework Core: how safe are expressions from SQL injection?

I am doing research on ways to prevent SQL injection when using Entity Framework Core ORM. Most blogs and sources cite official Microsoft documentation, that the best way is to sanitize values, use interpolated strings and parse/check user provided values before using them in queries.

But I've never found answer for my question about how safe is to use expression tree construction. And also, that's the reason for this post, because I've also not found any problem, but I might be speaking from my inexperience in this matter.

For example, this is an unsafe way of creating a SQL query:

public List<Blogs> GetFilteredBlogs(string filterField, string filterValue) 
{
    List<Blogs> blogs = context.Blogs
                               .FromSqlRaw($"select * from Blogs where {filterField} = {filterValue}");
    return blogs;
}

And this would be query equivalent using expression construction:

public List<Blogs> GetFilteredBlogs(string filterField, string filterValue) 
{
    ParameterExpression entity = Expression.Parameter(typeof(Blog));
    MemberExpression entityProperty = Expression.Property(entity, filterField);
    ConstantExpression valueConstant = Expression.Constant(filterValue);
    BinaryExpression equal = Expression.Equal(entityProperty, valueConstant);
    LambdaExpression filter = Expression.Lambda<Func<T, bool>>(equal, entity);
    
    List<Blogs> blogs = context.Blogs.Where(filter);

    return blogs;
}

This is a very simple example, but should work well with my question. Why is this not more widely used against SQL injections when dynamic queries are needed? I can see this can be abused when someone would want to transform full SQL query into expression tree, but when used with simple values and specific query construction, I think might be even better than to sanitize or parse check user provided inputs.

I am aware that in this example, user could access any column from Blogs table, but let's say it doesn't matter or there would be protection for which columns are accessible.

Upvotes: 0

Views: 80

Answers (2)

Jeremy Lakeman
Jeremy Lakeman

Reputation: 11163

To prevent SQL injection, bind all of your query parameters. That's all, no further thought is required to cleaning or validating inputs.

Using .FromSqlInterpolated will take each of the interpolated string parameters and turn them into bound query parameters. So again, no further thought is needed.

If you call context.Blogs.Where(filter); with any filter. EF will translate that filter into valid sql. And again, EF will bind parameters for any user supplied variables. If your expression does include constants, they will be translated into sql constants. EF will take care of escaping any weird values in any strings. This isn't vulnerable to sql injection as it isn't just appending raw strings together.

Constructing an "Expression Tree" manually, while also ensuring that any user supplied variables are treated as variables, is a more complicated topic. The motivation for doing this properly isn't to prevent sql injection, it's for performance. Both EF and your database can then cache and reuse a single query plan for a single sql statement. Instead of needing to rebuild this metadata for every single query with different "constant" values.

While it might seem like more work, I would recommend building queries by gluing small expression tree templates together. If you do need something more dynamic, then build an ExpressionVisitor to edit these templates into what you want. Though this is a complex topic, outside the scope of this stack overflow answer.

If you start with a simple expression like the following;

string parameter;
Expression<Func<string,bool>> equality = s => parameter == s;

The C# compiler will do the heaving lifting. Handling the captured parameter, in a way that is supported by EF to translate this into a bound sql parameter.

Upvotes: 0

Marc Gravell
Marc Gravell

Reputation: 1063844

Why is this not more widely used against SQL injections when dynamic queries are needed?

It is sometimes used, but generally: people don't often need a dynamic query where the test field is one of the clauses. Additionally, there's a wide range of type conversion considerations (if the field isn't string), and verification that the input fields are valid. Another approach that avoids these problems might be, for example:

Expression<Func<Blogs, true>> predicate;
switch (filterField)
{
    case "Name":
        predicate = blog => blog.Name == filterValue;
        break;
    case "Id":
        int id = int.Parse(filterValue, {culture etc});
        predicate = blog => blog.Id == id;
    // .. etc
    default:
        throw new ArgumentOutOfRangeException(nameof(filterField));
}
return context.Blogs.Where(predicate);

This also avoids reflection-heavy manual code, which is often a useful aim (and I say that speaking as a reflection affectionado).

Upvotes: 2

Related Questions