1

I'm writing some CRUD functions for media associated with certain products. To delete many records, I've been told to write the query as follows:

dataContext.ExecuteCommand("DELETE FROM ProductMedia WHERE ProductId = {0}", productId);

He says this is safe from SQL injection because it's 'parameterized'. I don't agree, but I also don't know a better way.

How do you protect from SQL injection when writing commands in this way?

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
Yahtzei
  • 13
  • 3
  • 3
    Did you read the docs for `ExecuteCommand`? https://learn.microsoft.com/en-us/dotnet/api/system.data.linq.datacontext.executecommand?view=netframework-4.8 – mjwills Jul 19 '19 at 09:53
  • 2
    Why don't you agree? Can you explain to me how the parametrized query is susceptible to injection? – CompuChip Jul 19 '19 at 09:55
  • 1
    `ExecuteCommand("DELETE FROM ProductMedia WHERE ProductId = {0}", productId)` is parametrized and safe. `ExecuteCommand(string.Format("DELETE FROM ProductMedia WHERE ProductId = {0}", productId))` is not. – GSerg Jul 19 '19 at 09:59
  • I've read the docs now, thank you for that. I just thought you might be able to pass something in like `"1 OR 1=1; UPDATE SensitiveRecords SET Password = ''"`. – Yahtzei Jul 19 '19 at 10:05
  • 2
    @Yahtzei You can pass that. It will delete the product whose Id equals `"1 OR 1=1; UPDATE SensitiveRecords SET Password = ''"`. – GSerg Jul 19 '19 at 10:14
  • As a last resort, you can always check [what it does](https://referencesource.microsoft.com/#System.Data.Linq/DataContext.cs,6d9aaf907e82e81f) that method checking the source code. – Cleptus Jul 19 '19 at 10:25

1 Answers1

2

He says this is safe from SQL injection because it's 'parameterized'.

As per the docs, the code in your question is perfectly valid and will be parameterised correctly:

The syntax for the command is almost the same as the syntax used to create an ADO.NET DataCommand. The only difference is in how the parameters are specified. Specifically, you specify parameters by enclosing them in braces ({…}) and enumerate them starting from 0. The parameter is associated with the equally numbered object in the parameters array.

It is essentially a wrapper - and it takes care of creating the necessary SqlParameter objects and assigning values to them. Other ORMs (like PetaPoco) have similar mechanisms.

Generally, the thing you want to avoid is string concatenation (or something that is effectively concatenation - like string.Format) - which this solution does not use. String concatenation is almost always open to SQL Injection. But, again, the code in your question is fine.

mjwills
  • 23,389
  • 6
  • 40
  • 63