Does this code prevent SQL injection?

Question

Background

I've been contracted to analyze an existing Data Provider and I know the following code is faulty; but in order to point out how bad it is, I need to prove that it's susceptible to SQL injection.

Question

What "Key" parameter could break the PrepareString function and allow me to execute a DROP statement?

Code Snippet

Public Shared Function GetRecord(ByVal Key As String) As Record
    Dim Sql As New StringBuilder()

    With Sql
        .Append("SELECT * FROM TableName")
        If String.IsNullOrEmpty(Agency) Then
            .Append(" ORDER BY DateAdded")
        Else
            .Append(" WHERE Key = '")
            .Append(PrepareString(Key))
            .Append("'")
        End If
    End With

    Return ExecuteQuery(Sql.ToString())
End Function

Public Shared Function PrepareString(ByVal Value As String) As String
    Return Value.Replace("''", "'") _
                .Replace("'", "''") _
                .Replace("`", "''") _
                .Replace("´", "''") _
                .Replace("--", "")
End Function

Answer 1

In answer to your direct question: Does this code prevent SQL injection: No

Here's the proof - push this string through the PrepareString method:

Dim input = "'" & Chr(8) & "; Drop Table TableName; - " & Chr(8) & "-"
Dim output = PrepareString(input)

Console.WriteLine(input)
Console.WriteLine(output)

I modified the GetRecord method you posted to return the fully prepared SQL string rather than get the record from the database:

Console.WriteLine(GetRecord(output))

And this is the output

Input  = ; Drop Table TableName; --
Output = '; Drop Table TableName; --
Query  = SELECT * FROM TableName WHERE Key = ''; Drop Table TableName; --'

Add 1 extra line of code:

My.Computer.Clipboard.SetText(input)

And you've got the string you need copied right to your clipboard to paste into your input field on the website to complete your SQL injection:

'; Drop Table TableName; - -

[Noting that the control characters have been omitted from the post output by StackOverflow, so you'll have to follow the code example to create your output]

After the PrepareString method is run, it will have the exact same output - the Chr(8) ASCII code is the backspace which will remove the extra "'" that you're appending to mine which will close your string and then I'm free to add whatever I want on the end. Your PrepareString doesn't see my -- because I'm actually using - - with a backspace character to remove the space.

The resulting SQL code that you're building will then execute my Drop Table statement unhindered and promptly ignore the rest of your query.

The fun thing about this is that you can use non-printable characters to basically bypass any character check you can invent. So it's safest to use parameterized queries (which isn't what you asked, but is the best path to avoid this).

Answer 2

To answer your questionable question, no it wouldn't work.

.Replace("``", "''") would prevent legitimate queries with '`'

.Replace("´", "''") would prevent legitimate queries with '´'

.Replace("--", "") would prevent legitimate queries with '--' in them

.Replace("''", "'") would incorrectly modify legitimate queries with '''' in them

and so on.

Furthermore, the full set of escape characters can vary from one RDBMS to another. Parameterized queries FTW.

Answer 3

I think it's safe (at least in SQL server), and I also think the only thing you actually need to do is s = s.Replace("'", "''"). Of course you should use parameterized queries, but you already know that.

Answer 4

I think it is unhackable if you just replace ' with ''. I have heard that it is possible to change the escape quote character, which could potentially break this, however I am not sure. I think you are safe though.

Answer 5

This MSDN article covers most of the stuff you need to look out for (I'm afraid to say all when it comes to SQL injection).

But I will echo everyone else's sentiment of parameters parameters parameters.

As for your example some gotchas [Edit: Updated these]:

• wouldn't the string "1 OR 1=1" allow the user to get back everything

• or worse "1; drop table sometablename"

According to the article you want to check for:

; - Query delimiter.

' - Character data string delimiter.

-- - Comment delimiter.

/* ... / - Comment delimiters. Text between / and */ is not evaluated by the server.

xp_ - Used at the start of the name of catalog-extended stored procedures, such as xp_cmdshell.

Answer 6

You are trying to black list characters to implement your own version of SQL Escaping. I would suggest reviewing this URL - SQL escaping is not necessarily the worst choice (i.e. quickly fixing existing apps) but it needs to be done right to avoid vulnerabilities.

That URL links to another page for escaping in SQL Server where the author gives suggestions that help you avoid vulnerabilities without limiting functionality.

If it helps, the articles suggest escaping braces too (I call them square brackets - but []).

Answer 7

If you try to use your code some one could pass a key (;select * from table; and get a list of what ever table they want.

In your code your not checking for the semi-colon which lets you end a t-sql statement and start another one.

I would go with a parameterized query.