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
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
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
I would recommend avoiding dynamic sql altogether. The problems are as follows:
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.
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.
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.
@Tableparam that's the problem here?@IDis abigint, so it can only be a number when you reach the point of building the dynamic SQL statement, right?