5

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.

Omaer
  • 817
  • 1
  • 7
  • 22
  • 5
    See [this question/answer](http://security.stackexchange.com/a/37751) on security. Two great points - the top answer talks about SQL smuggling techniques. And one of the first comments was "why not do it properly instead of wondering whether or not it's exploitable?" – Damien_The_Unbeliever Feb 04 '14 at 14:56
  • @Damien_The_Unbeliever I went through the SQL Smuggling PDF, and couldn't find anything there that would apply to the question above. Even writing a \' doesn't cause the code to fail. As for "why not do it properly instead of wondering whether or not it's exploitable", the problem is that my intern is a hundred percent certain that it isn't exploitable. I'm the one who isn't as sure... I guess I'm going to have to resort to: "I'm the boss..." – Omaer Feb 04 '14 at 15:44
  • 3
    But the *concept* of SQL Smuggling exists. Even if that PDF doesn't include any forms that are known to work today, who knows whether a new form that *does* work against SQL Server and your database won't be found tomorrow? Or in 3 years time? Is your intern promising to return on zero days notice and work for free to identify and fix all problem locations when this new exploit is discovered? Parameters allow you to cleanly separate "things that should be considered to be code" and "things that should definitely only ever considered to be data" – Damien_The_Unbeliever Feb 04 '14 at 15:48
  • 3
    You might want to have a read of this answer: http://stackoverflow.com/a/15596300/2505574 – Obsidian Phoenix Feb 04 '14 at 16:01
  • @Damien_The_Unbeliever I think you misunderstood the question. I'm not looking for someone to convince me to follow the best practices - I already plan on getting the code changed. I was simply hoping to have a concrete example to go with my explanation of best practices. – Omaer Feb 04 '14 at 16:03
  • @ObsidianPhoenix Thanks - but in my case, the user input has a single quote added at the start and end - it isn't a generic function which _could_ be used in a different situation (such as: ` or \"). – Omaer Feb 04 '14 at 16:07
  • Maybe it doesn't on this query but there's the chance that he will forget on an other query. You'll need someone to check all queries to make sure they are ok before going into production. Make sure numbers (not just strings) are also ok. – the_lotus Feb 04 '14 at 16:48
  • 1
    I'm just waiting for an answer (from your intern) to popup saying "It isn't". – ganders Feb 04 '14 at 16:53
  • I'm wondering, why are you adding new lines? Kind of pointless.... Are you logging or displaying the query somehow? Using parameter (which you should) will add a bit of difficulty if you want to display the query. – the_lotus Feb 04 '14 at 16:57
  • 2
    Well, with my follow up I was hoping to make it clear - when SQL Injection was first discovered, *many* people thought that just doubling up quotes would be sufficient. Then one smuggling technique was found, and suddenly some people's "not exploitable" code suddenly was. Then another technique came along. Then another. But each one of these was a break where the fundamental problem had never been fixed (mixing code and data) and the proper solution has been long known. Is that not a good explanation? – Damien_The_Unbeliever Feb 04 '14 at 17:07
  • 1
    So gist of the question is "Can we supply a specific value of `name`" that will allow us to execute arbitrary SQL? Probably not in this case. Though you would need to inspect the relevant source code for the entire stack to be sure .NET framework/transport protocol/SQL Server. There are other reasons for insisting on parameterized queries though. As well as being more definitely safe they also make better use of the plan cache rather than filling it with single use adhoc queries. – Martin Smith Feb 04 '14 at 17:15
  • @the_lotus The new lines are just so that the code is more readable during debugging. It doesn't really hurt, and makes debugging a little quicker. I usually encourage using newlines - is there any downside I haven't thought of? – Omaer Feb 04 '14 at 18:01
  • 1
    @Damien_The_Unbeliever Thanks for all the info you've given me - it should certainly make my case stronger when pointing this out. I guess the gist of it is that in this case it's fine, but because I don't want to sit and double check his queries every time, as a rule he shouldn't do this. And I'm the boss so he better listen ;) – Omaer Feb 04 '14 at 18:03
  • @MartinSmith yes, that's right... That is the gist of what I was looking for. And the performance aspect is one I had completely overlooked... thanks for pointing that out. – Omaer Feb 04 '14 at 18:04
  • 1
    @ganders haha, it would be awkward if my intern sees this :D But on second thought, perhaps it's best if I do show him how people get peeved off at the idea of concatenating SQL! – Omaer Feb 04 '14 at 18:05
  • 1
    Yes Omaer SHOW him coz one day I MYSELF might have to work on his code and I don't wanna spend all darn day debugging code like that!!!! – Monty Feb 14 '16 at 23:06

3 Answers3

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

Monty
  • 1,534
  • 2
  • 10
  • 12
0

I can get a list of all your users. If name = %%

Now I know the full name of everyone in your database.

I would consider that a security hole.

JNK
  • 63,321
  • 15
  • 122
  • 138
  • 3
    That doesn't change with the use of parameters. – Heinzi Feb 04 '14 at 15:09
  • 1
    @Heinzi No it doesn't, but it's still an answer to "why is the following code dangerous?". This is a different issue in this implementation :) – JNK Feb 04 '14 at 15:10
  • @JNK How do you know the SUB will show you the result of the query? ;) – Omaer Feb 04 '14 at 15:26
0

Sanitizing is not the answer. You can by pass quotes and use "smuggling". A good example is http://danuxx.blogspot.com.br/2011/08/sql-injection-bypassing-single-quotes.html

Also a good practice (to use dynamic queries) is to use parametric dynamic queries. Also SPs can do the trick (if you don't use dynamic queries inside it off course).

jean
  • 4,159
  • 4
  • 31
  • 52