Securing Dynamic SQL from SQL Injection

Question

I have a dynamic script running on all of the objects in a database and change the schema name for every one from [dbo] to the database name.

The script is working just fine, I would like to know if I can do anything better in order to secure it from SQL Injection?

BEGIN TRANSACTION

/* Change schema to all objects in database (from dbo)*/

DECLARE @SchemaName SYSNAME = db_name();
DECLARE @SQL NVARCHAR(MAX) = N'IF Not Exists (select 1 from sys.schemas where schema_id = SCHEMA_ID(@NewSchemaName))
        EXEC(''CREATE SCHEMA ''+@NewSchemaName+'''')' + NCHAR(13) + NCHAR(10);

SELECT @SQL = @SQL + N'EXEC(''ALTER SCHEMA ''+@NewSchemaName+'' TRANSFER [' + SysSchemas.Name + '].[' + DbObjects.Name + ']'');' + NCHAR(13) + NCHAR(10)
FROM sys.Objects DbObjects
INNER JOIN sys.Schemas SysSchemas
    ON DbObjects.schema_id = SysSchemas.schema_id
WHERE SysSchemas.Name = 'dbo'
    AND (DbObjects.Type IN ('U', 'P', 'V'))

EXECUTE sp_executesql @sql, N'@NewSchemaName sysname', @NewSchemaName = @SchemaName

ROLLBACK

In my quest of securing this one, I used this great article by Thom Andrews: Dos and Don'ts of Dynamic SQL

this is where I started: github.com/NathanLifshes

I cover a lot of the problems in my article you've linked above here. The key thing you want to be doing it properly quoting, using QUOTENAME. Rather than '...[' + SysSchemas.Name + ']...' you want '...' + QUOTENAME(SysSchemas.Name) + '...'. For your ALTER SCHEMA statement, you'll also inject @NewSchemaName as well (using QUOTENAME), as ALTER can't accept a variable as an input, it's needs a literal. — Thom A
– Thom A ♦, Commented Dec 10, 2019 at 8:52
thank for the comment. why do I need to use QUOTENAME on table columns I select from the database? — Nathan L
– Nathan L, Commented Dec 10, 2019 at 9:01
Inside the Alter I couldn't use any other object except sysname as an input (I tried using QUOTENAME with no luck) — Nathan L
– Nathan L, Commented Dec 10, 2019 at 9:08
2 reasons really. Someone could (though very unlikely) create an object to inject with, but also it'll properly quote nay object names that would have needed to be delimit identified. People do (foolishly) have object names with the ] character in them, and if you did they would need to be escaped properly. Your '[' and ']' that wrap it would cover any things like white space, but it's best to ensure that nothing can slip through. — Thom A
– Thom A ♦, Commented Dec 10, 2019 at 9:08
"'m using an EXEC command" Why are you creating dynamic SQL inside your dynamic SQL? — Thom A
– Thom A ♦, Commented Dec 10, 2019 at 10:15

Eitan Blumin · Accepted Answer · 2019-12-10 13:18:55Z

The script below should be much more secure. Note the use of the QUOTENAME function in the beginning of the script. This would work because if you use the QUOTENAME function "inline" inside an EXEC command, you may get a syntax error. So you need to apply it at an earlier stage. As luck would have it, you have such an "earlier" stage when you initialize the @SchemaName variable:

BEGIN TRANSACTION

/* Change schema to all objects in database (from dbo)*/

DECLARE @SchemaName SYSNAME = QUOTENAME(db_name());
DECLARE @SQL NVARCHAR(MAX) = N'IF Not Exists (select 1 from sys.schemas where schema_id = SCHEMA_ID(@NewSchemaName))
        EXEC(''CREATE SCHEMA ''+@NewSchemaName+'''')' + NCHAR(13) + NCHAR(10);

SELECT @SQL = @SQL + N'EXEC(''ALTER SCHEMA ''+@NewSchemaName+'' TRANSFER ' + QUOTENAME(SysSchemas.Name) + '.' + QUOTENAME(DbObjects.Name) + ''');' + NCHAR(13) + NCHAR(10)
FROM sys.Objects DbObjects
INNER JOIN sys.Schemas SysSchemas
    ON DbObjects.schema_id = SysSchemas.schema_id
WHERE SysSchemas.Name = 'dbo'
    AND (DbObjects.Type IN ('U', 'P', 'V'))

PRINT @SQL

EXECUTE sp_executesql @sql, N'@NewSchemaName sysname', @NewSchemaName = @SchemaName

ROLLBACK

Guy Glantser · Accepted Answer · 2019-12-10 12:27:37Z

0

In this case, I don't see an actual risk for SQL injection, since no value is supplied by a user. The script takes only the database name as an input. The only option to utilize SQL injection is by injecting a command into a database name. This is possible, of course. In order to protect against this option, you should use the QUOTENAME function to properly quote the schema name within your dynamic script.

answered Dec 10, 2019 at 12:27

Guy Glantser

11 bronze badge

1 Comment

Nathan L Over a year ago

Lets say we want to use this code and get a new Schema name from the user :-)

Collectives™ on Stack Overflow

Securing Dynamic SQL from SQL Injection

2 Answers 2

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related