6

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!

11
  • 5
    Downvote for threatening people and writing principals instead of principles in the same paragraph.
    – Vinko Vrsalovic
    Dec 2, 2009 at 17:20
  • Well your Chr(8) ends up inside a string so that's why SQL is not evaluating it. Dec 2, 2009 at 17:21
  • 6
    @Vinko I don't think thats fair, he was simply setting a guideline for answers. So instead of getting a whole bunch of answers all saying the same thing, he gets 1 or 2 that tell him what he wants.
    – Zoidberg
    Dec 2, 2009 at 17:22
  • 2
    Are you only trying to prove holes in the method for string fields? What about numeric values that don't require quoting at all? The general method of cobbling together an SQL string falls down pretty hard there. Dec 2, 2009 at 17:23
  • @ Vinko Vrsalovic fine, that part is removed, but I still don't want to hear any lectures about how I'm doing it wrong.
    – Jrud
    Dec 2, 2009 at 17:26

5 Answers 5

6

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.

1
  • +1'd because your answer may be shaping up to be the final answer for this question.
    – Jrud
    Dec 2, 2009 at 22:37
4

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--

6
  • We do not allow users to type any numbers into text fields that are not first checked for being a numeric first. I'm sure this would break if it were a numeric field, but we don't use this method for that. It is strictly for strings.
    – Jrud
    Dec 2, 2009 at 17:31
  • 4
    Don't forget that it could be ID numbers or something in query strings too - not just textboxes. Most of the injection i've come across has been unchecked variables in the query string, and not in form fields.
    – Scott Ivey
    Dec 2, 2009 at 17:38
  • 2
    +1 to Query Strings -- And don't forget that anyone could create a fake form to post to your page. I don't know how DGVNotes.SelectedRows(0).Cells("FilledFormID") is getting set, but if it's coming from the browser AT ALL, then SQL can be injected. I know you always check your numeric fields, but if you ever forget, it opens a hole. Dec 2, 2009 at 17:59
  • whoever downvoted me here - please explain so that we can all learn something from your knowledge?
    – Scott Ivey
    Dec 2, 2009 at 19:24
  • @Sean McMillan I can see what you are saying about posting, however this is a standard winforms app, and through proper naming conventions DGV stands for a DataGridView (but the value could still be changed with some external tools from what I've seen other people say in this thread) @Scott Ivey we do not let our users type in their ID numbers directly. The value is actually sourcing from a hidden ID field in a datagrid view, though I could see this as a problem if we were allowing input. Is there some other way of the user getting to my ID number if they cannot edit the datagrid view field?
    – Jrud
    Dec 2, 2009 at 22:22
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?

2
  • Probably just the DB technology I'm using... MS SQL Server sees \' to mean that you actually wanted to type \' its not a literal or escape character. For SQL Server, two single quotes together is interpreted as a literal single quote making any '' that gets passed into it a literal ' within the string and it will not be executed.
    – Jrud
    Dec 2, 2009 at 22:35
  • Yes, SQL only escapes certain things and uses [ as the escape, MySQL uses \ I believe so runs into this problem.
    – Andrew
    Dec 2, 2009 at 22:56
1

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.

3
  • yep, that is exactly what is going on, I understand. But I still need to break it somehow...
    – Jrud
    Dec 2, 2009 at 17:24
  • You don't need to break it - you just need to prove that your technique works for all cases, or not.
    – Callie J
    Dec 2, 2009 at 17:26
  • yes, if it works for all cases, that does sort-of disprove by elimination.
    – Jrud
    Dec 2, 2009 at 17:29
0

I think your problem is that Chr(8) is not executed, you need to find another way to get the leading quote mark in.

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.