CODEWITHSUNDEEP
sql-serversql-server-2008t-sql
Control Freak
Control Freak

Reputation: 13233

Making a SQL Server Stored Procedure Safe from SQL Injections

This can be easily injected here because the @ID param can be practically anything in this SQL statement by inputting it, however, how do you prevent this exploit?

I prefer to specifically prevent this exploit at this level rather than application level, any suggestions?

CREATE PROCEDURE [dbo].[GetDataByID]
@ID bigint,
@Table varchar(150)
AS
BEGIN

Declare @SQL Varchar(1000)

SELECT @SQL = 'SELECT * FROM ' + @Table + ' WHERE ID = ' + CONVERT(varchar,@ID)

SET NOCOUNT ON;

EXEC(@sql)  
END

Upvotes: 5

Views: 1668

Answers (4)

deroby
deroby

Reputation: 6002

Although I'd advice against dynamic sql in general, in this case I think you can get away with it by checking if the @Table variable contains a valid table name.

  • The question is if you plan on allowing schema names and/or cross-db queries, I' assuming you don't want to go out of the db (or the server) here but do allow for different schema's (AdventureWorks shows how they can be used)
  • You MIGHT want to also include views for @Table.
  • It probably would be 'nice' if you also checked if the object found actually has an ID column and thrown a 'userfriendly' error if not. Optional though.

Just putting QuoteName() around @table will NOT protect you against everything. Although a great function, it's far from perfect. IMHO your best bet would be to parse the @Table variable, check if its contents is valid and then create dynamic sql based on the obtained parts. I started out doing most of above and surprisingly there it requires a LOT of checking for something that looks as simple as this =)

CREATE PROCEDURE [dbo].[GetDataByID] ( 
                                        @ID bigint,
                                        @Table nvarchar(300)
                                      )
AS

DECLARE @sql nvarchar(max)

DECLARE @server_name sysname,
        @db_name     sysname,
        @schema_name sysname,
        @object_name sysname,
        @schema_id   int        

SELECT @server_name = ParseName(@Table, 4),
       @db_name     = ParseName(@Table, 3),
       @schema_name = ParseName(@Table, 2),
       @object_name = ParseName(@Table, 1)

IF ISNULL(@server_name, @@SERVERNAME) <> @@SERVERNAME
    BEGIN
        RaisError('Queries are restricted to this server only.', 16, 1)
        Return(-1)
    END

IF ISNULL(@db_name, DB_Name()) <> DB_Name()
    BEGIN
        RaisError('Queries are restricted to this database only.', 16, 1)
        Return(-1)
    END


IF @schema_name IS NULL
    BEGIN
        IF NOT EXISTS ( SELECT *
                          FROM sys.objects
                         WHERE name = @object_name
                           AND type IN ('U', 'V') )
            BEGIN
                RaisError('Requested @Table not found. [%s]', 16, 1, @object_name)
                Return(-1)
            END

        SELECT @sql = 'SELECT * FROM ' + QuoteName(@object_name) + ' WHERE ID = @ID'
    END
ELSE
    BEGIN

        SELECT @schema_id = Schema_id(@schema_name)

        IF @schema_id IS NULL 
            BEGIN
                RaisError('Unrecognized schema requested [%s].', 16, 1, @schema_name)
                Return(-1)
            END

        IF NOT EXISTS ( SELECT *
                          FROM sys.objects
                         WHERE name = @object_name
                           AND schema_id = @schema_id
                           AND type IN ('U', 'V') )
            BEGIN
                RaisError('Requested @Table not found. [%s].[%s]', 16, 1, @schema_name, @object_name)
                Return(-1)
            END

        SELECT @sql = 'SELECT * FROM ' + QuoteName(@schema_name) + '.' + QuoteName(@object_name) + ' WHERE ID = @ID'
    END

EXEC sp_executesql @stmt   = @sql,
                   @params = N'@ID bigint',
                   @ID     = @ID

Return(0)

Supra compiles, but you might need to iron out some bugs as I didn't quite go as far as checking all code-paths.

Upvotes: 0

Sebastian Piu
Sebastian Piu

Reputation: 8008

Check this page, it has a wonderful guide to dynamic sql, and the options to execute them safely

In your case it should be like this:

SELECT @SQL =  N'SELECT * FROM ' + quotename(@Table) + N' WHERE ID = @xid' 
EXEC sp_executesql @SQL, N'@xid bigint', @ID

Upvotes: 9

Gats
Gats

Reputation: 3462

I would recommend avoiding dynamic sql altogether. The problems are as follows:

  • The obvious injection attach scenario
  • Binary injection attacks are much smarter and can bypass traditional string escaping
  • Performance is the big one - SQL Server is designed to manage execution plans on stored procedures and they will run faster than queries that are built dynamically. If you are using dynamic sql, there is no real benefit to using a stored procedure at all. If you want flexibility in code for selecting from multiple tables, you should consider an ORM or something to make your code easier. Considering you have to pass in the table dynamically, then I would go so far as to say there is no point to a procedure like the above and a different solution is the best option. If you are just writing against SQL (ie no ORM), then code generating seperate procs would even be a better option.

NOTE QUOTENAME will NOT garantee you are injection safe. Truncation injection is still possible. Read http://msdn.microsoft.com/en-us/library/ms161953.aspx before using it.

Upvotes: 0

KM.
KM.

Reputation: 103587

1) create a new table that will have an identity PK and contain the string table names
2) insert all/only the valid tables you will allow in your procedure
3) use this int identity PK as the input parameter value (TableID) for the stored procedure
4) in the procedure, just look up the string value (table name) from the given identity PK and you are safe to concatenate that looked up string in your query. 5) your WHERE clause is fine since you pass in an int

Upvotes: 1

Related Questions