--- Will Cottay <[EMAIL PROTECTED]> wrote:
> The only reason I'm posting an example of how to do this is so
> that I can feel better about commenting on just how excruciatingly
> bad your code is.
Well, since you apparently feel that poor code is such an abomination, I don't suppose
you'll mind
my "going the extra mile" in reviewing your code, though I won't be so mean-spirited
about it.
Comments occur *after* the line in question.
> #!/usr/bin/perl -w
Hmm.... no taint checking. Wonder if this will catch us.
> use strict;
> use CGI qw(:all);
(:all) is extra, useless, overhead in this program because it merely imports the
CGI.pm functions.
You're using the object oriented interface, so why import functions you don't use?
> my $q = new CGI;
> print $q->header;
> print $q->start_html(-title=>'File Upload Example');
>
> my $filename = $q->param('img');
Old code. As of $CGI::VERSION 2.47, you should use the CGI::upload() function.
Further, trying
to use $filename as a filehandle should cause problems while running under strict.
Did you test
this code?
> $| = 1;
Why are you unbuffering output now, rather than at the top of the script *before* you
start
printing things?
> print $q->start_multipart_form(-name=>'fup');
> print $q->filefield(-name=>'img');
> print "<input type='submit' name='sfile'></form>";
?? Why use the HTML generating functions and then stop?
print $q->submit( { name => 'sname ' } ),$q->end_form;
> print $q->end_html;
>
> if ($filename) {
> my ($fn, $fout, $buffer, $bytesread);
> ($fn) = $filename =~ m!([^/|\\]+)$!;
Hey, this is interesting. Virtually every security guide says you should specify what
to include,
not what to exclude. Wonder if anything was missed there.
Well, yes. What's going to happen if someone uploads a file called '.htaccess'?
Hmm... that
could be lots of fun. They could upload CGI scripts and try to execute them. On some
systems,
the caret "^" is equivalent to the pipe. Users could append a caret and have fun
trying to
execute commands. Oh, and doesn't the Mac use a colon for a path separator? Hmm...
now we have
reverse directory traversal available, too!
> $fout = $ENV{DOCUMENT_ROOT} . '/photos/ptmp/' . $fn;
> open (OUTFILE,"> $fout") or die "Can't open $fout: $!";
The preceeding paragraph was all about the above line: never let users name files, if
you can
help it. And what's to top them from overwriting files? What's to stop a DoS attack
based on
uploading too many files?
> print OUTFILE $buffer while ($bytesread = read($filename, $buffer, 1024));
What if this is run on Windows? Needs 'binmode' for both the read and write. Not
very portable.
Further, you're just thowing away the value of $bytesread. Why use it?
> close OUTFILE;
> }
Admittedly, you were just supplying an example and that's a good thing. I wouldn't
have said a
single word about your code if you hadn't been so vicious in your reply.
Cheers,
Curtis Poe
=====
Senior Programmer
Onsite! Technology (http://www.onsitetech.com/)
"Ovid" on http://www.perlmonks.org/
__________________________________________________
Do You Yahoo!?
Get personalized email addresses from Yahoo! Mail
http://personal.mail.yahoo.com/
