2

I am wondering how safe the fromSqlRaw method is. I am doing the following in my code, where the person id is a parameter from the method itself:

string sql = String.Format("SELECT * FROM [users].[user] WHERE Id LIKE {0}", id)

var list = this.context.Person.FromSqlRaw<Person>(sql).ToList();

Is this code safe to SQL injection? And is there other security vulnerabilities that I should know of when using this?

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
  • 2
    use "[SqlParameter](https://www.sqlnethub.com/blog/using-the-csharp-sqlparameter-object-writing-more-secure-code/)" to protect you. Don't use any other assumptions. Your code will be changed, by you or someone else, the vulnerability may not be present now, but that's not a reason for "slacking off" and not use SqlParameter. (you've probably used more time writing this question and monitoring the answers than writing the sanitized version!) – Pac0 Aug 03 '20 at 08:48
  • But isn't my approach basically the same as a SqlParameter? –  Aug 03 '20 at 08:53
  • Note that the precise quesiton is not answerable as it is, IMHO. You don't show the type of "Id" (even though I suppose it is `string` because of the `LIKE`), and even if it's is an argument of your function, nothing shows us that it is not ultimately coming fom external data. – Pac0 Aug 03 '20 at 08:53
  • 1
    not at all. There is no sanitizing code here, as far as I see. And even though we may point you something specific, and then if you fixed this partiular weakness, it's not a good practice to do security by reinventing the wheel yourself, by trial and error, especially while you have an easy-to-use and experience-proof method at your disposal. – Pac0 Aug 03 '20 at 08:55
  • Let's say the parameter id is a string. Will this approach be vulnerable? –  Aug 03 '20 at 08:56
  • Okay! Then I will use the SqlParameter approach ;) –  Aug 03 '20 at 08:56
  • I guess it will be; of course it depends on whether `Id` can be set by the "outside" (e.g. user) or not . But even if it is "safe" now because it can't be set by the user, some months from now there will a slight "improvement" of the software because "customer wants to do more queries himself" and.... now you have an big bad hole in your backyard wall. – Pac0 Aug 03 '20 at 08:57
  • This was just an example, not the real code of course. But users can pass it from the url like: personId=1. –  Aug 03 '20 at 08:59
  • I am trying to implement this as I am using a context. However, this does not work: this.context.Database(sql, "@id", "1"). Any suggestions? –  Aug 03 '20 at 10:47
  • I think you can get inspiration here: https://learn.microsoft.com/en-us/ef/core/querying/raw-sql . Wiht `FromSql` and `FromSqlInterpolated`, you can use some parameters similarly as with `string.Format` or as `$"..."` (but the main difference is that internally it's using SQL parameters which will be sanitized, see the warning on the doc) – Pac0 Aug 03 '20 at 11:05

1 Answers1

3

Use proper parametrization for your input.

After clarifications in comments, it seems that your parameter is user-input string, this is a wide door opened for injection attacks.

Usually, you can create a SqlCommand, and provide some SqlParameter in it.

In EFCore, FromSqlRaw and FromSqlInterpolated (in 3.0, replacement for FromSql in EFCore < 3.0) allow you to shorten this syntax, see the documentation.

string sql = "SELECT * FROM [users].[user] WHERE Id LIKE {0}"
var list = this.context.Person.FromSqlRaw<Person>(sql, "42")

Note that this looks very similar to what you did in the question... But the difference is clearly emphasized in the documentation:

Warning

Always use parameterization for raw SQL queries

When introducing any user-provided values into a raw SQL query, care must be taken to avoid SQL injection attacks. In addition to validating that such values don't contain invalid characters, always use parameterization which sends the values separate from the SQL text.

In particular, never pass a concatenated or interpolated string ($"") with non-validated user-provided values into FromSqlRaw or ExecuteSqlRaw. The FromSqlInterpolated and ExecuteSqlInterpolated methods allow using string interpolation syntax in a way that protects against SQL injection attacks.

Indeed, in your case, the string was first interpolated as a string (without any sanity-check), then executed as-is.

FromSqlRaw had no idea that the "Id" part was coming from a parameter.

