2

I have an extension for Entity Framework Core query to support between and I see that the SQL being parsed includes literal values. Is it vulnerable for SQL injection attacks?

This is the extension method:

public static IQueryable<T> WhereBetween<T, TValue>(
    this IQueryable<T> query,
    TValue? min,
    TValue? max,
    Expression<Func<T, TValue>> selector)
    where TValue : struct, IComparable<TValue>
{
    if (min.HasValue)
    {
        query = query.Where(Expression.Lambda<Func<T, bool>>(
            Expression.GreaterThanOrEqual(
                selector.Body,
                Expression.Constant(min.Value)
            ),
            selector.Parameters
        ));
    }

    if (max.HasValue)
    {
        query = query.Where(Expression.Lambda<Func<T, bool>>(
            Expression.LessThanOrEqual(
                selector.Body,
                Expression.Constant(max.Value)
            ),
            selector.Parameters
        ));
    }

    return query;
}

I use it as a filter with minimum value and maximum value, and this returns as >= and <=. I use it for multiple data types like dates, ints etc.

Example method:

public List<Order> GetOrders(int minValue, int maxValue, DateOnly startDate, DateOnly endDate)
{
    return dbContext.Orders
        .WhereBetween(minValue, maxValue, o => o.OrderTotal)
        .WhereBetween(startDate, endDate, o => o.OrderDate)
        .ToList();
}

This uses the same extension for DateOnly and int. The SQL that it's being parsed to is

select ...
from Orders o
where o.OrderTotal >= 10 
  and o.OrderTotal <= 20 
  and o.OrderDate >= '2025-01-01' 
  and o.OrderDate <= '2025-02-02'

It's being parsed as literal values not parameterized. Do I need to worry for SQL injection attacks? Or is it just displayed in the logs as literal values but it is being sent as escaped literal values?

6
  • 1
    Please edit your question to include an SQL injection attempt by using string values and TEXT/VARCHAR columns, even though the compare operators wouldn't make any sense. Add the generated SQL query to the question. Commented Sep 3 at 17:04
  • 2
    and when you do Where(o.OrderTotal>=min && ..) manually it's different? Commented Sep 3 at 17:25
  • 1
    @IvanPetrov yes it's being parsed as where o.OrderTotal >= @order_min or something like that. Commented Sep 3 at 17:27
  • 1
    @Progman My question is not necessarily for this extension as this extension won't work for string values. But I want to add other similar extensions in the future and I'm worried that as EF is not parameterizing the values it might be vulnerable to SQL injection attacks. Commented Sep 3 at 17:54
  • @Shmiel There is no problem in having a query parsed as where o.OrderTotal >= @order_min. Keep in mind that prepared statements do not add the values "back" into the query. Commented Sep 3 at 19:00

2 Answers 2

3

You are probably safe, but if you want to be explicitly safe, EF Core 9 added the EF.Parameter method in response to the feature request: Add EF.Parameter to force parameterization in query

In it you can see a comment almost exactly about your use-case:

We have a bunch of query building methods that dynamically craft an expression tree. We were ending up with constant values plugged directly into the query because we were using Expression.Constant. As a workaround I use the following helper method to force a closure over the value(s) passed into our builders:

public static Expression CloseOver<T>(T constant)
   => ((Expression<Func<T>>)(() => constant)).Body;

...

Looks like a working workaround if you cannot upgrade to EF Core 9.

For EF Core 9, you'd have to build a Expression.Call pointing to the EF.Parameter method in place of your Expression.Constant.

So your original:

query = query.Where(Expression.Lambda<Func<T, bool>>(
    Expression.GreaterThanOrEqual(
        selector.Body,
        Expression.Constant(min.Value)
    ),
    selector.Parameters
));

needs to be rewritten into something like this:

// TODO: probably cache methodinfo for TValue
var efParameterMethod = typeof(EF)
    .GetMethod(nameof(EF.Parameter))
    .MakeGenericMethod(typeof(TValue));

query = query.Where(Expression.Lambda<Func<T, bool>>(
    Expression.GreaterThanOrEqual(
        selector.Body,
        Expression.Call(
            efParameterMethod,
            Expression.Constant(max.Value)
        )
    ),
    selector.Parameters
));
Sign up to request clarification or add additional context in comments.

4 Comments

More precisely, it's definitely safe, just not very efficient in terms of plan cache reuse, which is why EF.Parameter exists
@Charlieface if it's definitely safe, then the question is probably a duplicate :)
While it's possible to build something like CloseOver manually; passing a generic structure as a constant, then loading the field as an expression.... It's much simpler to let C# build the expression, then pull it apart again. Plus C# will use IL shorthand for some steps, which you can't write in C# at all.
Thanks. Will see if I can upgrade to EF Core 9 and use EF.Parameter.
1

Yes, your query is still injection safe.

Even though your parameters are being evaluated as constants by the query generation pipeline, the query generation pipeline still type casts and uses escape characters in string literals to prevent injection.

For some data types you can force the query generation pipeline to treat your values as parameters with this change:

if (min.HasValue)
    {
        var minValue = min.Value;
        query = query.Where(Expression.Lambda<Func<T, bool>>(
            Expression.GreaterThanOrEqual(
                selector.Body,
                Expression.Property(Expression.Constant(minValue), nameof(minValue))
            ),
            selector.Parameters
        ));
    }

    if (max.HasValue)
    {
        var maxValue = max.Value;
        query = query.Where(Expression.Lambda<Func<T, bool>>(
            Expression.LessThanOrEqual(
                selector.Body,
                Expression.Property(Expression.Constant(maxValue), nameof(maxValue))
            ),
            selector.Parameters
        ));
    }

As your comment uncovered...

Sadly none of the tricks for reboxing your parameter to force parameterized queries will work in your generic method, because several datatypes have nuances you have to address individually to avoid runtime errors.

But you can still run the experiment on your integer fields and see that it produces the parameterized query.

Also, the opening statement of the answer holds true. Your query is injection safe, even though it is producing the query with constant values instead of parameters.

2 Comments

I tried your suggestion and I get this error for DateOnly Instance property 'Value' is not defined for type 'System.DateOnly' (Parameter 'propertyName')
never mind. this won't work for all data types. in fact none of the tricks for reboxing your parameter to force parameterized queries will work in your generic method, because several datatypes have nuances you have to address individually. But you can still run the experiment on your integer fields and see that it produces the parameterized query. I've updated the answer to include these comments.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.