OK, I know many of you have seen this before (an oldie, but a goodie!):
(image from xkcd.com, with "copy and share" license described here: License)
But, what can you do to prevent this? And, when would this even be possible?
This is possible when DSE (dynamic string execution) occurs. There are still some VERY relevant and important reasons to use DSE and some are performance related (ok, this is another post for another day) but suffice it to say - I use DSE but I also know how to prevent Little Bobby Tables from running amuk within my database.
(April 5 note: after a few comments about sp_executesql, I've modified the following paragraph to explain a bit more... I'm *very* aware of sp_executesql and while it does have some benefits, it also has some negatives! I promise, this is coming soon in another post. I really want this post to focus solely on how to minimize injection when you do NEED to use a dynamically constructed string and there are times when this is the ONLY way to solve certain performance problems. sp_executesql - while often compared to EXEC as a better choice just isn't ALWAYS better and really it's not a simple comparison as they are not equivalent!! In fact, there are some cases where I would specifically avoid sp_executesql... So, I promise... this will be coming up in a blog post soon!! Great comments! Thanks!! :))
So, while the discussion on when, where and why you might use DSE is another one - especially when compared with sp_executesql (yes, I hear another blog post coming - and, well, I'm kind of on a roll right now :)... I want to give you a couple of very cool tips/tricks to reduce the possibilities of problems if/when you do use it (and, there REALLY are places where it's necessary!). It's especially important as there are a few things that many folks just don't even know exist. And, there are 3 parts to this article... I would expect that most apps need QUOTENAME() and/or REPLACE() but ALL apps should consider my recommendations/comments for EXECUTE AS - if/when you're actually using DSE (through EXEC).
-
QUOTENAME()
-
REPLACE()
-
EXECUTE AS
Part I: QUOTENAME()
In a question/thread (this is many moons ago), the following question was asked (NOTE: I have "tweaked" the code to highlight ONLY the problem here AND I've made it so that it won't execute):
I have a procedure that creates a database (below). When people use spaces in their database names it fails. How can I make it work if they supply spaces?
CREATE PROCEDURE dbo.CreateDBProc
(
@DBName sysname
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @ExecStr = N'CREATE DATABASE ' + @DBName -- + all of the other stuff to place the files, etc...
SELECT @ExecStr
--EXEC(@ExecStr)
go
And, there was a reply to this question where the answer was (and I am NOT recommending this answer. Please do NOT stop reading here otherwise you are asking for a world of pain - and LOTS of SQL Injection):
Just go ahead and do the following. By putting brackets around the name you will solve this problem.
CREATE PROCEDURE dbo.CreateDBProc
(
@DBName sysname
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @ExecStr = N'CREATE DATABASE [' + @DBName + N']' -- + all of the other stuff to place the files, etc...
SELECT @ExecStr
--EXEC(@ExecStr)
go
OK, this is a SCARY recommendation. Not just because it won't always work (what if someone actually wants [don't get me wrong - I don't know WHY they'd want to do this but...] to put [brackets] in the name?) but because it's prone to SQL Injection.
This won't work:
EXEC dbo.CreateDBProc N'This is the most stupid :) name I can think of with [brackets] and all sorts of junk! in the name'
And, this is just scary what I can do:
EXEC dbo.CreateDBProc N'fakedbname] DROP DATABASE foo--'
EXEC dbo.CreateDBProc N'[fakedbname] EXEC(''CREATE PROC Test AS SELECT * FROM pubs.dbo.authors'') DROP DATABASE foo --'
So, what should we do?
ANSWER: If you're building strings where identifiers are parameterized (and, there are many reasons for why this many not be the best idea - but, if you absolutely must), USE QUOTENAME()!
So, here's the correct way to code this procedure so that an identifier cannot be subjected to SQL Injection:
CREATE PROCEDURE dbo.CreateDBProc
(
@DBName sysname
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @DBName = QUOTENAME(@DBName, N']')
SELECT @ExecStr = N'CREATE DATABASE ' + @DBName -- + all of the other stuff to place the files, etc...
SELECT @ExecStr
--EXEC(@ExecStr)
go
Now, no matter what messed up name someone actually WANTS to create - this will protect it such that SQL Injection is not possible. You can try any of these combinations with this version of the proc and while creating a database actually named "[fakedbname] DROP DATABASE foo --" probably isn't desirable, dropping a database isn't either (and I would argue is MUCH worse).
All of these executions are "protected" from SQL Injection:
EXEC dbo.CreateDBProc N'this is my test database name'
EXEC dbo.CreateDBProc N'[fakedbname] DROP DATABASE foo --'
EXEC dbo.CreateDBProc N'[fakedbname] EXEC(''CREATE PROC Test AS SELECT * FROM pubs.dbo.authors'') DROP DATABASE foo --'
EXEC dbo.CreateDBProc N'This is the most stupid :) name I can think of with [brackets] and all sorts of junk! in the name'
Note: The first and second execute (if you actually execute instead of just SELECT @ExecStr) *and* create databases with these ugly names. The third and fourth don't execute because some of the injected characters are not supported for file/directory names. However, the most important point is that *all* are protected from SQL Injection!
Part II: REPLACE()
But, what if the string you need to work with isn't an identifier? Can you still use QUOTENAME()? And, the answer is YES, IF the string is compatible with SYSNAME which is an nvarchar(128). However, if you need longer strings or if you are protecting a string which is not an identifier - what else can you do? Well... it's definitely NOT as pretty but it works. The answer here is to use REPLACE. If I had wanted to do this with the procedure above, I would have had to do the following:
CREATE PROCEDURE dbo.CreateDBProcQuotes
(
@DBName sysname
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @DBName = REPLACE(@DBName, N'''', N'''''')
SELECT @ExecStr = N'CREATE DATABASE ' + @DBName -- + all of the other stuff to place the files, etc...
SELECT @ExecStr
--EXEC(@ExecStr)
go
However, this will NOT work here because an identifier must be properly quoted within the string and there are only 2 ways to "quote" identifiers. One is the [bracket] and the other is "double quotes" not single. Instead, this is what the procedure will look like (nope! we don't need REPLACE):
CREATE PROCEDURE dbo.CreateDBProcQuotes
(
@DBName sysname
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @ExecStr = N'CREATE DATABASE [' + @DBName + N']'-- + all of the other stuff to place the files, etc...
SELECT @ExecStr
--EXEC(@ExecStr)
go
And, SQL only supports brackets by default (without any special settings). To allow double quotes, you can turn on QUOTED_IDENTIFIER (SET QUOTED_IDENTIFIER ON) to allow double quotes instead of brackets but this has a few negative consequences (See SET Options That Affect Results) and personally, I always try to avoid making SET options changes in most of my stored procs.
CREATE PROCEDURE dbo.CreateDBProcQuotes
(
@DBName sysname
)
AS
SET QUOTED_IDENTIFIER ON
DECLARE @ExecStr nvarchar(2000)
SELECT @DBName = REPLACE(@DBName, N'''', '''''')
SELECT @ExecStr = N'CREATE DATABASE "' + @DBName + N'"'-- + all of the other stuff to place the files, etc...
SELECT @ExecStr
--EXEC(@ExecStr)
go
Once again, REPLACE isn't needed. And, to be honest, this becomes a nightmare with " [double quotes] or actual [brackets] in the name. QUOTENAME() deals with those properly. So, with an identifier you really need QUOTENAME(). So, where does replace come in? With actual strings in your DSE. Imagine that you're wanting to pass in a first name but something about your application requires that this happen through DSE (and, this CAN definitely happen - mostly for performance purposes). Don't get me wrong, I just don't have the ability to do all of this within one post but simply put, because of how stored procedures create their execution plans there are OFTEN reasons for why you might want a string to be built dynamically. No, probably not one quite this simple but I'm trying to keep this particular example simple. This is probably what it would look like to start:
CREATE PROCEDURE GetMembers
(
@FirstName nvarchar(50)
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @ExecStr = N'SELECT * FROM dbo.member WHERE FirstName = ' + @FirstName
SELECT @ExecStr
-- EXEC(@ExecStr)
go
And, on first test:
EXEC GetMembers N'Kimberly'
And, on first test it generates: SELECT * FROM dbo.member WHERE FirstName = Kimberly which gives you an error because the firstname isn't properly quoted. So, you think... oh, I need to quote these in my EXEC statement:
ALTER PROCEDURE GetMembers
(
@FirstName nvarchar(50)
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @ExecStr = N'SELECT * FROM dbo.member WHERE FirstName = ''' + @FirstName + N''''
SELECT @ExecStr
--EXEC(@ExecStr)
go
EXEC GetMembers N'Kimberly'
Ah, that works... for awhile. Then, someone complains that it doesn't work for C'Anne (I just met someone named C'Anne - pronounced SeaAnne) this past week.
EXEC GetMembers N'C''Anne'
And, it generates:
SELECT * FROM dbo.member WHERE FirstName = 'C'Anne'
which gives you a syntax error.
Msg 102, Level 15, State 1, Line 1
Incorrect syntax near 'Anne'.
Msg 105, Level 15, State 1, Line 1
Unclosed quotation mark after the character string ''.
Darn. But, at least this is how you catch it. Instead of with Little Bobby Tables. What if this gets executed:
EXEC GetMembers N'''Robert''); DROP TABLE Students; --'''
Luckily, I got:
Msg 3701, Level 11, State 5, Line 1
Cannot drop the table 'Students', because it does not exist or you do not have permission.
But, what if there had been a table called Students AND the context of the execution was such that the could execute this? Well, that's what's proposed by the cartoon and well... I've *unfortunately* heard of that happening!
So, what do you do? First, you need to protect the string being inserted! This is what you need:
ALTER PROCEDURE GetMembers
(
@FirstName nvarchar(50)
)
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @ExecStr = N'SELECT * FROM dbo.member WHERE FirstName = ''' + REPLACE(@FirstName, N'''', N'''''') + N''''
SELECT @ExecStr
--EXEC(@ExecStr)
go
And, this requires that the string be surrounded with quotes AND the string submited have all single quotes replaced with '' [single quote followed immediately by a second single quote]). Yes, I realize that this is starting to look ugly but... what happens with Little Bobby Tables? We get his row. NOTE: We do have other potential performance problems here because the parameter (when it gets thrown back out into the string - is no longer an NVARCHAR, it's only a varchar). So, we should also consider adding an N into the string like the following:
SELECT @ExecStr = N'SELECT * FROM dbo.member WHERE FirstName = N''' + REPLACE(@FirstName, N'''', N'''''') + N''''
And, what if the string being built has numerics or other types - what should we do there? To be honest, I often EXPLICITLY type the variable in what looks like a completely unnecessary CONVERT. However, for performance purposes I *often* do this. Ugh, the future posts are REALLY starting to build from here! Again, I'm not hitting performance in this one so I'm not going to take the bait of the tangent. Another time for sure!
Finally, what if our strings are REALLY complex and we need to do some strange stuff and well... we're not sure that the string can be completely protected? What if we're passing in a much more complicated "piece" of a WHERE clause or a full and customized ORDER BY? What if it's not just a SINGLE value? Ugh... this is much harder but you definitely need EXECUTE AS.
Part III: EXECUTE AS
This is the ultimate in protection for your DSE code. This is where you decide that you're going to allow virtually anything in the WHERE clause or ORDER by but you really want to protect your database. Here, what I need is EXECUTE AS. And I'm not a big fan of this kind of code - but, again, I'm not looking at perf or anything here... I'm *just* looking at the security and potential of the injection.
SQL Server 2005 introduced EXECUTE AS and it has a lot of options - in fact, 4 options:
- EXECUTE AS Caller
- EXECUTE AS User
- EXECUTE AS Self
- EXECUTE AS Owner
EXECUTE AS Caller is the default. This means that the user that executes the procedure must have the DIRECT rights associated with the Dynamically Executed String. And, so, this is good. This should protect MOST cases altogether. However, what if the users have higher privileges? Well, I'd say that there's already something wrong with your database (because this should NEVER be the case) but... well... I've seen apps (and even RECENTLY) heard of apps that connect as SA. Yes, seriously. So... what can they do... Well, much worse that DROP TABLE. How about DROP DATABASE. Or, well, let's just say that if I found vulnerabilities in that - I could possible get outside of their SQL Server and do even more damage - to files, to the network. So... this is REALLY REALLY REALLY REALLY BAD. I would STRONGLY suggest that anyone that connects as SA - IMMEDIATELY WORK HARD TO CHANGE THIS.
EXECUTE AS Owner is almost as bad (although nothing's as bad as connecting as SA) - especially when the OWNER has elevated rights. So, I'd try to avoid EXECUTE AS Owner unless you SEVERELY restrict who has permissions to the procedure in general. But, don't EXECUTE AS Owner for a stored procedure that's granted EXEC to public. That completely defeats the purpose!
EXECUTE AS Self is a bit strange. But, when a stored procedure is created in a schema, the ownership chaining and privileges follow from the OWNER of the schema. So, what if the author of the stored procedure has rights that the owner doesn't. No, I don't really recommend this but that's where "self" comes in. This is where the procedure can execute under the context of the actual creator (not the owner). However, often these are the same and often they're DBO. This is even worse... Although still, not as bad as connecting as SA.
EXECUTE AS User (where the User is a low-privileged user with very few rights) is good. And, in fact, this will be the answer to this problem!
Yes, even if you connect as SA, I can STILL protect you. Yes, we can still protect this code even when run under the connection context of SA - with EXECUTE AS.
CREATE USER User_GetMembers
WITHOUT LOGIN
go
GRANT SELECT ON Member TO User_GetMembers
go
ALTER PROCEDURE GetMembers
(
@FirstName nvarchar(50)
)
WITH EXECUTE AS N'User_GetMembers'
AS
DECLARE @ExecStr nvarchar(2000)
SELECT @ExecStr = N'SELECT * FROM dbo.member WHERE FirstName = ''' + REPLACE(@FirstName, '''', '''''') + ''''
SELECT @ExecStr
--EXEC(@ExecStr)
go
And, even if for some reason you can't use REPLACE() this procedure will be limited in what they can do because the user (User_procedurename) has limited rights within the database. In fact, I would recommend that the user ONLY have the necessary rights for THAT stored procedure and I'd even recommend that you create one user for each stored procedure (that has DSE). Yes, that's potentially a lot of users... but, better than the alternative(s)!
Summary/Resources
Finally, please be sure that you're doing some form of auditing if you're using EXECUTE AS. And, you should also do a quick check to see if anyone is ELEVATING user's rights to a more privileged user. Check out these two blog posts for additional information:
And... ah... I'm done! There's a lot here but a lot of tips. You CAN prevent SQL Injection.
Thanks for reading!
kt