Do I need to worry about a SQL injection for an SSRS report that uses a stored procedure for its main dataset?

Question

I have a report and I want to add a single valued text parameter (allows nulls). This parameter will be passed to a stored procedure. If this parameter is null, the SP will ignore it. However, if the parameter is not null, then I want the SP to parse through it and use some of the values contained within.

Users will be passing in values in the form whse=Blah&whse=Blah2&item=item1&lot=lotA&date:IsNull=True&um=&client=client2. As you might guess, these are parameter names and values for a report. My idea is to allow the user to pass in some parameter values that they want in this form.

However, I'm worried that someone could enter in some malicious text to try to damage my database. Do I need to worry about this, given that I am using SQL Server 2008 R2 and SyteLine 8? Or are there already protections in place for this and I shouldn't have to worry?

Answer 1

How are you parsing it and what are you doing with the results?

If you write code that builds a SQL statement, you are vulnerable to SQL injection.

Like suppose you write:

set @myquery = 'select customer_name from customer where customer_id = ''' + @custid + ''''
execute (@myquery)

And suppose you get @custid ultimately from a user input, say a text box on a form.

If the user enters

42

then the query becomes

select customer_name from customer where customer_id = '42'

Cool. Presumably that's what you intended.

But suppose the user enters

42'; update customer set balance_due = 0 where customer_id = '42

Now your query becomes

select customer_name from customer where customer_id = '42'; update customer set balance_due = 0 where customer_id = '42'

and the customer has just zeroed out his balance due. Or he might delete whole tables, etc.

The moral of the story is, Never ever EVER build a SQL query dynamically using user inputs. It gives users the ability to do anything they want to your database.

The right way to do this is:

select customer_name from customer where customer_id = @custid

Then if the user tries to use SQL injection, they just create a not-found on the record because the SQL string they type in is not a valid customer id. (Or if you don't do any type checking, they may get a data conversion error. Which is also bad and you should prevent that, but not as bad as SQL injection.)

Avoid building SQL statements dynamically. Whenever possible, pass values in as parameters.

Okay, sometimes you just have to build statements dynamically. The most common case I can think of is a selection-criteria screen where the user can type in values for 10 different criteria -- date in this range, name contains this, amount less than that, etc -- but each condition is optional, and if the user doesn't type a value, then you don't want that test as part of the query. So there are millions of possible combinations and you want to put the query together on the fly. That's fine, but DON'T insert the values when building the query on the fly. Insert parameters, and then set the parameters.

If values are generated within the program rather than from user input, and you are absolutely sure that these values will never contain SQL injection text, then it's safe to build values into your SQL. The only time I do this is when I have fixed, hard-coded values. Like if I might build code that says "where type=1" or it might say "where type=2", and the 1 and 2 are hard-coded like that in the program, not a user input. If in doubt, create parameters.

Answer 2

Short answer:

YES

You always have to use safe coding practices to avoid SQL injection vulnerabilities.

No language, framework, or RDBMS product can prevent SQL injection. Or, thinking about if another way, you always have the opportunity to write unsafe code, in spite of the features that exist to allow you to write safe code.

SQL injection defense is the developer's responsibility, and failing to follow safe coding habits and use the features that help you defend against SQL injection would be your fault.

Stored procedures alone do not make input data safe.

If you use input data in dynamic SQL statements, you should use sp_executesql() to make the dynamic parts into parameters, where possible. That is safe.

Here's an example from https://learn.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-executesql-transact-sql?view=sql-server-2017

EXECUTE sp_executesql   
          N'SELECT * FROM AdventureWorks2012.HumanResources.Employee   
          WHERE BusinessEntityID = @level',  
          N'@level tinyint',  
          @level = 109;  

Use query parameters. Don't just concatenate variables into your SQL strings.

For example, the following is unsafe, because @inBusEntityId might contain some content that makes your query do something you don't expect:

EXECUTE sp_executesql   
          N'SELECT * FROM AdventureWorks2012.HumanResources.Employee   
          WHERE BusinessEntityID = ' + @inBusEntityId;

Answer 3

It's good that you're thinking this through. There are ways you could design the report that would make it more vulnerable to SQL injection. For example, if you were using dynamic SQL and appending the parameter value directly into the query.

However, with the built-in parameter functionality, this is not an issue. The values are parameterized when you pass them to the stored procedure. If the value is not valid for any reason, it will not even attempt to run the query text. In other words, you don't have to think of all the different ways people could enter invalid strings.