SQL Injection: Why is the following code dangerous?

Question

An intern has written the following vb.net code:

    Public Sub PopulateUsersByName (name As String)
        'Here, the parameter 'name' is unsantized, coming straight from the form

        Dim query As String = ""
        query += vbNewLine + " SELECT Id, FirstName, LastName FROM dbo.[Users]"
        query += vbNewLine + " WHERE FirstName LIKE '" + REPLACE(name, "'", "''") + "'"
        query += vbNewLine + "     OR LastName LIKE '" + REPLACE(name, "'", "''") + "'"

        'Execute SQLCommand with above query and no parameters
        'Then do some other stuff

    END Sub

I have explained that in general, one should never use string concatenation when trying to do something like the above. The best practice is to use either an SP, or an SQLClient.SQLCommand with parameters.

His logic is: any sql varchar(xxx) gets sanitized by replacing all single quotes with two single quotes (and adding additional single quotes at the start and end of the string).

I am unable to provide an example of something the user could type that would get around this - I'm hoping I can find something more convincing than "But, as a general principal, one should avoid this - you know... coz... well, don't argue with me - I'M THE BOSS AND MY WISH IS YOUR COMMAND.".

Note: The code will always connect to our Microsoft SQL Server. But I can imagine it failing to sanitize the input on some other SQL Implementation.

UPDATE:

Just to make it a little clearer, what I'm looking for is a possible value of the parameter name which will allow someone to inject SQL into the query.

Answer 1

Try this with his code using '%_%' (with and without the single quotes) as the input....same as this in SQL....

select SELECT Id, FirstName, LastName FROM dbo.[Users] from TBL_EMPLOYEES where FirstName like '%_%' or LastName like '%_%'

irrespective if it failing or not, his is very poor code... fair enough this one is only a one liner, but anything more than that, including complicated SQL statements would be difficult to maintain and debug.. in any case, using Sps gets him used to using them AND allows him to take advantage of the flexibility and power of T-SQL... LOL, i'd dread to think how some of the SQL I have written would look like in code.....

You know, I come across code like this (and a lot worse) all the time... just because it might work for a while DOESN'T mean it's the right way to do it.... If your intern that does NOT listen to experience he will NEVER make a good developer and that is sadly a fact

I once reduced a junior developers attempt at importing a CSV file (50 million rows) in the same way your intern has done, from her 300 lines of code (which was never going to work) to just one line of LINQ to convert it to XML (we couldn't use Bulk Insert or bcp) and a fancy SP... Was bullet proof, job done....