ConstantExpressions do not get parameterized in SQL statements

[Drapondur]

Summary:

I've noticed that ConstantExpressions do not get parameterized when an IQueryable is translated to SQL.
While this won't be problem under most circumstances it can be when manually creating expression trees.

Description:

Consider the following queries (where Client.Identifier is a nvarchar(50) column in a SQL Server 2022 database)

string id = "MPI";
var cl = db.Clients.FirstOrDefault(x => x.Identifier == id);
var cl2 = db.Clients.FirstOrDefault(x => x.Identifier == "MPI");

The first query is translated to

DECLARE @take Int -- Int32
SET     @take = 1
DECLARE @id NVarChar(50) -- String
SET     @id = N'MPI'

SELECT TOP (@take)
	[x].[Id],
	[x].[ParentId],
	[x].[Identifier],
	[x].[Comment],
	[x].[FullIdentifier]
FROM
	[dbo].[Client] [x]
WHERE
	[x].[Identifier] = @id

whereas the second yields in the not parameterized SQL

DECLARE @take Int -- Int32
SET     @take = 1

SELECT TOP (@take)
	[x].[Id],
	[x].[ParentId],
	[x].[Identifier],
	[x].[Comment],
	[x].[FullIdentifier]
FROM
	[dbo].[Client] [x]
WHERE
	[x].[Identifier] = N'MPI'

For usually this won't be a big deal in terms of SQL injection, since under most circumstances ConstantExpressions are created by Linq-Statements like the one above, that is they are written by the developer and not created from user input.

However, I am creating the predicate expression using the System.Linq.Expressions.Expression factory method, like so:

Expression<Func<Client, bool>> where = null;
var paramXp = Expression.Parameter(typeof(Client));
var compare = Expression.Equal(Expression.Property(paramXp, nameof(Client.Identifier)), Expression.Constant(id));
where = Expression.Lambda<Func<Client, bool>>(compare, paramXp);
var cl3 = db.Clients.FirstOrDefault(where);

This again yields in an unparameterized SQL statement, which then is vulnerable to SQL injection (or would break SQL Server Always Encrypted),

Currently this is no problem for me personally as I am able to create a MemberExpression which leads to a parameterized SQL statement.

var dummy = new { Value = id };
compare = Expression.Equal(
				Expression.Property(paramXp, nameof(Client.Identifier)),
				Expression.Property(Expression.Constant(dummy), nameof(dummy.Value)));
where = Expression.Lambda<Func<Client, bool>>(compare, paramXp);
var cl4 = db.Clients.FirstOrDefault(where);

That notwithstanding I'd like to propose to always parameterize ConstantExpressions in case anyone creates expression trees like I did first and thereby accidentally gets insecure SQL statements.

Environment details

Linq To DB version: 5.2.2

Database (with version): SQL Server 2022

ADO.NET Provider System.Data.SqlClient 4.8.6

Operating system: Win 10

.NET Version: .net 6

[jods4]

I don't think this should be done.

This is a very impacting change that has potentially dramatic impact on execution plans, depending on exact queries, similar queries executed beforehand and RDBMS.
In some instances users are gonna ask that constants should be inlined rather than parameterized.

The described benefit is: "I can create insecure SQL if I manually build an expression where a 'constant' is user-injected".
I don't find that argument compelling because:

1. This is not string concatenation and shouldn't be subject to injection attacks. You should provide a concrete repro if you actually found one. Strings constants are escaped before being injected in query.
2. You probably shouldn't be doing that, and libraries can't always protect you from yourself. Your example is not subject to injection attacks (see 1) but linq2db has several other advanced features that let you manipulate the SQL text directly. With great power... you know the drill. Other apis let you force inlining of local variables (instead of parameterization), for reasons linked to execution plans mentioned above. They are much simpler ways to inject user-content in your query (should still be safe though, just saying the argument to change ConstantExpression handling is not compelling).

[joonatanu-softwerk]

I have observed somewhat similar issue with Skip() and Take() methods.

query.Skip(10).Take(10) will inline the values.
query.Skip(() => 10).Take(() => 10) will parameterize the values.

Is that expected behaviour?