0

I have been converting insecure dynamic queries to parameterized queries. I have figured out ways to exploit most of the weaker attempts people have made in an attempt to sanitize their input but I have not figured out how to actually exploit a query that is essentially 

DECLARE @input VARCHAR(100) = ''';SELECT ''INJECTED''--'
DECLARE @SQL NVARCHAR(100) = 'SELECT ''example'' WHERE ''1'' = ''' + REPLACE(@input, '''', '') + ''''
EXEC sp_executesql @SQL

or

DECLARE @input VARCHAR(100) = ''';SELECT ''INJECTED''--'
DECLARE @SQL NVARCHAR(100) = 'SELECT ''example'' WHERE ''1'' = ''' + REPLACE(@input, '''', '''''') + ''''
EXEC sp_executesql @SQL

I have seen people online break escaping (in MySQL but never TSQL) but I have not been able to find any way to break stripping single quotes.

I plan on fixing the above instances, too, but I cannot figure out how to demonstrate that they are insecure.

How can you exploit the above queries?

Marie
  • 2,114
  • 1
  • 17
  • 31
  • 2
    No that is not effectively safe. You can still send in binary data which will contain no apostrophes but will still wreak havoc. Why not use proper parameterized queries and stop executing strings? There are VERY few times when you really need dynamic sql...and even dynamic sql can be parameterized. – Sean Lange Apr 27 '17 at 16:48
  • Doubling quotes is not even close to safe. – Sean Lange Apr 27 '17 at 16:51
  • 2
    Invaluable reference on dynamic sql in sql server: [The curse and blessings of dynamic SQL - Erland Sommarskog](http://www.sommarskog.se/dynamic_sql.html) – SqlZim Apr 27 '17 at 16:54
  • 1
    Why spend time and effort pondering these things. The right solution is parameterization. That is 100% effective because it separates *code* from *data*. Even if *we*, random strangers on the internet, tell you that your mad encoding scheme is okay, how will you be *sure*? – Damien_The_Unbeliever Apr 27 '17 at 16:55
  • I already said I am fixing these. I am the one that discovered them and demanded that we fix them. For my own knowledge I am curious if there is an easy way around the two methods I posted. – Marie Apr 27 '17 at 17:03
  • @Marie - well, the obvious problem with option one is that `'`s could very well be *valid data* and you're wrecking it. I'm not immediately aware of a simple escape from option two. But there is **no** good reason to do either so it seems pointless to ask. – Damien_The_Unbeliever Apr 27 '17 at 17:10
  • I ask because I want to know. What more reason does anybody need? Besides that the danger effects the priority which decides when/if I am allowed to fix these issues. – Marie Apr 27 '17 at 17:16
  • So, we go back to my previous comment. You're willing to re-prioritise work based on "random strangers on the internet told me it's safe", rather than fixing the problem? That's not a sane process to work with. – Damien_The_Unbeliever Apr 27 '17 at 17:18
  • Your reading comprehension needs work. I dont get a say in the matter. If i can prove it can be broken then I can fix it now. If I cannot prove it can be broken they may say we will fix it eventually. I plan to argue for fixing it immediately regardless but it would definitely help my case if somebody answered my question instead of arguing with me about it. – Marie Apr 27 '17 at 17:28
  • @Marie [Some good reading on the subject](https://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string). But more importantly, by stripping out the `'`'s, you're *destroying user data*. You should *never* be altering a user's input to protect against SQL Injection. Additionally, yes, it is very possible to do SQLi with your `'` "fix." Use what's been proven to work, otherwise you're just waiting for someone to be creative and exploit it. Be *proactive*, **not** *reactive*... – Siyual Apr 27 '17 at 17:33
  • @Marie ... and we *are* answering your question. If your management or whoever is pushing back on these changes because "it hasn't happened yet," you need to make them understand that the changes are needed. The last thing you want is to find out the hard way. If your company can't understand that risk, maybe you should look for a new company. Use parameterized sql. Period. There is not an alternative. Everything else is exploitable. – Siyual Apr 27 '17 at 17:35
  • You're getting your answer, but not in the form you wanted. *Nobody* considers escaping to be "effectively safe". We don't spend lots of time trying to *prove* that it's unsafe. We know that the fundamental flaw that is surfaced by SQL injection is when data is treated (potentially) as code. So we avoid that situation completely and know that we're safe (from SQLi) – Damien_The_Unbeliever Apr 27 '17 at 18:04
  • I think what you want is only an answer to your question and not an explanation. In that vein here is my answer to is the original query here safe. NO – Sean Lange Apr 27 '17 at 18:14
  • With binary as I mentioned previously. I will update my answer with a working example to show you. – Sean Lange Apr 27 '17 at 18:32
  • That would be awesome, thank you – Marie Apr 27 '17 at 18:32
  • Take a look at the edits to my original answer. – Sean Lange Apr 27 '17 at 18:39

1 Answers1

4

You can and should still parameterize your dynamic sql to make it truly safe. Your example query could be something like this.

declare @z int = 10

EXEC sp_executesql 'SELECT x FROM y WHERE MyColumn = @z', N'@z int', @z

Now we have created a parameterized query inside your dynamic sql and passed the parameter to the execution. This is NOT vulnerable to sql injection.

--EDIT--

Since you wanted an answer to the question of if your original query is vulnerable to sql injection I will demonstrate that it is in fact quite open. You are only dealing with single quotes which is truly on the tip of the iceberg when it comes to sql injection. Have you considered what happens if instead of a string they pass in a binary representation of a string?

Let's say your query is receiving a nvarchar(max) as a parameter. In my example I call this @BadCode. Now in my example I am not doing any harm to your system but in the wild this binary could be literally anything.

Here is a simple stored procedure with a pattern very close to what you demonstrated.

create procedure InjectionTest
(
    @BadCode nvarchar(max)
) as
    set @BadCode = REPLACE(@BadCode, '''', '')
    EXEC sp_executesql @BadCode

GO

Now from the front end I am going to pass in the value 0x730065006C0065006300740020002A002000660072006F006D0020007300790073002E00640061007400610062006100730065007300. Again, this is harmless binary here. If you want to see simply put that inside a convert(varchar(max),...

Here is an example of the above procedure being called. The binary string coming in is what the user would pass in. Notice there are no single quotes in that.

declare @Test nvarchar(max) = 0x730065006C0065006300740020002A002000660072006F006D0020007300790073002E00640061007400610062006100730065007300

exec InjectionTest @Test

There are plenty of much longer explanations and more trickery but this demonstrates the basics of how this can easily be broken.

Sean Lange
  • 33,028
  • 3
  • 25
  • 40
  • Yes. It should be a cardinal rule to never build up SQL from string concatenation in any system that will be exposed to user input. Always parameterize SQL. This has the added benefit of not polluting the plan cache, somewhat improving performance. – pmbAustin Apr 27 '17 at 16:55
  • I know that and I am already updating all of these. I was just curious if it is easily exploited because I couldnt think of a way. This also doesnt even remotely answer my question. – Marie Apr 27 '17 at 17:00
  • Your edit definitely proves the first example. It looks like it covers the second example, too, but I cant figure out how to convert a string to binary that works. running `SELECT convert(binary, '1'';union all select ''test''')` returns `0x31273B756E696F6E20616C6C2073656C6563742027746573742700000000` which comes up as `✱画楮湯愠汬猠汥捥⁴琧獥❴ `. How did you get that binary? – Marie Apr 27 '17 at 18:46
  • Use convert(varbinary(max), 'Your String Here') – Sean Lange Apr 27 '17 at 18:53
  • Strangely that didnt work for me, even converting the result from @Test gave a slightly different and shorter binary result. Besides that, It looks like the conversion happens immediately as the binary is assigned to @Test which means `REPLACE(@BadCode, '''','')` would still strip any apostrophes and protect against injection. – Marie Apr 27 '17 at 19:00
  • Well as I said this is barely the tip of the iceberg of what can happen. Yes this contrived example would actually make that work. Consider if they don't pass in a single quote, it is vulnerable to injection. Consider if instead of select * from sys.databases it was instead ";drop database master;--" No single quotes there and your master database goes bye bye. Up to you if you think you are safe. I am telling you that your code is not safe. – Sean Lange Apr 27 '17 at 19:33
  • @Sean I never said I thought I was safe. I said I need to come up with a way to prove that they are not safe so they will let me fix it. Passing in sql directly wont work because everything is concatenated with `'`s wrapping it and any `'` in the term replaced. Use the queries I posted in my question, they are safe from your examples so your examples don't help me at all. – Marie May 02 '17 at 17:51
  • OK then I give up. I really dislike the approach your code takes because it changes user input. The code presented is highly risky unless you modify the original data by removing all single quotes. I am pretty sure there are some other ways to break this that I can't think of. Given the nature of your question I suspect you actually realize this is dangerous or you wouldn't have asked in the first place. I really don't like systems that change my input because they are not written well enough to handle reasonable data. – Sean Lange May 02 '17 at 18:39