3

I'm reviewing some old code written by other devs and I stumbled upon the following code on login page:

...
var sql = String.Format("SELECT * FROM Users WHERE Username = '{0}'", txtUsername.Text.Replace("'", "''"));
var command = new SqlCommand(connection, sql);
...

At first I thought it is vulnerable to SQL Injection, but after some testing I couldn't break it. Is just replacing quotes enough to prevent SQL Injection in this case?

I am not asking for a best practice. I'm trying to enter something in txtUsername to inject SQL and prove that it is not enough.

Delphi.Boy
  • 1,199
  • 4
  • 17
  • 38
  • http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php – Wobbles May 17 '16 at 17:27
  • No it is not. Best practice is to use [parameterized queries](http://www.dreamincode.net/forums/topic/268104-the-right-way-to-query-a-database-parameterizing-your-sql-queries/). – Lews Therin May 17 '16 at 17:29
  • 2
    @AlexGravely OP isnt asking about how prevent sql injection, he is asking if quotes replacement is a good method. – Juan Carlos Oropeza May 17 '16 at 17:30
  • @Sylverac OP isnt asking about how prevent sql injection, he is asking if quotes replacement is a good method. – Juan Carlos Oropeza May 17 '16 at 17:30
  • Yeah, except when parameterized queries are too limiting. This is a valid question. – TomTom May 17 '16 at 17:30
  • @AlexGravely He is literally asking "Is just replacing quotes enough to prevent SQL Injection in this case?" The answer is no, that is not enough. If it were, this would be the recommended best practice instead of SQL parameters. – Lews Therin May 17 '16 at 17:34
  • @TomTom That's not what the OP asked. See my response to AlexGravely above. – Lews Therin May 17 '16 at 17:35
  • @JuanCarlosOropeza There we go, great find. Answers OP's question exactly. – Lews Therin May 17 '16 at 17:37
  • 1
    @Sylverac Funny thing I just put title question on google and was first suggestion – Juan Carlos Oropeza May 17 '16 at 17:40
  • Thanks everybody, but I'm not looking for a best practice. I already know that. I want to convince people that it is not enough and to convince them I need to enter something in the textbox that breaks the system. Edited the question as well. Please remove duplicate mark. – Delphi.Boy May 17 '16 at 17:43
  • @Eser Just send a terminator to find him. – Juan Carlos Oropeza May 17 '16 at 17:45
  • @Eser It will be `Username = 'O''Connor'` which is correct SQL. – Delphi.Boy May 17 '16 at 17:45
  • The way to break this depends on your platform and localization on that platform. In UTF16 there are many ways to specify a quote that most major platforms will support. – Hogan May 17 '16 at 18:28
  • @Hogan It is C# and SQL Server on Windows. – Delphi.Boy May 17 '16 at 18:46
  • ok are you filtering U+2018 and U+2019? – Hogan May 17 '16 at 20:31
  • @Hogan You are seeing the entire code. Just replacing ' with 2 quotes. – Delphi.Boy May 17 '16 at 20:36
  • you also have to filter U+0027 -- like others said, user parameter queries. – Hogan May 17 '16 at 20:38
  • @Hogan As I mentioned in the question and the comments, I'm not going to fix it. I'm going to show that it can be broken. How can I use `U+0027` in the query to inject SQL it? – Delphi.Boy May 17 '16 at 20:47
  • @Delphi.Boy - yeah I'm not going to bother. I have a consulting firm you are welcome to hire me, or you could look at MS's and pass a U+0027 to your function and see it not work, as you wish. – Hogan May 18 '16 at 02:45

2 Answers2

3

Generally speaking, yes*.

Using proper escaping of input is a recommendation from OWASP as a valid approach to avoiding SQL Injection and while it may handle all of your needs, it's worth listing their preferences to avoid SQL Injection attacks :

  • Option 1: Using Parameterized Queries
  • Option 2: Using Stored Procedures
  • Option 3: Escaping All User Supplied Input (Your Approach)

So as you can see, properly escaping the input falls third on the list, but for all intents and purposes, it should be enough to avoid SQL Injection. I'm not a big advocate of the approach personally, but that's just my opinion.

Recommendation: Use Parameterization When You Can

While using quotes can help, you should consider using proper parameterization to take advantage of some of the built-in protection that the .NET framework provides to check that the parameters are of the proper type, etc. if it is an option :

// Define a parameter in your query using the @parameter format (or ? in OleDbConnections)
var sql ="SELECT * FROM Users WHERE Username = @username";
using(var command = new SqlCommand(connection, sql))
{
     // Ensure your connection is open and other code here...

     // Add your parameter
     command.Parameters.AddWithValue("@username",txtUsername.Text);

     // Any other logic here...

     // Execute your query
     using(var reader = command.ExecuteReader())
     {
          // Do your thing...
     }
}

When Parameterization Is Not An Option

As TomTom mentions, there are some scenarios when parameterization might be possible or practical due to implementation (i.e. too many parameters required) or performance limitations. In these scenarios, OWASP recommends the use of stored procedures or your current technique of escaping input, which implies you can rely on using quotes, but just be careful and consider sanitizing your input as much as possible.

Community
  • 1
  • 1
Rion Williams
  • 74,820
  • 37
  • 200
  • 327
  • 1
    No, he is asking a generic question. There are plenty of reasons to generate full SQL - for example writing a SQL generator for an ORM, writing a highly efficient bulk insert generator (where you run out of the 1000 parameters per batch limit fast). As per site rule he submitted a minimal example - as such it should be taken. – TomTom May 17 '16 at 17:29
  • Thanks TomTom, I'll clarify with your suggestion as well. – Rion Williams May 17 '16 at 17:32
  • This is not the answer to my question. – Delphi.Boy May 17 '16 at 17:46
  • I've provided just about all of the information (and sources) pertaining to your question and added some additional information to clarify. You current approach should be enough to avoid SQL Injection (as it would fall under OWASP's third recommendation). A question like this is worth including recommendations on how a scenario like this would best be handled, which is why I included in the information on parameterization, etc. – Rion Williams May 17 '16 at 17:53
  • I will humbly disagree with OWASP that escaping input is good enough. It will work on strings but it doesn't prevent against binary insertion. This page has an example of binary injection working. https://www.simple-talk.com/sql/database-administration/sql-injection-how-it-works-and-how-to-thwart-it/ – Sean Lange May 17 '16 at 18:13
  • @SeanLange I'm not a big fan of their recommendation or the approach either. I haven't personally ran into a use case where parameterization wasn't an option and if I did, I would probably still be inclined to use a stored procedure instead. I know the folks at OWASP generally know what they are talking about with regards to security, but there's a reason I put an asterisk next to "yes". – Rion Williams May 17 '16 at 18:17
  • Of course. I was not suggesting there is anything wrong with your answer. I was arguing that their assessment of token replacement is good enough. I too have never seen a situation where parameters wouldn't suffice and I always use a stored procedure over pass through queries. They are just so much easier to maintain. :) – Sean Lange May 17 '16 at 18:19
-1

Typically, the best option is to practice the use of parameterized queries. That's a solid first line of defense for SQL Injection.

This blog on the MSDN website will probably be a good read, regarding to your question.

https://blogs.msdn.microsoft.com/sqlphp/2008/09/30/how-and-why-to-use-parameterized-queries/

iDillon
  • 122
  • 11
  • 1
    Not an answer. There are plenty of reasons to generate full SQL - for example writing a SQL generator for an ORM, writing a highly efficient bulk insert generator (where you run out of the 1000 parameters per batch limit fast). As per site rule he submitted a minimal example - as such it should be taken – TomTom May 17 '16 at 17:29
  • This is not the answer to my question. – Delphi.Boy May 17 '16 at 17:46