Ayende @ Rahien

Refunds available at head office

Chasing the SQL Injection that never was

So, I am sitting there quietly trying to get EF Prof to work in a way that I actually like, when all of a sudden I realize that I am missing something very important, I can’t see the generated queries parameters in the profiler.

Looking closely, I started investigating what appear to be a possible SQL injection issue with EF. My issue was that this query:

entities.Posts.Where(x=>x.Title == “hello”)

Generated the following SQL:

SELECT
1 AS [C1],
[Extent1].[Id] AS [Id],
[Extent1].[Title] AS [Title],
[Extent1].[Text] AS [Text],
[Extent1].[PostedAt] AS [PostedAt],
[Extent1].[BlogId] AS [BlogId],
[Extent1].[UserId] AS [UserId]
FROM [dbo].[Posts] AS [Extent1]
WHERE N'hello' = [Extent1].[Title]

It literally drove me crazy. Eventually I tried this query:

var hello = "hello";
entities.Posts.Where(x=>x.Title==hello);

Which generated:

SELECT
1 AS [C1],
[Extent1].[Id] AS [Id],
[Extent1].[Title] AS [Title],
[Extent1].[Text] AS [Text],
[Extent1].[PostedAt] AS [PostedAt],
[Extent1].[BlogId] AS [BlogId],
[Extent1].[UserId] AS [UserId]
FROM [dbo].[Posts] AS [Extent1]
WHERE [Extent1].[Title] = @p__linq__1

This was more like it.

It seems (and Julie Lerman confirmed it) that EF is sticking constant expressions directly into the SQL, and treating parameters differently.

I am not quite sure why, but from security standpoint, it is obviously not a problem if it does so for constants. It have a lot less hair now, though.

Comments

Dmitry
11/11/2009 01:56 PM by
Dmitry

Yes, that is normal behavior for EF.

Remco Ros
11/11/2009 06:37 PM by
Remco Ros

So.. would there be a special reason for this design?

Rob Conery
11/11/2009 07:11 PM by
Rob Conery

Yep - Linq to Sql does the same thing. If it sees a ConstantExpression it just puts the value straight in with the thinking that it was set programmatically. So with our NHibernate stuff we did the other day where we set the CategoryID==33, it would put 33 in the query.

A bit weird when you consider the mandate to scrub everything, but at the same time I think it's generally OK.

Ayende Rahien
11/11/2009 07:31 PM by
Ayende Rahien

Rob,

NHibernate does not do it that way, though.

It makes everything a parameter

Frank
11/11/2009 11:23 PM by
Frank

If the constant parameter contains a single quote, I guess it will nicely escape it in the SQL query itself.

If something is constant, then it is better to actually use it as a constant in the SQL query itself, instead of a parameter. When a constant is used, the statistics of the column will be included in deciding on which path to take. In case of a parameter, only the index selectivity will be used in deciding on the query plan.

Dmitry
11/12/2009 02:44 AM by
Dmitry

I have done a lot of logging for Linq-to-SQL and it does not do this. In fact, Linq-to-Entities is the only ORM Linq provider I have seen that does that.

I think it is a bad decision because you will get different execution plans for different length constants.

P
11/12/2009 03:50 PM by
P

So everything rob said was wrong?...

Alex James
11/12/2009 06:03 PM by
Alex James

I learned of this distinction for the first time while showing some Generated SQL to some SQL Server MVPs at the MVP summit in 2007!

A mild panic attack followed. But all is well that ends well.

I personally think both inlining or using a parameter for constants is fine.

Alex

Ayende Rahien
11/12/2009 06:06 PM by
Ayende Rahien

Alex,

Yes, it isn't a major issue, although I think that I can get it to generate a SQL injection if I was using MySQL.

It is not much of an injection, though, if it has to be compiled to get there.

Do you have any idea what was the reasons for this decision?

Comments have been closed on this topic.