I'm doing an audit of a system, which the developers insist is SQL injection proof. This they achieve by stripping out the single-quotes in the login form - but the code behind is not parameterized; it's still using literal SQL like so:
username = username.Replace("'", "");
var sql = "select * from user where username = '" + username + "'";
Is this really secure? Is there another way of inserting a single quote, perhaps by using an escape character? The DB in use is Oracle 10g.
So, no one can have a name like O'Brian in their system?
The single quote check won't help if the parameter is numeric - then 1; DROP TABLE user;-- would cause some trouble =)
I wonder how they handle dates...
If the mechanism for executing queries got smart like PHP, and limited queries to only ever run one query, then there shouldn't be an issue with injection attacks...
Maybe you can also fail them because not using bind variables will have a very negative impact on performance.
Have a look at the testing guide here: http://www.owasp.org/index.php/Main_Page That should give you more devious test scenarios, perhaps enough to prompt a reassessment of the SQL coding strategy :-)
A few tips:
1- It is not necessarily the ' character that can be used as a quote. Try this:
select q'#Oracle's quote operator#' from dual;
2- Another tip from "Innocent Code" book says: Don't massage invalid input to make it valid (by escaping or removing). Read the relevant section of the book for some very interesting examples. Summary of rules are here.
No, it is not secure. SQL injection doesn't require a single-quote character to succeed. You can use AND, OR, JOIN, etc to make it happen. For example, suppose a web application having a URL like this: http://www.example.com/news.php?id=100.
You can do many things if the ID parameter is not properly validated. For example, if its type is not checked, you could simply use this: ?id=100 AND INSERT INTO NEWS (id, ...) VALUES (...). The same is valid for JOIN, etc. I won't teach how to explore it because not all readers have good intentions like you appear to have. So, for those planning to use a simple REPLACE, be aware that this WILL NOT prevent an attack.
What is the client language ? That is, we'd have to be sure exactly what datatype of username is and what the Replace method does in regard to that datatype. Also how the actual concatenation works for that datatype. There may be some character set translation that would translate some quote-like character in UTF-8 to a "regular" quote.
For the very simple example you show it should just work, but the performance will be awful (as per Thilo's comment). You'd need to look at the options for cursor_sharing
For this SQL
select * from user where username = '[blah]'
As long as [blah] didn't include a single quote, it should be interpreted as single CHAR value. If the string was more than 4000 bytes, it would raise an error and I'd be interested to see how that was handled. Similarly an empty string or one consisting solely of single quotes. Control characters (end-of-file, for example) might also give it some issues but that might depend on whether they can be entered at the front-end.
For a username, it would be legitimate to limit the characterset to alphanumerics, and possibly a limited set of punctuation (dot and underscore perhaps). So if you did take a character filtering approach, I'd prefer to see a whitelist of acceptable characters rather than blacklisting single quotes, control characters etc.
In summary, as a general approach it is poor security, and negatively impacts performance. In particular cases, it might (probably?) won't expose any vulnerabilities. But you'd want to do a LOT of testing to be sure it doesn't.