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

I am creating a search function in my site, in which I used Perl and MySQL.

I am just wondering, about this....
In the search box, I am putting a popup_menu, using CGI.pm to create it, with a few variables, such as 10, 20, 40, 80, and 120 "results to show".

So in my search results I am putting this code:
    $sth = $dbh->prepare("SELECT * FROM products WHERE ? IN (prod_name
+,prod_desc) LIMIT 0,?");
    $sth->execute($search_term,$max_recs1);

[download]
I am very concerned about the second limit number, since $max_recs1 is this: $max_recs1 = param("max_rec"); So if I put it in the query itself, and someone posted to the search form, a max_rec value of 100 DELETE FROM products or something like that I don't know that it would work, but if it did, that COULD be disasterous.

So, is the code I put above ok?
Because I'm getting this error:
DBD::mysql::st execute failed: You have an error in your SQL syntax ne
+ar ''10'' at line 1 at /home/user/path/to/pages/products.dat line 182
+6.

[download]
10 is the number I selected when I tested the search.

Thank you for any tips/advice you have.

thx,
Richard

Title edit by tye

Replies are listed 'Best First'.
Re: security question...
by dws (Chancellor) on Apr 25, 2003 at 08:29 UTC
    I am very concerned about the second limit number, since $max_recs1 is this: $max_recs1 = param("max_rec");

    Is there any reason you don't verify that param("max_rec") is one of the numbers that you expect (or at least a number)?

    Consider sanity checking all of the form parameters before you do anything else. Depending on the structure of your app, you might want to re-issue the form if any of the parameters are bad, or you might want to just issue an error page. (In the case of a pop-up menu, if you get bad data you either have a programming error, or someone is trying to hack you. An error page is a reasonable response to either case.)

[reply]
[d/l]
[select]
Re: security question...
by ajt (Prior) on Apr 25, 2003 at 08:42 UTC

    I can't comment on the SQL, but the basic princiles of web input is DON'T TRUST USER INPUT. What you need to do is filter the input so that you let through only values you want to let through, rather than filtering out values you don't want.

    See also:

    *Added extra link.


    --
    ajt
[reply]
Re: security question...
by Tanalis (Curate) on Apr 25, 2003 at 08:37 UTC
    I'd say it's likely to be a bad idea to accept user input directly into a database query that way. As you say yourself, it's trivial to pass arbitrary commands into the script, and hence on into the database query.

    As far as returning a maximum number of results goes, I'd probably verify that you're getting a number, and only a number, back from the webpage. You can do this quite easily with a regexp, and it'd probably just add one line to your source code.

    It's always a good idea, when you're accepting data into your script from an unknown source, to verify the data is exactly in the format you expect.

    Hope that helps a little ...
    -- Foxcub
    A friend is someone who can see straight through you, yet still enjoy the view. (Anon)

[reply]
Re: security question...
by Abigail-II (Bishop) on Apr 25, 2003 at 09:09 UTC
    Use of placeholder does prevent exactly what you are afraid of. What you might want to do is to turn on the ShowErrorStatement attribute in the handle - this will print the complete statement send to the database.

    Abigail

[reply]
Re: security question...
by Biker (Priest) on Apr 25, 2003 at 08:35 UTC

    If you are concerned about the value given to $max_recs1 = param("max_rec"); (which you definitively should be), then you should verify it's contents before using it in your SQL query.

    I'd check that the value is an integer value, positive and smaller or equal to some max value you will have to decide. (In your case typically 120. ;-)

    Everything went worng, just as foreseen.

[reply]
[d/l]
Re: security question...
by zby (Vicar) on Apr 25, 2003 at 09:17 UTC
    I don't have a MySQL database here to try it but perhaps this exerpt from DBI documentation can help: 
    Data Types for Placeholders

   The "\%attr" parameter can be used to hint at the data type the
   placeholder should have. Typically, the driver is only interested
   in knowing if the placeholder should be bound as a number or a
   string.

        $sth->bind_param(1, $value, { TYPE => SQL_INTEGER });

    [download]
    In the error message the number 10 was quoted (like a string), MySQL usually accepts quoted numbers where a number must be, but perhaps not in the LIMIT part of the statement.

    (this answer was lead by a suggestion by PodMaster in CB).

[reply]
[d/l]
Re: security question...
by mce (Curate) on Apr 25, 2003 at 09:22 UTC
    Hi,

    It is strangs that nobody mentioned Taint directly in their post. Use the -T flag in your CGI, or the Taint module from CPAN.
    This way, you are sure that all the parameters need to pass an untaint method (like a regex).

    I hope this helps,
    ---------------------------
    Dr. Mark Ceulemans
    Senior Consultant
    BMC, Belgium

[reply]
Re: security question, mysql, limit, dbi, and placeholders
by Anonymous Monk on Apr 25, 2003 at 20:38 UTC
    Have you tested the search without the LIMIT? Because I suspect the error is elsewhere in your SQL. I don't know MySQL, but other databases I have seen want values in the list for the IN statement. Column names don't work. You might try rewriting it like this: 
    SELECT * FROM products WHERE prod_name = ? OR prod_desc = ?

    [download]
    Also, PostgreSQL doesn't support placeholders for the number after the LIMIT. MySQL may have the same limitation. If you can't use placeholders, you will need to interpolate the value when you construct the SQL. This means you must be careful about validating the value to be a number.
[reply]
[d/l]
      Also, PostgreSQL doesn't support placeholders for the number after the LIMIT.

      Yes it does.

      #!/usr/bin/perl -w
use strict;
use DBI;
my $dbh = DBI->connect("dbi:Pg:dbname=pfau")
    or die "Unable to connect to db: $DBI::errstr";
$dbh->{RaiseError} = 1;
my $s = $dbh->prepare("select relname from pg_class limit ? offset ?")
+;
my $r = $s->execute(3,10);
while (my @row = $s->fetchrow_array) {
    print "Table: $row[0]\n";
}
$s->finish;
$dbh->disconnect;

      [download]

      Produces:

      Table: pg_attrdef
Table: pg_toast_17086_idx
Table: pg_trigger

      [download]
      90% of every Perl application is already written. ⇒
      dragonchild
[reply]
[d/l]
[select]
Re: security question, mysql, limit, dbi, and placeholders
by powerhouse (Friar) on Apr 25, 2003 at 15:43 UTC
    oops, I'm sorry I thought I put what I WAS doing with it, because of my concern, I guess I put that in the question I asked in the MySQL list...

    I added this, just before the MySQL Syntax:
    if ($max_recs1 !~ /^(10|20|40|80|120|160)$/) {
    $html_content .= qq~You have entered an ILLEGAL Number of Results 
+to post!!~ . br() x 2;
    search_box();
    return;
}

    [download]
    Sorry about that :o(

    What I'm wondering about is if I can put the $max_recs1 in as a placeholder, in a LIMIT.

    Did that make it clearer I hope?


    UPDATE: Sorry, nevermind. I guess If I check to make sure it is one of the numbers I offer as a choice, then it don't matter If I make it a placeholder or not, since they cannot put anything but one of the numbers....

    thx,
    Richard
[reply]
[d/l]

Back to Seekers of Perl Wisdom