http://www.perlmonks.org?node_id=603188

Win has asked for the wisdom of the Perl Monks concerning the following question:

I am executing MS SQL Server SPROCSs through a Perl program. The critical piece of code that sets up the execute statement follows:
$Command = join(' ', 'EXEC', $SPROC, join(', ', @CHOICE[1 .. $elements_in_array])) . '';
I really want advice on the best way of preventing a malicious injection attack or some other attack. I guess that it might be an idea to limit the SPROCs that can be called. It might be an idea to make it impossible to activate any SPROC that is a system SPROC. That would require screening of the $SPROC variable. Should I exclude the possibility of @CHOICE containing a variable that has DELETE in it. Or a variable that has ‘;’ in it. Is there anything else that I should do?

Replies are listed 'Best First'.
Re: Preventing malicious T-SQL injection attacks
by bart (Canon) on Mar 05, 2007 at 11:18 UTC
    I think you ought to be able to use placeholders for the parameters. Thus, you provide just as many question marks as there are parameters (BTW don't the parameters start at index 0?), and pass the actual parameter in the do call.
    $Command = "EXEC $SPROC " . join ', ', ('?') x $elements_in_array;
    This will produce something that looks like
    EXEC FOO ?, ?, ?
    (I have no idea if this is the proper syntax for calling stored procedures in T-SQL — perhaps it needs parens?)
    which later you call through
    $dbh->do($Command, undef, @CHOICE[1 .. $elements_in_array])
    (The undef comes in place of the \%attr in the docs.)

    That ought to remove all possible problems related to dangerous values in ther parameters, as they're all treated as content of strings.

    And yes, you should check if $PROC looks right, like a proper procedure name, for example with a regex.

    A reply falls below the community's threshold of quality. You may see it by logging in.
Re: Preventing malicious T-SQL injection attacks
by davorg (Chancellor) on Mar 05, 2007 at 12:54 UTC

    I second the idea of whitelisting the acceptable values for $SPROC and also using placeholders to insert the elements of @CHOICE into the SQL.

    Combining the two ideas, might give something like this:

    # %procs contains the names of the valid stored procs # together with the number of parameters each requires my %procs = ( proc1 => 2, proc2 => 0, proc3 => 1, # ... ); unless (exists $procs{$SPROC}) { die "Unknown stored proc: $SPROC\n"; } my $sql = "EXEC $SPROC ". join ', ', ('?') x $procs{$SPROC}; my $sth = $dbh->prepare($sql); $sth->execute(@CHOICE);

    This code also has the advantage of dieing if the number of elements in @CHOICE doesn't match the expected number of parameters.

    A reply falls below the community's threshold of quality. You may see it by logging in.
    A reply falls below the community's threshold of quality. You may see it by logging in.
    A reply falls below the community's threshold of quality. You may see it by logging in.
Re: Preventing malicious T-SQL injection attacks
by jonadab (Parson) on Mar 05, 2007 at 12:35 UTC
    I guess that it might be an idea to limit the SPROCs that can be called. It might be an idea to make it impossible to activate any SPROC that is a system SPROC. That would require screening of the $SPROC variable.

    Whitelist. Make a list of all the acceptable values of $SPROC, and don't execute anything that's not on the list. In information security, it is always far, far better to use a whitelist than a blacklist. Anything not expressly allowed is automatically verboten. Don't put anything on the list that doesn't actually need to be there.

    Plus do what bart suggests above, with the placeholders. I was going to say that, but he beat me to it. It's good advice.

    -- 
    We're working on a six-year set of freely redistributable Vacation Bible School materials.
Re: Preventing malicious T-SQL injection attacks
by Moron (Curate) on Mar 05, 2007 at 13:07 UTC
    The traditional solution, both for MS SQL Server and Sybase is to grant the ordinary database user execute privilege - but nothing else! Then all insert/update/delete/select can only be performed by executing procs written by the privileged users. It means writing four access procedures per logical table, but these can be templated and generated from Perl.

    -M

    Free your mind

      We do as Moron++ prescribes but it's easier said than done. You need discipline to only grant execute access to SQL or Windows logons for the s-procs (direct base-table or view access is not allowed) and write your s-proc code accordingly. Your security model can easily get botched by folks that don't understand the details and grant db_datareader/db_datawriter in a troubleshooting panic and don't understand the long term repercussions of such an action.
        Yes, for example, it means programmers can't go issuing any SQL they like from the client programs but have to put all SQL in stored procedures and execute it with parameters determined at the client end. One source of this strategy was back in the heyday of C++ in the 1990s when programmers (with usually more C knowledge than database knowledge) were apt to embed all SQL in their client programs which then tended to cause objects to deadlock each other and even themselves in some cases and also to prevent unecessary communication across the network. Rather than have a client program sending SQL requests results and getting sometimes huge result sets over the net and then processing them, it is more efficient overall to let the client program handle the client functionality and stored procedures handle the server functionality - but there are many people out there who want to be able to code everything from the only language they know and that is a tendency especially we as Perl programmers with all our many modules also need to avoid sometimes.

        -M

        Free your mind

Re: Preventing malicious T-SQL injection attacks
by Trizor (Pilgrim) on Mar 05, 2007 at 12:32 UTC

    It isn't clear if @CHOICE comes from you or the user. If it comes from you the issue is paranoid versus pragmatic: sure someone could have found a way to malicously modify that variable, but is it worth the extra effort here to make sure its safe? Or would it be more worth your time to find the holes that could lead to the modification.

    Of course this goes out the window if @CHOICE isn't your creation, in which case I'd reccomend using a prepared statement to check syntax before execution. If the create fails, then its likely that an injection attack was attempted and you can log or take necessary action.

    eval { $dbh->prepare($Command); } if ($@) { # Those jerks tried to inject us... }