Skip Menu |
 

This queue is for tickets about the CGI-Simple CPAN distribution.

Report information
The Basics
Id: 8475
Status: resolved
Priority: 0/
Queue: CGI-Simple

People
Owner: Nobody in particular
Requestors: steve [...] purkis.ca
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: 0.075
Fixed in: (no value)

Attachments
CGI-Simple-post_max.patch
CGI-Simple-upload.patch



Subject: POST_MAX not obeyed for file uploads
Download (untitled) / with headers
text/plain 576b
Hiya, I've been using CGI::Simple, and have run into a critical problem -- it seems $CGI::Simple::POST_MAX is not being obeyed for form/multipart file uploads (I haven't tried regular POSTs, but at a glance, there may be some problems there too). I've had a quick look under the hood, and it seems the culprit is _parse_multipart() -- it never checks $self->{'.globals'}->{'POST_MAX'}, but instead keeps on appending the read $buffer to $data. The attached patch takes a stab at fixing this; while I wouldn't consider it a complete solution, at least it's a start. -Steve
--- /usr/lib/perl5/site_perl/5.6.1/CGI/Simple.pm 2004-06-01 19:20:00.000000000 +0100 +++ /home/spurkis/perl5lib/CGI/Simple.pm 2004-11-16 18:02:15.000000000 +0000 @@ -227,6 +227,13 @@ return; } } + + # TODO: this makes no sense -- + # Shouldn't we really be doing this POST_MAX test above? + # Checking POST_MAX here actually means we'll read the whole POST, + # even if POST_MAX is set! + # -spurkis + # we do this test after the read so we strip the data from STDIN if ( $self->{'.globals'}->{'POST_MAX'} != -1 and $length > $self->{'.globals'}->{'POST_MAX'} ) { $self->cgi_error( "413 Request entity too large: $length bytes on STDIN exceeds \$POST_MAX!" ); @@ -313,11 +320,28 @@ my $data = ''; my $length = $ENV{'CONTENT_LENGTH'} || 0; my $CRLF = $self->crlf; + my $post_max = $self->{'.globals'}->{POST_MAX}; + +$post_max = 170; READ: while ( $got_data < $length ) { - last READ unless sysread( STDIN, my $buffer, 4096 ); + warn "got data: [$got_data]\n"; + if ( $post_max != -1 and $got_data > $post_max ) { + warn( "413 Request entity too large: $length bytes on STDIN exceeds \$POST_MAX($post_max)!\n" ); + $self->cgi_error( "413 Request entity too large: $length bytes on STDIN exceeds \$POST_MAX($post_max)!" ); + my $discard_len = $length - $got_data; + # silently discard data + while ( $discard_len > 0 ) { + last unless sysread( STDIN, my $buffer, 4096 ); + $discard_len -= length( $buffer ); + } + return $got_data; + } + + last READ unless sysread( STDIN, my $buffer, 128 ); + $data .= $buffer; $got_data += length $buffer;
From: spurkis [...] cpan.org
Download (untitled) / with headers
text/plain 767b
I've revised the previous patch so it should be production-ready. See attached patch. -Steve [SPURKIS - Tue Nov 16 13:05:38 2004]: Show quoted text
> Hiya, > > I've been using CGI::Simple, and have run into a critical problem -- > it seems $CGI::Simple::POST_MAX is not being obeyed for > form/multipart file uploads (I haven't tried regular POSTs, but at > a glance, there may be some problems there too). > > I've had a quick look under the hood, and it seems the culprit is > _parse_multipart() -- it never checks $self->{'.globals'}-
> >{'POST_MAX'}, but instead keeps on appending the read $buffer to
> $data. > > The attached patch takes a stab at fixing this; while I wouldn't > consider it a complete solution, at least it's a start. > > -Steve
--- CGI-Simple-tmp/CGI/Simple.pm 2004-11-17 14:38:38.000000000 +0000 +++ /usr/lib/perl5/site_perl/5.6.1/CGI/Simple.pm 2004-06-01 19:20:00.000000000 +0100 @@ -206,9 +206,11 @@ my $type = $ENV{'CONTENT_TYPE'} || 'No CONTENT_TYPE received'; my $length = $ENV{'CONTENT_LENGTH'} || 0; my $method = $ENV{'REQUEST_METHOD'} || 'No REQUEST_METHOD received'; - if ( $length and $type =~ m|^multipart/form-data|i ) { - return $self->_parse_multipart; + my $got_length = $self->_parse_multipart; + $self->cgi_error( "500 Bad read on multipart/form-data! wanted $length, got $got_length" ) + unless $length == $got_length; + return; } elsif ( $method eq 'POST') { if ( $length ) { @@ -225,13 +227,6 @@ return; } } - - # TODO: this makes no sense -- - # Shouldn't we really be doing this POST_MAX test above? - # Checking POST_MAX here actually means we'll read the whole POST, - # even if POST_MAX is set! - # -spurkis - # we do this test after the read so we strip the data from STDIN if ( $self->{'.globals'}->{'POST_MAX'} != -1 and $length > $self->{'.globals'}->{'POST_MAX'} ) { $self->cgi_error( "413 Request entity too large: $length bytes on STDIN exceeds \$POST_MAX!" ); @@ -309,45 +304,31 @@ my ($boundary) = $ENV{'CONTENT_TYPE'} =~ /boundary=\"?([^\";,]+)\"?/; unless ($boundary) { $self->cgi_error( '400 No boundary supplied for multipart/form-data' ); - return 0; + return 0; } # BUG: IE 3.01 on the Macintosh uses just the boundary, forgetting the -- $boundary = '--'.$boundary unless $ENV{'HTTP_USER_AGENT'} =~ m/MSIE\s+3\.0[12];\s*Mac/i; $boundary = quotemeta $boundary; - - my $CRLF = $self->crlf; - my $post_max = $self->{'.globals'}->{POST_MAX}; - my $length = $ENV{'CONTENT_LENGTH'} || 0; my $got_data = 0; - my $data = ''; + my $data = ''; + my $length = $ENV{'CONTENT_LENGTH'} || 0; + my $CRLF = $self->crlf; READ: - while ( $got_data < $length ) { - - if ( $post_max != -1 and $got_data > $post_max ) { - $self->cgi_error( "413 Request entity too large: $length bytes on STDIN exceeds \$POST_MAX($post_max)!" ); - my $discard_len = $length - $got_data; - # silently discard data - while ( $discard_len > 0 ) { - last unless sysread( STDIN, my $buffer, 4096 ); - $discard_len -= length( $buffer ); - } - return; - } - - last READ unless sysread( STDIN, my $buffer, 128 ); + while ( $got_data < $length ) { + last READ unless sysread( STDIN, my $buffer, 4096 ); $data .= $buffer; $got_data += length $buffer; BOUNDARY: + while ( $data =~ m/^$boundary$CRLF/ ) { next READ unless $data =~ m/^([\040-\176$CRLF]+?$CRLF$CRLF)/o; my $header = $1; - (my $unfold = $1) =~ s/$CRLF\s+/ /og; - my ($param) = $unfold =~ m/form-data;\s+name="?([^\";]*)"?/; + (my $unfold = $1) =~ s/$CRLF\s+/ /og; + my ($param) = $unfold =~ m/form-data;\s+name="?([^\";]*)"?/; my ($filename) = $unfold =~ m/name="?\Q$param\E"?;\s+filename="?([^\"]*)"?/; - if (defined $filename ) { my ($mime) = $unfold =~ m/Content-Type:\s+([-\w\/]+)/io; $data =~ s/^\Q$header\E//; @@ -355,18 +336,14 @@ $self->_add_param( $param, $filename ); $self->{'.upload_fields'}->{$param} = $filename; $self->{'.filehandles'}->{$filename} = $fh if $fh; - $self->{'.tmpfiles'}->{$filename} = {'size'=>$size, 'mime'=>$mime } if $size; - next BOUNDARY; + $self->{'.tmpfiles'}->{$filename} = {'size'=>$size, 'mime'=>$mime } if $size; + next BOUNDARY; } next READ unless $data =~ s/^\Q$header\E(.*?)$CRLF(?=$boundary)//s; $self->_add_param( $param, $1 ); } } - - $self->cgi_error( "500 Bad read on multipart/form-data! wanted $length, got $got_data" ) - unless $length == $got_data; - - return; + return $got_data; } sub _save_tmpfile {
From: tshinnic [...] io.com
Download (untitled) / with headers
text/plain 246b
I just hit this also, and was thinking of describing this as "No size limit on file uploads - $POST_MAX ignored for multipart/formdata". But the essence of the problem is that if file uploads are enabled at all, then DOS attacks are possible.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.