Prone To Injection Attack?

[18experience]

Recently, BrailleSchool posted this code

strSQL = "SELECT Un, Pw " & _
"FROM Auth " & _
"WHERE Un = '" & txtUsername.Text & "' " & _
"AND Pw = '" & txtPassword.Text & "'"

To which ShaggyHiker responded

That leaves your SQL open to a slightly obscure harrassing attack called something like an Insertion attack.

I understand what an Insertion Attack is, and it would appear that we are prone to them where I work, except for one thing; we use .Replace("'", "''") whenever (most of the time anyway) we insert user inputed text into a SQL statement. Like this:

strSQL = "SELECT Un, Pw " & _
"FROM Auth " & _
"WHERE Un = '" & txtUsername.Text.Replace("'", "''") & "' " & _
"AND Pw = '" & txtPassword.Text.Replace("'", "''") & "'"

So, my question is, can anyone demonstrate how they could still do an insertion attack with those .Replaces in there? I know creating and adding parameters is the safest way, but are we safe (enough) using our method?

[Asgorath]

Hi
I use stored procedure for everything...

[penagate]

It's a SQL injection attack and depending on your DBMS you may still be leaving yourself open when only replacing single quotes. You really should always use parameterised queries, not only for the security benefits but also because they make your queries easier to read and maintain and when coupled with prepared statements they also increase performance.

Using stored procedures can also be a good practice, depending on what you use them for, but it doesn't avoid injection issues since you can still use string concatenation to build a query that calls a stored procedure.

[18experience]

Penagate,
My reasoning is, if we replace "'" with "''" then anything they type in that textbox will be inserted as text. I tried to to do an injection once, and I couldn't get around that. No matter what I typed, no matter how many 's I typed, it just got inserted as one big block of text. Am I forgetting about something? Is there a way around that?

[penagate]

I guess not, but you'd still be better off using parameterised queries. It's just messy otherwise, and you are likely to forget to escape the quotes one time and then you are back to where you started.

[18experience]

Penagate,
That is absolutely true. Its not so bad for injection attacks in our case, but it also causes an error when the user uses 's.

[kleinma]

there are additional safe guards you can employ, which will be valid for stored proceedures or if you just call the SQL right from code.

For example, how long is a username? how long is a password?

If a user name can't be more then lets say 12 characters, then you should be limiting that input on entry (maxlength property of textbox) as well as validating it before its used in the query to trim it down to the appropriate size.

[Negative0]

Also you example is fine for text, but what about numbers, dates and times. If you don't validate all of the input before you put it in the string, you could get hit with an injection attack.

[GaryMazzone]

You should alway validdate any input going to a database for storage in any condition. If a DB field is expecting a number and the textbox that value is coming from contains anything other then an empty string (null value) or a valid number then you don't post anything to the DB and prompt the user to fix the problem. The same goes for date values. (I always use DateTimePickers for date values or time values).

[18experience]

Kleinma,
Thats another problem we have. You know, when you're testing, you just don't think the user would ever need more than twenty characters for a first name. I mean, how could they?

So, we forget to put maxlenghts on the textboxes, which causes the data to be truncated. It takes discipline to set forth standards, and then follow those standards. Or rather, it takes a lot of discipline to hunt down all the places you need to change if you do change your standards.

[Shaggy Hiker]

While it looks plausible, why would you come up with a solution as awkward as this? You can only maintain it by rigorously following a consistent protocol of using Replace, but if you are going to maintain a rigorous protocol, why not make it a protocol of parameters?

[kleinma]

Kleinma,
Thats another problem we have. You know, when you're testing, you just don't think the user would ever need more than twenty characters for a first name. I mean, how could they?

So, we forget to put maxlenghts on the textboxes, which causes the data to be truncated. It takes discipline to set forth standards, and then follow those standards. Or rather, it takes a lot of discipline to hunt down all the places you need to change if you do change your standards.

if nothing else, put it on your ToDo list for when you check your tab orders on your forms to make sure they are correct. Since you are basically looking at EVERY control when you set your taborders to be correct, that is also a good time to check other properties, like maxlength to make sure they match your database schema.

[penagate]

Kleinma,
Thats another problem we have. You know, when you're testing, you just don't think the user would ever need more than twenty characters for a first name. I mean, how could they?

So, we forget to put maxlenghts on the textboxes, which causes the data to be truncated. It takes discipline to set forth standards, and then follow those standards. Or rather, it takes a lot of discipline to hunt down all the places you need to change if you do change your standards.

Maybe it's the web developer in me talking, but I don't rely on things like maximum length properties. They are good UI hints, but I don't treat them as validation.

Of course, in a desktop app, there is probably no way to get around such limitations, and so you could treat them as validation. However, I consider it a bad practice not to check it in the code anyway.

[kleinma]

Even in a desktop app, I wouldn't make it the final barrier between the app and the database, but setting these properties does help as a front line.