How hard is it to try to make your SQL queries safe?

Question

I am in a situation where I am provided with a comma-separated VarChar as an input to a stored procedure. I want to do something like this:

SELECT * FROM tblMyTable INNER JOIN /*Bunch of inner joins here*/ WHERE ItemID IN ($MyList); 

However, you cannot use VarChar with the IN operator. There are two ways around this problem:

• (Incorrect path) Create an SQL query in String, for example:

  SET $SQL = ' SELECT * FROM tblMyTable INNER JOIN /*Bunch of inner joins here*/ WHERE ItemID IN (' + $MyList + ');

  EXEC($SQL);

• (Correct path) Create a temporary table containing the values ​​of $MyList , then join this table in the original query.

My question is:

Option 2 is relatively successful in creating a temporary table that is less than ideal.

While Option 1 is open to SQL injection attack, since my SPROC is being called from an authenticated source, does it really matter? Only trusted sources will run this SPROC, so if they want to connect to the database, this is their prerogative.

So how far could you make your code safe?

Answer 1

What database are you using? in SQL Server, you can create a split function that can split a long string and return a subsecond table. you use a table function call like a regular table in a query (no temp table needed)

You need to create a split function, or if you just use it. Here's how to use the split function:

 SELECT * FROM YourTable y INNER JOIN dbo.yourSplitFunction(@Parameter) s ON y.ID=s.Value 

I prefer the number table approach to split strings in TSQL , but there are many ways to split strings in SQL Server, see the previous link that explains the PRO and CON of each of them.

For the Numbers Table method to work, you need to perform this setting of a single time table that will create a Numbers table that contains rows from 1 to 10000:

 SELECT TOP 10000 IDENTITY(int,1,1) AS Number INTO Numbers FROM sys.objects s1 CROSS JOIN sys.objects s2 ALTER TABLE Numbers ADD CONSTRAINT PK_Numbers PRIMARY KEY CLUSTERED (Number) 

Once the Numbers table is configured, create this split function:

 CREATE FUNCTION [dbo].[FN_ListToTable] ( @SplitOn char(1) --REQUIRED, the character to split the @List string on ,@List varchar(8000)--REQUIRED, the list to split apart ) RETURNS TABLE AS RETURN ( ---------------- --SINGLE QUERY-- --this will not return empty rows ---------------- SELECT ListValue FROM (SELECT LTRIM(RTRIM(SUBSTRING(List2, number+1, CHARINDEX(@SplitOn, List2, number+1)-number - 1))) AS ListValue FROM ( SELECT @SplitOn + @List + @SplitOn AS List2 ) AS dt INNER JOIN Numbers n ON n.Number < LEN(dt.List2) WHERE SUBSTRING(List2, number, 1) = @SplitOn ) dt2 WHERE ListValue IS NOT NULL AND ListValue!='' ); GO 

Now you can easily split the CSV row into a table and join it:

 select * from dbo.FN_ListToTable(',','1,2,3,,,4,5,6777,,,') 

CONCLUSION:

 ListValue ----------------------- 1 2 3 4 5 6777 (6 row(s) affected) 

You can use a CSV row like this rather than a temp table:

 SELECT * FROM tblMyTable INNER JOIN /*Bunch of inner joins here*/ WHERE ItemID IN (select ListValue from dbo.FN_ListToTable(',',$MyList)); 

Answer 2

I personally would prefer option 2 in that just because the source is authenticated does not mean that you should let go of your guard. You will leave yourself open to potential escalations of rights where an authenticated user with a low level of lvl will be able to execute commands against a database that you did not plan.

The phrase you use "trusted sources" might be better if you assume that X-Files is aproach and you don't trust anyone.

Answer 3

If someone is eavesdropping on the database, you can still call.

A good option, similar to option 2, is to use a function to create a table in memory from a CSV list. This is fast enough and provides protection for the second option. Then this table can be attached to the Internal join, for example.

 CREATE FUNCTION [dbo].[simple_strlist_to_tbl] (@list nvarchar(MAX)) RETURNS @tbl TABLE (str varchar(4000) NOT NULL) AS BEGIN DECLARE @pos int, @nextpos int, @valuelen int SELECT @pos = 0, @nextpos = 1 WHILE @nextpos > 0 BEGIN SELECT @nextpos = charindex(',', @list, @pos + 1) SELECT @valuelen = CASE WHEN @nextpos > 0 THEN @nextpos ELSE len(@list) + 1 END - @pos - 1 INSERT @tbl (str) VALUES (substring(@list, @pos + 1, @valuelen)) SELECT @pos = @nextpos END RETURN END 

Then in the connection:

 tblMyTable INNER JOIN simple_strlist_to_tbl(@MyList) list ON tblMyTable.itemId = list.str 

Answer 4

Option 3 is to confirm that each item in the list is actually an integer before concatenating a string in an SQL statement.

Do this by parsing the input string (for example, dividing by an array), loop through and convert each value to int, and then recreate the list before concatenating back into an SQL statement. This will give you reasonable assurance that SQL injection cannot happen.

It is safer to concatenate the strings created by your application because you can do things like check for int, but it also means that your code is written in such a way that a subsequent developer can modify it a bit, thereby opening a backup copy of the risk of SQL injection, since they don’t understand what your code protects. Make sure you comment well on what you are doing if you are following this route.

Answer 5

Third option: pass values ​​to a stored procedure in an array. You can then either collect the comma-separated string into your code, or use the dynamic SQL parameter, or (if your choice of RDBMS allows this) use the array directly in the SELECT statement.

Answer 6

Why don't you write a CLR separation function that will do everything you need and easily? You can write user-defined table functions that return a table that splits rows with the .Net infrastructure. Hell in SQL 2008, you can even give them hints if they return rows sorted anyway ... like climbing or something that can help the optimizer? Or maybe you cannot use CLR integration, then you should stick to tsql, but I personally will go to Soluton CLR