Beefy Boxes and Bandwidth Generously Provided by pair Networks
Perl: the Markov chain saw
 
PerlMonks  

Preventing malicious T-SQL injection attacks

by Win (Novice)
on Mar 05, 2007 at 10:24 UTC ( #603188=perlquestion: print w/ replies, xml ) Need Help??
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?

Comment on Preventing malicious T-SQL injection attacks
Download Code
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.

      Does the following look like a sensible precaution? My computer is tied up at the moment. Can't test it.
      for my $chosen (@CHOICE) { if ($_ =~ /\-\-/ || $_ =~ /\;/){ print "There is a possible injection attack here"; ## Security fun +ction here die; }
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... }
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 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.

      Thanks for your post which raises useful points. I have three questions. Why is the hash labels ordered 2,0,1 (I'm very easily confused) ? Also, why do we have sth->execute(@CHOICE);. And, why does the EXEC die if there is not the expected number of elements in the array. Is that because of the prepare statement?
        Why is the hash labels ordered 2,0,1 ?

        I thought the comments above the hash explained that. "%procs contains the names of the valid stored procs together with the number of parameters each requires". The key of each hash entry is the name of a valid stored proc. The value associated with the key is the number of parameters that each stored proc requires. The actual numbers that I used (2, 0, 1) were just sample numbers that I made up at random.

        why do we have sth->execute(@CHOICE);

        That is how you put values into the placeholders in an SQL statement. So if you have an SQL statement that is something like select foo from bar where baz = ? then you pass the value for baz as a parameter to the execute function. If you have more than one placeholder (as we do in this case) then we can pass a list (or, in this example, an array that is converted to a list) instead.

        Of course, you could have got all this from the DBI documentation.

        Why does the EXEC die if there is not the expected number of elements in the array

        If you have placeholders in your SQL statement, then execute must be passed enough parameters to match all of the placeholders. If there are too many or too few parameters then execute will throw a fatal error.

      I've tested this bit:
      my $sql = "EXEC $SPROC ". join ', ', ('?') x $procs{$SPROC};
      It doesn't work. It produced a load of '?' in a row. and removing the quotes does not fix the problem. Also, I am having problems adapting the code like follows:
      my @procs = qw/recept/; unless (exists $procs{$SPROC}) { die "Unknown stored proc: $SPROC\n"; }
      Clearly there is a difference between hashes and arrays when it comes to using the exist function.

        I've tested this bit:

        my $sql = "EXEC $SPROC ". join ', ', ('?') x $procs{$SPROC};

        It doesn't work. It produced a load of '?' in a row.

        Erm... yes. That's what it is supposed to do. It produces an SQL statement with the correct number of placeholders in it (a placeholder is marked with a question mark).

        What were you expecting it to produce?

        Also, I am having problems adapting the code like follows:

        my @procs = qw/recept/; unless (exists $procs{$SPROC}) { die "Unknown stored proc: $SPROC\n"; }

        Clearly there is a difference between hashes and arrays when it comes to using the exist function.

        Clearly :-)

        For example. hashes are indexed with strings and arrays are indexed with integers. So trying to see if a string key exists in an array is always going to be doomed to failure.

        But actually, that's not what you're doing is it? You're setting up an array and then looking for a key in a non-existant hash.

        Has someone recommended that you use "strict" and "diagnostics" in your code? Because that would have explained what your problem is here.

      What is the best way of checking that the looked up value in the hash is the same as $elements_in_array for the key $SPROC?
      my %procs = ( reception => 29 ); unless (exists $procs{$SPROC}) { die "Unknown stored proc: $SPROC\n"; } $Command = join(' ', 'EXEC', $SPROC, join(', ', @CHOICE[1 .. $elements_in_array])) . '';
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

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://603188]
Approved by bart
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others taking refuge in the Monastery: (19)
As of 2014-07-22 16:00 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    My favorite superfluous repetitious redundant duplicative phrase is:









    Results (118 votes), past polls