Re^5: DBI do() SQL injection

If you have no control over the contents of $id, you shouldn't use it directly. What if $id was set to 42', 'Batman'); drop table customers; --?

In fact, I tried it with two different database drivers. Interestingly, in DBD::SQLite, the table wasn't removed, as it doesn't support multiple statements at a time. On the other hand, DBD::Pg happily removed the table and crashed when I tried using it later.

#!/usr/bin/perl
use warnings;
use strict;
use feature qw{ say };
use experimental qw{ signatures };

use DBI;

sub run_do($dbh, $id, $name) {
    $dbh->do(qq{
        INSERT INTO customers
        (id,name)
        VALUES('$id','$name')});
}

sub run_select($dbh) {
    my $sth = $dbh->prepare('SELECT * FROM customers');
    $sth->execute;
    while (my @row = $sth->fetchrow_array) {
        say "@row";
    }
}

my %connection = ('dbi:SQLite:dbname=:memory:' => ["", ""],
                  "dbi:Pg:dbname=$ENV{USER}" => [$ENV{USER}, ""]);

for my $dbcs (keys %connection) {
    my $dbh = 'DBI'->connect($dbcs, @{ $connection{$dbcs} });
    $dbh->do('CREATE TABLE customers (id INTEGER, name TEXT)');

    run_do($dbh, 1, 'John');
    run_select($dbh);

    my $id = q{42', 'Batman'); DROP TABLE customers; -- };
    run_do($dbh, $id, 'Joe');
    run_select($dbh);
}
[download]

The output:

1 John
1 John
42 Batman
1 John
DBD::Pg::st execute failed: ERROR:  relation "customers" does not exis
+t
LINE 1: SELECT * FROM customers
                      ^ at ./1.pl line 18.
DBD::Pg::st fetchrow_array failed: no statement executing at ./1.pl li
+ne 19.
[download]

You can always fix that by wrapping the value into a quote:

    $dbh->do(join "",
             'INSERT INTO customers(id,name) VALUES(',
             $dbh->quote($id), ', ',
             $dbh->quote($name),
             ')');
[download]

map{substr$_->[0],$_->[1]||0,1}[\*||{},3],[[]],[ref qr-1,-,-1],[{}],[sub{}^*ARGV,3]

Comment on Re^5: DBI do() SQL injection Select or Download Code

Replies are listed 'Best First'.
Re^6: DBI do() SQL injection by afoken (Chancellor) on Oct 19, 2023 at 15:00 UTC
If you have no control over the contents of $id, you shouldn't use it directly. Even if you have control, you should use placeholders. Keeping the query string constant and only varying the parameters allows everything below DBI (i.e. the DBD and especially the database engine) to cache the already parsed query string, including all optimizations. Changing the query string for every new value of `$id` prevents that. Alexander -- Today I will gladly share my knowledge and experience, for there are no sweeter words than "I told you so". ;-)	[reply] [d/l]
Re^6: DBI do() SQL injection by dsheroh (Monsignor) on Oct 20, 2023 at 08:13 UTC
You can always fix that by wrapping the value into a `quote` And is there any advantage to using `quote` instead of placeholders? `sub run_do_with_placeholders($dbh, $id, $name) { $dbh->do(qq{ INSERT INTO customers (id,name) VALUES(?, ?)}, undef, $id, $name); }` [download] There are a few places (table and column names, mostly) where you're required to use `quote` instead of placeholders, but, IMO, you should always use placeholders for data values where possible and never rely on quoting unless you absolutely have to.	[reply] [d/l] [select]
Re^7: DBI do() SQL injection by choroba (Cardinal) on Oct 20, 2023 at 17:38 UTC
> There are a few places (table and column names, mostly) where you're required to use quote instead of placeholders No, please don't. Use `quote_identifier` for table and column names. `quote` is useful when placeholders can't be used, e.g. you're sending the SQL statement to a function that doesn't accept any other arguments and you can't change it; but generally placeholders are easier and cleaner. `map{substr$_->[0],$_->[1]\|\|0,1}[\\|\|{},3],[[]],[ref qr-1,-,-1],[{}],[sub{}^ARGV,3]`	[reply] [d/l] [select]
Re^8: DBI do() SQL injection by dsheroh (Monsignor) on Oct 21, 2023 at 11:21 UTC
No, please don't. Use `quote_identifier` for table and column names. Whoops! Good catch, and thanks for the correction!	[reply] [d/l]
Re^6: DBI do() SQL injection by ikegami (Patriarch) on Oct 20, 2023 at 16:22 UTC
Even if the table isn't dropped, it's still an injection bug since the code did not behave as desired (i.e. did not use `42', 'Batman'); drop table customers; --` as the id).	[reply] [d/l]
Re^6: DBI do() SQL injection by Anonymous Monk on Oct 19, 2023 at 18:34 UTC
How does dbh quote save the day? What is different from simple quotinq with '' or q or qq ?	[reply]
Re^7: DBI do() SQL injection by choroba (Cardinal) on Oct 19, 2023 at 18:38 UTC
`#!/usr/bin/perl use warnings; use strict; use feature qw{ say }; use DBI; my $dbh = 'DBI'->connect('dbi:SQLite:dbname=:memory:', "", ""); my $id = q{42', 'Batman'); DROP TABLE customers; -- }; say qq('$id'); say $dbh->quote($id); __END__ Output: '42', 'Batman'); DROP TABLE customers; -- ' '42'', ''Batman''); DROP TABLE customers; -- '` [download] `map{substr$_->[0],$_->[1]\|\|0,1}[\\|\|{},3],[[]],[ref qr-1,-,-1],[{}],[sub{}^ARGV,3]`	[reply] [d/l] [select]