Re: Bad Practice

Aside for the inappropriate use of local() instead of my, there are some easy problems here. Strict, -w, and -T would have caught some of them, too.

You can start with this node. Ok, so this differentiates POST and GET requests. It doesn't validate CONTENT_LENGTH. It doesn't handle multiple select forms.

This code allows a web user to create any variable he wishes, and overwrite other variables in the rest of the script. Somebody could really have fun with that.

There is insufficient error checking. What if the literal %YZ appears in the URL? The pack() attempts to dehexify it, but it's not truly URL-encoded. Where is the error caught?

In fact, this snippet looks much like Ovid's favorite example of bad code on this node.

For a simpler argument, CGI.pm does it right, according to RFCs, and has been extensively peer-reviewed. If I understand your post correctly, your code has basically been reviewed only by 3 beginners, and will break if you try to use it for things the original coders did not envision when they wrote it.

--isotope

Comment on Re: Bad Practice

Replies are listed 'Best First'.
Re: Bad Practice by Abigail-II (Bishop) on Feb 27, 2003 at 10:43 UTC
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	[reply] [d/l] [select]
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]
Re: Bad Practice by Abigail-II (Bishop) on Feb 27, 2003 at 16:11 UTC
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]
Re: Bad Practice by ChemBoy (Priest) on Feb 27, 2003 at 16:36 UTC
Re: Re: Bad Practice by ihb (Deacon) on Feb 27, 2003 at 16:35 UTC
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]


Syntactic Confectionery Delight
	PerlMonks