Proving SQL Injection

Question

I'm trying to simply prove here that this simple function isn't good enough to prevent every sql injection in the world:

Function CleanForSQL(ByVal input As String) As String
    Return input.Replace("'", "''")
End Function

Here is a typical insert statement from one of our apps:

Database.DBUpdate("UPDATE tblFilledForms SET Text1 = '" + CleanForSQL(txtNote.Text) + "' WHERE FilledFormID = " + DGVNotes.SelectedRows(0).Cells("FilledFormID").Value.ToString)

I know its not secure, because of googling and looking up other questions on StackOverflow.com. Here is one question that I found in which all functions such as the one I presented above are irrelevant and pointless.

So based on the post I linked to, simply typing

'Chr(8); update tblMaint SET Value1 = 2 WHERE ValueID = 2--

into txtNote should be enough to clear every value in text1 in the entire tblFilledForms table, and then update the tblmaint table's second row to be 2 correct?

What SHOULD happen here is that VB will interpret this as

UPDATE tblFilledForms SET Text1 = '''Chr(8); update tblMaint SET Value1 = 2 WHERE ValueID = 2--' WHERE FilledFormID = 5120327

and send it to SQL which will intern execute the Chr(8) to erase the third ' which would produce

UPDATE tblFilledForms SET Text1 = ''; update tblMaint SET Value1 = 2 WHERE ValueID = 2--' WHERE FilledFormID = 5120327

to be actually executed on the database correct?

I then coppied a Chr(8) from the clipboard and replaced the Chr(8) in the textbox with the clipboard contents and still a no-go. It puts the whole string directly into the field w/o problems.

So what am I doing wrong here? or what else can I do to break it?

Technologies and background: I'm using MS SQL Server 2005, and VB .NET 2005. the Text1 field in the database is a Varchar(600) field (don't ask my why its not MAX, its pointless, i know) There are certain triggers on the table that would prevent a mass update such as this and throw some errors if the injection actually worked right.

PS. I know parametrized queries are the way to go here and I'm not looking for answers like "well i dunno why it doesn't work, but parametrized queries are the way to go". I'm looking for the ability to prove that our software is broken and that we need to rewrite it using better principles.

To anyone reading this question to figure out how to better filter your text fields, the answer is DON'T! Use the parameters! they are much better, safer, and easier!

Answer 1

The Chr(8) is part of the quoted literal string, as is the update statement, so SQL Server is not going to interpret it as a function call. With this example, Text1 will be set to the literal value:

'Chr(8); update tblMaint SET Value1 = 2 WHERE ValueID = 2--

(yes, including that single quote)

So, with this example, your code is secure. Most hang-wringing over SQL injection is about accidentally failing to validate and quote values, there is nothing inherently unsafe in a properly-quoted SQL statement.

Answer 2

Your CleanForSQL method only handles string situations. What happens when you're not using a string but an INT instead? In that case, there would be no end tick to close with, so the injection would still happen. Consider this example...

Database.DBUpdate("UPDATE tblFilledForms SET Int1 = " + CleanForSQL(txtNote.Text) + " WHERE FilledFormID = " + DGVNotes.SelectedRows(0).Cells("FilledFormID").Value.ToString)

in that case, just entering the following will work...

0; update tblMaint SET Value1 = 2 WHERE ValueID = 2--

Answer 3

Scott Ivey has the classic case that can break it, the lack of quotes protecting a numeric input. (+1'ed that)

Depending on the language and where the string is being 'cleansed' and the database being used your immediate risk is that they language permits the string to be escaped. At that point the single quote you are trying to avoid getting thru goes wrong

\'; DROP yourTable;-- => \''; DROP yourTable;--

That goes into your sql string as

UPDATE tblFilledForms SET Text1 = '" + \''; DROP yourTable;-- + ' etc.

Which is then:

UPDATE tblFilledForms SET Text1 = '\''; DROP yourTable;-- ' etc.

'\'' is taken as the literal string of a single quote, if your database supports escaped characters - bingo your compromised.

Equally the protection has to be remembered to be effective, even the example update statement provided failed to protect the parameter in the where clause, was it because DGVNotes.SelectedRows(0).Cells("FilledFormID").Value.ToString) could never be entered by a user? will that hold true for the entire lifetime of the app etc?

Answer 4

You're not doing anything wrong. This is how SQL Server parses strings. The first quote opens the string, then you've followed that immediately with an escaped quote followed by Chr(8).

As an exercise, what happens if you run this in SQL Server: SELECT '''Hello'? Exactly the same parsing rules are being applied in this case.