.net - 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).