0

Is the following safe from SQL injection? The user would pass in a comma separated list of codes as a string (Ex. '1', '1,3', '1,2,3', etc). I read that SQL parameters protect against SQL injection.

CREATE PROCEDURE CurrentActions   
    @StatusCodes nvarchar(100)   -- 0 = Active, 1 = Completed, 2 = On Hold
AS   
    SELECT ActionId, ActionName, ActionStatusCode, Comments  
    FROM Action
    WHERE ActionStatusCode IN (@StatusCodes)
GO

It seems like someone could inject malicious code into @StatusCodes, but what I've read, that's not the case, because using parameters will prevent that. Parameters can fail when using dynamic SQL statements and unsafe concatenation, but that's not what's happening here.

So my guess is that it's safe, however, I wasn't able to find anything specifically related IN clause and a string parameter inserted into the middle. Yes, I know there are other ways to pass in an array of ints, but this is a very convenient way to do it. But it has to be safe, of course, or I won't do this.

Thanks.

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
John
  • 3,332
  • 5
  • 33
  • 55
  • 5
    You'll end up with the single string value at IN this way. Consider [TVP](https://learn.microsoft.com/en-us/sql/relational-databases/tables/use-table-valued-parameters-database-engine?view=sql-server-ver16) instead – Serg Apr 06 '23 at 18:18
  • 3
    `IN ( @StatusCodes)` would be the same as `= @StatusCodes`; an `(n)varchar` containing a comma delimited string isn't parsed by the data engine as a tuple. – Thom A Apr 06 '23 at 18:21

1 Answers1

2

What you are seeking to do won't allow any sql injection, but it also will not work the way you want.

Say you pass a value like 1,2,3 via @StatusCodes.

This will NOT result in the ActionStatusCode IN (1,2,3) expression you're hoping for. Rather, you have one varchar value, so what you end up with is more like ActionStatusCode IN ('1,2,3'). The type mismatch means this probably won't even execute, and if it does execute it's only looking at 1,2,3 as one contiguous string instead of three separate integers.

To get around this, your options are a Table-Value Parameter (which I personally find awkward), using String_Split() to separate the values inside the procedure, or sending the values one at a time to a temp/session/shopping cart table or similar storage as the user selects them to use with a JOIN.

Honestly, none of these options are particularly great. If it came down to it, I'd probably use String_Split, though I need to get more comfortable using a TVP from ADO.Net.

Joel Coehoorn
  • 399,467
  • 113
  • 570
  • 794