5

I inherited an old piece of software and the code checks user input for containing a single quote character ' before construction an SQL statement using the string concatenation.

Is this sufficient to avoid SQL injection (besides being bad style) or do I have to take immediate action and change it to parameter usage?

okrumnow
  • 2,346
  • 23
  • 39
  • 1
    It is insufficient and you need to take immediate action and change it to parameter usage. – Danny Beckett Jul 01 '13 at 11:31
  • http://www.unixwiz.net/techtips/sql-injection.html – Daniel Hilgarth Jul 01 '13 at 11:33
  • 2
    Depends on what type of fields it's putting them into. In general, you can't be certain you're safe with just that. Depending on the specific SQL, you might be lucky, but you'd have to check that thoroughly. – Stobor Jul 01 '13 at 11:34
  • http://stackoverflow.com/questions/139199/can-i-protect-against-sql-injection-by-escaping-single-quote-and-surrounding-use - I hope it's well explained – Kamil Budziewski Jul 01 '13 at 11:34
  • Whoa. Never thought such a question could cause a discussion. I am kind of jealous of C# youthfulness and naivety :) – Your Common Sense Jul 01 '13 at 11:41
  • Thanks for all the insight. I will asap check for sql data types that are not strings, as I understood that those can still be attacked without a quote in the input. – okrumnow Jul 01 '13 at 12:09

3 Answers3

7

Nope, it is not enough.

Yes, you have to take immediate action and change it to parameter usage where applicable.

Just a few guidelines for you to get it straight:

  1. Never take care of any injections. But make sure you have formatted your SQL literals properly. A properly formatted literal is error-proof and - just as a side effect - also invulnerable.
  2. Discover the fact that SQL query consists of literals of several different types. Each require distinct formatting, incompatible and useless for all others.
  3. Make sure such a formatting applied unconditionally. A prepared statement is the only way to be sure of.
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
4

Using the blacklist approach (i.e. block single quotes) may help prevent specific cases but it's generally better to use parameterized queries because it'll be difficult to blacklist all vectors.

SQL injection is still possible without quotes as described here so whilst blocking a single quote may prevent the 'easy' SQL injection, it's not foolproof. (And that example is one that's been made public but there could be other less well-known exploits being used).

Community
  • 1
  • 1
keyboardP
  • 68,824
  • 13
  • 156
  • 205
  • You're linking to a dynamic SQL attack. Using parameters will not prevent a dynamic SQL attack. – Andomar Jul 01 '13 at 11:44
  • Fair comment but the idea is to reduce the number of vectors available to hackers. The point is that a single quote is not the only type of SQL injection, it's just the easiest/most common one. The quote could be escaped by the attacker or alternative characters used (I'm not an expert on the other various types of attacked but double quotes and pipes can also be used [according to this post](http://markettorrent.com/topic/2969)). – keyboardP Jul 01 '13 at 11:56
-3

Yes, in SQL Server, filtering ' or replacing ' with '' stops SQL injection.

As Daniel Hilgarth mentions in the comments, you should not pass a C# string as a SQL integer.

(I'm aware this is not a popular position, but as far as I know, it's correct.)

Andomar
  • 232,371
  • 49
  • 380
  • 404
  • Can the downvoters explain why this is incorrect? My guess is that it can't be assumed to be correct for all flavours of SQL. – Rob Lyndon Jul 01 '13 at 11:35
  • 4
    This is not correct. Check [this link](http://www.unixwiz.net/techtips/sql-injection.html). Sections "Sanitize the input" and "Escape/Quotesafe the input". – Daniel Hilgarth Jul 01 '13 at 11:35
  • @DanielHilgarth: The question is labeled SQL Server. – Andomar Jul 01 '13 at 11:36
  • 1
    @Andomar: So what? Have you actually looked at the sections I mention? They apply to SQL Server just as well. And BTW: The database used in the article *is* SQL Server. – Daniel Hilgarth Jul 01 '13 at 11:37
  • @DanielHilgarth And that section says that escaping quotes it difficult, but *removing* all quotes from the input should be trivial. Nevertheless, doing so blocks you from inputting quotes at all, which I think is not desirable. Also parameters have other advantages, like type support (i.e. not worrying about date format) and speed. Nevertheless, I think strictly speaking this answer might be correct, although there are probably exceptions I don't know of. Anyway, I wouldn't take the risk. – GolezTrol Jul 01 '13 at 11:39
  • 2
    @GolezTrol: Nope, this answer is not correct. Depending on the query - and that is relevant, because the OP didn't show a specific query! - you can perform SQL injection *without* entering quotes. – Daniel Hilgarth Jul 01 '13 at 11:41
  • @DanielHilgarth: The second section you mentions applies only to MySQL. The first only applies if your code mixes up integers and strings. – Andomar Jul 01 '13 at 11:47
  • @Andomar: This doesn't make your answer anymore correct. Your answer *unconditionally* states, that escaping quotes is enough to prevent SQL injection. It is not, the link proves this. An answer like this from a user with as much rep as you have is outright dangerous, because others researching this issue will give it a lot of weight. – Daniel Hilgarth Jul 01 '13 at 11:51
  • Apart from all the vulnerabilities this "protection" measure being source of, the only question I ask from its advocates: What if this very site were using it? The OP were unable to post their question at all. So, it's neither safe nor viable. – Your Common Sense Jul 01 '13 at 11:55
  • @DanielHilgarth: I've added your condition to the answer. There is no need to panic if your legacy app does proper `'` replacement. – Andomar Jul 01 '13 at 11:58