FormattableString overload on ExecuteSqlCommand invites subtle SQL injection vulnerability

[breyed]

This code is safe:

db.ExecuteSqlCommand($"delete from Log where Time<{time}");

But this is not:

db.ExecuteSqlCommand(useLogA ?
    $"delete from LogA where Time<{time}" :
    $"delete from LogB where Time<{time}");

Likewise, this is not:

db.ExecuteSqlCommand(
    $"delete from Log " +
    $"where Time<{time}");

In the first unsafe case, when two FormattableString operands are used with the conditional operator, the expression evaluates to a string, not another FormattableString. Unless you know this subtle (and I find rather unintuitive) rule, you might easily write code expecting the argument to be passed as a SQL parameter, only to find you've really dumped unsanitized user content into a SQL command.

In the second unsafe case, you're using multiple lines for readability. If you're a big C# fan, you might think C# is awesome enough to use string literal concatenation with interpolated strings. You'll be extra disappointed when your evil hacker trim, who hates C#, thinks otherwise, and proves he's right by bringing down your site.

In both unsafe cases the implicit conversion from string to RawSqlString causes the unintended overload to be invoked.

Proposed resolution:

1. Long term: Improve C# string interpolation.
2. Short term: Mark the FormattableString overload obsolete so that a warning alerts devs to the potential danger. Create a replacement that simply has a different name from the RawSqlString overload, perhaps ExecuteFormattedSqlCommand.

Both of your issues relate to how Roslyn implemented interpolation. Yes it causes a lot of headaches, and is non-intuitive, but they made the compiler prefer string over FormattableString. So any compile chain that could produce a string instead of a FormattableString will.

For your first issue, I think if you explicitly cast one or both (not sure off the top of my head which it must be) it should work.

for your second, you cannot concatenate Formattables out of the box and there is no built in formattable builder.

Side note:
The RawSQLString is a workaround for the issue of the compiler choosing string first as it then sees the string as more expensive. It is an ingenious workaround to an unintuitive problem.

[breyed]

I don't question that there are reasonable ways to get EF Core to do what you want. Indeed, when I encountered this issue, once I discovered the parameters in my query weren't getting sent as SQL parameters, it was easy to fix.

My concern is discovery. In my experience that led to uncovering this issue, I was alerted to the problem when I was passing a string value that resulted in invalid SQL query since the string wasn't quoted. If a surprise absence of FormattableString always causes tests to fail, there's no problem. However, if there are scenarios where the SQL command would still work even without SQL parameters, that's when security holes can sneak in.

I think the key question - which I don't know the answer to - is whether there are types that are (1) open to SQL injection and (2) work with and without SQL parameterization. For example, strings are open to SQL injection, but would a string intended to be a parameter always fail if it ended up inline, due to lack of quoting?

In general, EF Core's design should account for rules of the major languages used to program with it. A good design avoids making it easy to write code that is insecure in a way likely to go unnoticed by the initial dev or a security code review.

[divega]

@breyed @Mrswain The issue was previously brought to our attention e.g. at #9734 (comment), but we appreciate the level headed explanation and the additional examples provided here.

We are currently evaluating/discussing options within reach of the EF team, mainly trying to understand if changing things would actually result in a net improvement.

The problem with deprecating an overload (either the one that takes FormattableString or the one that takes RawSqlString) is that it might affect the usability of the API differently across different tools and IDEs, but this is indeed one of the options on the table. We are also going to brainstorm with the language team to see if they can come up with anything better.

What I think the "ideal" solution would be, really is not ideal because it cannot be backported to prior compilers. I think this is an example of the need for custom pragma warnings, similar to obsolete but not so ominous, but can be disabled with pragma warn markers. This would allow notice that using either of these functions could have SQL injection vulnerabilities, and maybe a pointer to interpolation issues, and the developer can then suppress them as a form of compliance acceptance.

Just my thoughts.

[breyed]

@Mrswain Such a pragma would have the same effect as marking the overload obsolete: It would mark existing use and discourage future use of the dangerous construct. The obsolete marker is much easier to implement. In either case, there still should be a safe, friction-free option for new code and for migration of existing code, such as a non-overloaded method that takes the FormattableString.

I had another thought, and this would not be a compiler change. Instead of RawSQLString, how about reversing it and making a wrapper called ParameterizedSqlString (or some such) that allows an explicit conversion from FormattableString. This would both signify that the formattable args will be processed as parameters and not break any 1.x migrated code..

As i work on my project I keep thinking about this and I have another thought as an addendum to my prior one.

Add a static public method to RawSqlString:

public static FormattableString Parameterized(FormattableString parameterizedSql)
{
    return parameterizedSql;
}

This can be used either via RawSqlString.Parameterized(...) or via just Parameterized(...) with a

using static Microsoft.EntityFrameworkCore.RawSqlString;

at the top.

Now you can obsolete the RawSqlString overload and re-add back a string overload. This would allow ported code to work, a warning for anybody compiled against the RawSqlString version, for explicit casting of FormattableString, and provide a helper to ease in explicit casting.