Pac0
  • 21,465
  • 8
  • 65
  • 74
  • Thank you for a good answer! I am using this approach now, is it safe? https://paste.ubuntu.com/p/wtBVq2Zgm3/ –  Aug 03 '20 at 12:02
  • Updated code with function: https://paste.ubuntu.com/p/hSD9wPTfHD/ –  Aug 03 '20 at 12:11
  • However, from this article: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.relationalqueryableextensions.fromsqlraw?view=efcore-3.1, it seems like my approach actually was ok? –  Aug 03 '20 at 12:40
  • concerning your code "paste.ubuntu.com" links: correct, but you should remove the `String.Format` which is currently unused, and misleading (if it were used, that would be wrong). – Pac0 Aug 03 '20 at 12:43
  • concerning your comment with the link from Microsoft doc: Good, and that was what I was trying to convey in my answer: you can use a `string.Format`-like syntax so it looks "friendly" to you, along with the advantages of the the correct parameters-sanitizing. – Pac0 Aug 03 '20 at 12:45
  • Removed the String.Format now. Anyways, didn't you say my code was unsecure? I am confused now.. –  Aug 03 '20 at 12:55
  • @newbie which one are we talking about? The ones posted on comments here are good. The one in your question is insecure. Are you confused about the reason? We can discuss it until it's clear. I may have overlooked something or written something wrong while thinking another thing. – Pac0 Aug 03 '20 at 12:55
  • I am talking about the article. Because in my question I am using id from a input parameter from my function like this: https://paste.ubuntu.com/p/4kVBsM6krC/, and from what I can see, this should be ok. And if this approach is ok, is SqlParameter more secure? –  Aug 03 '20 at 13:00
  • SqlParameter is secure. In https://paste.ubuntu.com/p/4kVBsM6krC it is not. The reason is because in your code this is only a _string_ operation. Even though in the article it _looks like_ the same as `string.Format`, it is **not** it. `string.Format("... {0}", id)` is "blind"/ "dumb", it will replace `{0}` by whatever you give in `id`. Using `ToSqlRaw(".... {0}", id)` is smart and ok, because it should take care of checking the way it is quoted and filter injection attacks. Using `SqlParameter` is ok for the same reasons. – Pac0 Aug 03 '20 at 13:41
  • Aha! So by dropping String.Format, both should work? –  Aug 03 '20 at 13:43
  • Yes, it is exactly that! this apparently harmless `string.Format` is the "evil" part. Note that using `$"{id}"` would cause the same issue, instead you should use `FromSqlInterpolated` _without_ the `$`, as far as I understand (not tested). – Pac0 Aug 03 '20 at 13:45
  • 1
    Thank you so much for your comprehensive answer! Now I understand it. –  Aug 03 '20 at 13:47
  • Another question for you. When I want to use this with schema names, I do the following: https://paste.ubuntu.com/p/kJTny6GKfn/ to access the schema. It does not work without the `"+ + "`. Is this safe? –  Aug 04 '20 at 10:52
  • @newbie If the value comes from the user, then you are doing string concatenation without checking, so it's not safe. – Pac0 Aug 04 '20 at 11:34
  • BTW, the code is short enough to fit in a comment : `var naturalCodeParameter = new SqlParameter("@aSchemaName", aSchemaName); string sql = @"SELECT * FROM [Schema].["+@aShcemaName+"]"` – Pac0 Aug 04 '20 at 11:35
  • 1
    Unfortunately, it seems you cannot set the _schema_ as a parameter. (see https://stackoverflow.com/questions/9094370/how-to-use-a-sqlcommand-parameter-to-specify-the-schema-name-for-a-select-query). As an answer there stated: you can at least restrict characters to what's needed. Additionally, you could also get all existing schema names, and check that your variable holds an existing one. – Pac0 Aug 04 '20 at 11:40
  • Okay! Thanks for answer ;) –  Aug 04 '20 at 13:47
  • Just so that I understand it clearly. Won't the @shcemaName be a input value in the database and will not run as a script? If this is true, then this should be safe enough? –  Aug 04 '20 at 13:50
  • Ok, I am a bit confused by the names, reading too quickly maybe. 1) Your "schema name" variable is not the name of the schema, but the name of a _table_ . 2) I don't understand what "run as a script" means here 3) not sure about "input in the database" either, but I guess you mean a parameter as we discussed above? – Pac0 Aug 04 '20 at 14:33
  • first, even if we're talking about a _table_ instead of a _schema_, unfortunately you cannot use a "parameter". See this other Q&A for some details: https://stackoverflow.com/questions/33254721/mysql-table-name-as-parameter . The whole story about "sql parameters" to protect you against injection attacks is for _values_ only. (`SELECT * FROM Table WHERE Column = ***Value***`). Here, you cannot use this technique to replace value by a variable `@value`. You should validate yourself. – Pac0 Aug 04 '20 at 14:37
  • 1
    *You should validate yourself the schema, table name and column names if needed. Currently, with the `string query = "SELECT * FROM [schema].[" + variableName + "] WHERE isPublic=1"`;, this is string concatenation. Injection attacks need to be bit more complex, but to give you an exemple, imagine that `variableName` is `users] --`. Now I can get all users of your database, without any restriction, any WHERE clause would be commented out. Worse, imagine something like `variableName` is `users]; DROP TABLE [schema].[users]; --`. – Pac0 Aug 04 '20 at 14:47
  • (again, that's more complicated than that, but I hop you get the idea) – Pac0 Aug 04 '20 at 14:49