in reply to Re: Bad Practice
in thread Bad Practice

I just looked into this CGI module which is supposed to be so great and do it all right. Could you please point out where it actually validates CONTENT_LENGTH? All I can see is that it expects it to be a number. It'll explicitely substitute '0' if the environment variable is undefined, but that's something what Perl would do anyway.

The fact that it doesn't handle multiple select forms isn't necessary bad. If the forms used on the site don't have multiple selects anyway, it's not bad if you can't deal with them correctly.

You also say that this code allows to create any variable he wishes. I've looked at the given code a few times, but I can't see what you see. Unless you see a way of the web user being able to manipulate %input, I don't see how he can.

As for %XY, which is invalid in a URL, this code with treat it as if it had been %00 (which is valid). Is it a problem that the error isn't caught? CGI.pm leaves %XY as %XY, with no feedback. This code at least issues a warning (if they are turned on).

Your last remark is a bit silly. Do you really think CGI.pm will not break if you use it for things Lincoln Stein didn't envision when he wrote it?

I'm not saying the presented code is all perfect, not at all. But CGI.pm isn't as holy as you may think.

Abigail

Replies are listed 'Best First'.
Re: Re: Bad Practice
by jasonk (Parson) on Feb 27, 2003 at 16:00 UTC

    You also say that this code allows to create any variable he wishes. I've looked at the given code a few times, but I can't see what you see. Unless you see a way of the web user being able to manipulate %input, I don't see how he can.

    Right after the call to readparse, there is a foreach which loops through %input and creates global variables for every element in the hash. As a contrived example, this could allow the browser to change the process name, by submitting a form containing <input type=hidden name="0" value="HA! HA! GOT YOU!">. If the rest of the code is as insecure as this, you could have a lot of fun with this site.

[reply]
      Oh, I've seen that code. But please tell me, how is the web user able to manipulate %input? As far as I can tell. <input type = hidden name = '0' value = 'HA! HA! GOT YOU!'> causes $in {0} to be set to 'HA! HA! GOT YOU!'.

      I don't know which programming language you are using, but any Perl I've used so far uses more than 2 significant letters in indentifiers. %in is not at all the same as %input.

      Abigail

[reply]
[d/l]
[select]

        But please tell me, how is the web user able to manipulate %input?
        Possibly I'm mistaken, but it seems to me that the first line of readparse() aliases the local glob *in to the argument of readparse, which is (in this case) the global *input. 

        readparse(*input);

sub readparse {
    local (*in) = @_ if @_;
    ...
}

        [download]

        So in fact, the variable %in within readparse is the same as the %input that isotope and jasonk are complaining about.


        If God had meant us to fly, he would *never* have given us the railroads.
            --Michael Flanders

[reply]
[d/l]
        %in is not at all the same as %input.

        Unless they're aliased... 

          readparse(*input);
  ...
  sub readparse {
    local (*in) = @_ if @_;

        [download]

        ihb
[reply]
[d/l]
[select]
Re: Re: Bad Practice
by isotope (Deacon) on Feb 27, 2003 at 20:28 UTC
    From version 2.91: 
    if ($meth eq 'POST') {
  $self->read_from_client(\*STDIN,\$query_string,$content_length,0)
    if $content_length > 0;

    [download]
    So CGI.pm is verifying that it's a positive number. Further, within read_from_client(), it calls read(), which only uses the length as an upper bound. The logic then seems to flow like this: If the Content-length header is missing, assume 0. If it's there, and greater than zero, then try to read from the client until you reach the end of the input, or the value of content_length, whichever comes first. Granted, Lincoln didn't explicitly verify that content_length is an integer, but the above code and the call to read() should cover that.

    Treating %XY in a URL as the literal "%XY" makes more sense to me than trying to unescape it and coming up with '\0', but that's probably just me.

    CGI.pm isn't perfect, but I'd trust it a whole lot more than this snippet bastardized from cgi-lib.pl, especially coupled with the loop that does create global variables named for the CGI parameter names.

    --isotope
[reply]
[d/l]

In Section Seekers of Perl Wisdom