SQL injection: isn't replace("'", "''") good enough?

Question

While I can certainly see the advantages of using parameters for SQL queries, especially when dealing with datetimes and things like that, I'm still unsure about parameters as the only way to prevent SQL injection.
The fact is, I inherited an application and it has things like

"SELECT Field FROM Table WHERE Filter='"+userinput.Replace("'", "''")+"'"

all over the place. Now while those doesn't look very pleasant to my eyes, and I wouldn't mind rewriting them, my question is, do I need to? Try as I might, I can't see a way to perform SQL injection with this.

Solution 1

No, it is not enough. It will do in a pinch, but it is a very weak alternative, and using parameterized queries or parameterized stored procedures is better, if your platform and/or RDBMS support either feature.

From

OWASP's SQL Injection Prevention Cheat Sheet

...this methodology is frail compared to using parameterized queries. This technique should only be used, with caution, to retrofit legacy code in a cost effective way.

There are more below

SQL injection — but why isn't escape quotes safe anymore?

Sql Injection Myths and Fallacies

SQL Injection after removing all single-quotes and dash-characters

Solution 2

Yes, .Replace("'", "''") stops SQL injection to the same degree that parameterization does.

There is still double or reflective injection. For example, you can store

'; delete from orders'

in a comment field. If part of the database uses the comment field in dynamic SQL, it might run the delete instead.

Solution 3

If the user only needs read only access to the data then have the UI execute via a SQL user that only has read only access. Read only does not protect you from injection attacks - they can use it to view data you did not intend them to view but they cannot use injection to delete data.

Solution 4

I think you're getting an answer on the way as to why it isn't enough, but you also run into the problem of somebody forgetting to do a replace on a string. If you 'always' use parameters, this is less of an issue.

View more solutions

Comments

[Polynomial]

I particularly like this post: codeinsecurity.wordpress.com/2011/12/09/…

[Andomar]

@Polynomial: that post doesn't apply to this query. Here, the user input cannot escape being inside a string.

[Mike M.]

I was going to argue against your original post which was incredibly against this method, but it seems doing some research has brought you a more reasonable viewpoint :). Sure, It's not the best method, but if it's in place, I don't see a total re-write of all data access being necessary unless you find yourself with the time.

[David]

@Mike M. - Oh, I'm still incredibly against it if you can reasonably use a better method. I just took the time to re-word my answer to a less "extreme" statement as I found the links I was looking for. I agree that re-writing an entire application to get rid of escaped characters is probably not justified, but fixing them while you're in that section of code would be worth it to me. I have an unfortunate tendency to make extreme statements when I'm in shoot-from-the-hip mode.

[Mike M.]

@DavidStratton Completely agree!

[dburges]

If you are refactoring anyway, why not go to the best practice at the same time?

[paparazzo]

I agree. You only need to miss a string replace in one spot and it is game over.

[Mr Lister]

Thanks all. All valuable, if somewhat contradictory, info. I know what I should do now, and if I get the time, I certainly will!

[Mr Lister]

Good one, but in this case not applicable I'm afraid. I can keep it in mind for any next projects though.

[Orhun Alp Oral]

Please give some simple reasons/examples why it is not good enough instead of giving alternative advices..

[SQL Police]

I don't understand that answer. How could your input example be an exploit for the OP's question ? He is escaping the single quote, so I sese no chance.

[Andomar]

@SQLPolice: For example, declare @sql nvarchar(max); select @sql = 'select ''' + Field + '''' from yourtable; exec (@sql);

[SQL Police]

but the OP is escaping all single quotes. See the replacestatement. The sql string becomes a normal string then, it will not be seen as executable code.

[Michael Z.]

Actually, if you know your database and your code will only talk to that database system and you only escape when you need to write to the database then it works perfectly fine and no other string manipulation is required to eliminate SQL Injection. It is good practice to use prepared statements so that your code can work across different database systems.

[BlackICE]

Your first statement: Yes, .Replace("'", "''") stops SQL injection to the same degree that parameterization does. is completely incorrect, even when it was written.