12

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.

Mr Lister
  • 45,515
  • 15
  • 108
  • 150
  • 8
    If the industry accepted best practice is to use parameterized queries, what, specifically, is the reason you're not using them? – taylonr Dec 14 '11 at 15:04
  • 2
    This code was written by somebody else, like I said. And I am to add some functionality to it. If I also change the rest of the program in dozens of places, I've got a lot of testing to do before I'm certain it behaves the same it does now. So I'm not sure it's worth the effort, if there are no dangers as it is now. – Mr Lister Dec 14 '11 at 15:09
  • 2
    You have to evaluate tradeoff of testing vs someone stealing/destroying your data. – taylonr Dec 14 '11 at 16:21
  • 1
    Not all SQL statements are parameterizable, such as `set role "a@b.com"` or `listen channel1`, where "a@b.com" or "channel" is something you might want to let a user set – Neil McGuigan Apr 25 '15 at 07:06

5 Answers5

16

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

Community
  • 1
  • 1
David
  • 72,686
  • 18
  • 132
  • 173
  • 1
    I particularly like this post: http://codeinsecurity.wordpress.com/2011/12/09/mysql-php-everyone-doing-it-wrong/ – Polynomial Dec 14 '11 at 15:02
  • @Polynomial: that post doesn't apply to this query. Here, the user input cannot escape being inside a string. – Andomar Dec 14 '11 at 15:05
  • 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. – Mike M. Dec 14 '11 at 15:10
  • @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. – David Dec 14 '11 at 15:16
  • @DavidStratton Completely agree! – Mike M. Dec 14 '11 at 15:18
  • If you are refactoring anyway, why not go to the best practice at the same time? – HLGEM Dec 14 '11 at 16:31
  • 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 Dec 14 '11 at 18:49
  • 8
    Please give some simple reasons/examples why it is not good enough instead of giving alternative advices.. – Orhun Alp Oral Oct 03 '13 at 18:50
  • 2
    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. – Michael Z. Jun 14 '16 at 22:08
4

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.

Andomar
  • 232,371
  • 49
  • 380
  • 404
  • 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. – SQL Police Nov 24 '15 at 20:59
  • @SQLPolice: For example, `declare @sql nvarchar(max); select @sql = 'select ''' + Field + '''' from yourtable; exec (@sql);` – Andomar Nov 25 '15 at 06:22
  • but the OP is escaping all single quotes. See the `replace`statement. The sql string becomes a normal string then, it will not be seen as executable code. – SQL Police Nov 25 '15 at 10:30
  • Your first statement: `Yes, .Replace("'", "''") stops SQL injection to the same degree that parameterization does.` is completely incorrect, even when it was written. – BlackICE Dec 20 '18 at 16:49
  • @BlackICE can you provide a counter example, I am really interested in to convince management to do a refactoring on legacy code written in that way – Skary Feb 15 '23 at 08:26
  • @Skary see https://stackoverflow.com/a/8506593/264607, the `SQL Injection after removing all single-quotes and dash-characters` link – BlackICE Feb 21 '23 at 04:48
  • @Skary this is also good reading https://security.stackexchange.com/questions/3611/sql-injection-why-isnt-escape-quotes-safe-anymore/3633#3633 – BlackICE Feb 21 '23 at 04:54
  • @BlackICE i have tried to write a PoC, but even after reading those articles i can't find a way to make the injection work. Seems like that SQL server accept only hex(27) as string delimiter, and if the delimiter is doubled then is escaped. Also seems no to coalasce other charset to hex(27). May i try corrupted utf8 to trick SQL Server? My code is wirtten in C# and look like "SELECT * FROM tbl WHERE column = N'" + input.Replace("'","''") + "'"; – Skary Feb 21 '23 at 10:23
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.

paparazzo
  • 44,497
  • 23
  • 105
  • 176
  • Good one, but in this case not applicable I'm afraid. I can keep it in mind for any next projects though. – Mr Lister Dec 14 '11 at 18:50
1

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.

Paddy
  • 33,309
  • 15
  • 79
  • 114
0

Use a procedure.

Convert the statement to static SQL, placing the value of parameter into a local variable.

This does help !

StackFlowed
  • 6,664
  • 1
  • 29
  • 45