Tech Off Thread

9 posts

Forum Read Only

This forum has been made read only by the site admins. No new threads or comments can be added.

SQL Injection is bad. So how should I replace the code I was using?

Back to Forum: Tech Off
  • User profile image
    vandil

    I've got a program that allows you to choose some criteria to filter a list. So lets say that I've got the ability to go down a list and check off some things like age 18-21, student, income greater than $10,000, income less than $20,000. It's easy for me to associate each of these with a where clause, but you have to be careful because if someone puts an item in the list named
    '; drop table tblReallyImportant;
    or something like that, it can screw up your stuff.

    I'm using parameterized stored procedures for my selects on indexes, where I know exactly what I'm going to be filtering on ahead of time, but what is the recommended replacement for dynamic query generation in the situation where I've got a bunch of items, some of which could be used twice in a single query?

  • User profile image
    Maurits

    There are two seperate cases.

    For string data, you have to make sure that 's are correctly encoded.  Otherwise not only are you vulnerable to a SQL Injection attack, but it is possible for real data (legitimately containing a ') to be mis-handled.

    For numeric data, you have to make sure the data really is numeric.  There's no concern about legitimate data being mis-handled.

    The string case is easily handled by the following pseudocode:

    sql += "@Foo = '" + foo.replace("'", "''") + "'"; // encode ' as ''

    The numeric case is more difficult to handle but usually involves running the variable through a regular expression, casting it to a numeric type, or multiplying it by 1.

  • User profile image
    vandil

    I tried to do that to the best of my ability. I've actually got each of the options set up with a separate function for addition and removal from the query, so a lot of it can be handled there. It just seems that once I get everything locked down, I read about another way people are using to get around it.

  • User profile image
    TommyCarlier

    You can have safe dynamic SQL code, if you use parameters. Just build your SQL-statement dynamically, like you always do, and instead of appending/inserting the actual values, append parameters, like '@value1'. After you've built the command, pass in the values as parameters.

  • User profile image
    vandil

    I've had to change the way I generated the command, as I was only generating the where clause and inserting it in to the command string, but this seems to be working. I'd thought that parameters were only for stored procedures. Thanks for the heads up. Smiley

  • User profile image
    zzzzz

    TommyCarlier wrote:
    You can have safe dynamic SQL code, if you use parameters. Just build your SQL-statement dynamically, like you always do, and instead of appending/inserting the actual values, append parameters, like '@value1'. After you've built the command, pass in the values as parameters.


    Stupid question i don't think this stops people for building a nested select inside the where clause or appending in new fields in the select statement.

    What i have done is scan through the strings the users creates via options in the webform make sure thatno new fields that are not listed are not added and on the where clause makes sure it contains no SQL commands and enforce strict datatype and length of strings.

    its a time consuming process   What i have seen in SQL injection is UNIONS being used to get at more data or cause the server to crash which is another objective of SQL injection


  • User profile image
    TommyCarlier

    The main principle is: user input should not be used in the SQL-statement. Each user input field should become a parameter. You can generate SQL-statements dynamically, as long as you use parameters for the user input fields.

  • User profile image
    vandil

    The can build the statement to their hearts content, but it won't evaluate if you put it in a parameter. That's why something like

    Select * from tblTest Where ID in (1,2,3,4)

    works, but something like

    Select * from tblTest Where ID in (@IDLIST)

    doesn't work, even if you pass in the 1,2,3,4 thing. The value that you pass in to @IDLIST isn't treated as a list of numbers, it's treated as a string.

    I've seen some people do some very strange things to get something like this to work, such as passing in @IDLIST to a stored procedure that looks like this

    Select 'Select * from tblTest Where ID in (@IDLIST)' as query

    exec(query)

    which just opens you back up to SQL injection.

  • User profile image
    Maurits

    There are a couple of things you can do to protect yourself from injection in that case.

    Perhaps the simplest thing is to run the input against a regex... something like

    if (ids + ",") does not match ^(\d+,)+$
    {
        error!
    }

    sql = "... foo in (" + ids + ")..."

    Another way is to do that in sql with a stored procedure:

    CREATE Procedure Foo_Search
    (
        @IDs varchar(500) = '',
        @Bar varchar(500) = '',
        @Baz int = 0,
        ...
    )
    AS

    DECLARE @SQL varchar(8000)

    -- make sure @IDs is just numbers, commas, and spaces
    IF @IDS LIKE '%[^0-9, ]%'
    BEGIN
        RAISERROR('@IDs contains invalid characters', 11, 1)
        RETURN (1)
    END

    SELECT
        @SQL =
    'SELECT * FROM Foo WHERE 1 = 1 ' +
    CASE WHEN @IDS = '' THEN ''
        ELSE 'AND ID IN (' + @IDS + ') ' -- checked for injection above
    END +
    CASE WHEN @Bar = '' THEN ''
        ELSE 'And Bar = ''' + Replace(@Bar, '''', '''''') + ''' '
    END
    CASE WHEN @Baz = 0 THEN ''
        ELSE 'And Baz = ' + CAST(@Baz AS VARCHAR(20)) + ' '
    END

    EXEC(@SQL)

    RETURN (0)

    If you have a lot of data and a lot of columns, it can actually be faster to run the stored procedure by building the SQL as a string than it would be by letting SQL build an execution plan and reusing it.

    See Ian Jose's post on SQL's difficulty with building execution plans for stored procedures with broad scope.

Conversation locked

This conversation has been locked by the site admins. No new comments can be made.