From 9f724a7193eb29efbc22a20fd38e5b6855aab8ac Mon Sep 17 00:00:00 2001 From: Colin Kuskie Date: Wed, 11 Aug 2010 14:50:19 -0700 Subject: [PATCH] block the same set of extensions in http, scalar and file uploads in Storage. --- docs/changelog/7.x.x.txt | 1 + lib/WebGUI/Storage.pm | 42 ++++++++++++++------ t/Storage.t | 86 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 109 insertions(+), 20 deletions(-) diff --git a/docs/changelog/7.x.x.txt b/docs/changelog/7.x.x.txt index b80d52d01..56a25f46c 100644 --- a/docs/changelog/7.x.x.txt +++ b/docs/changelog/7.x.x.txt @@ -3,6 +3,7 @@ - fixed #11779: SQLReport can run arbitrary queries - fixed possible vulnerability loading template parser - fixed possible vulnerability loading help files + - fixed possible vulnerability with filenames in storage locations 7.9.11 - fixed #11755: New cart does not update shipping methods correctly diff --git a/lib/WebGUI/Storage.pm b/lib/WebGUI/Storage.pm index c24b51aaa..d40f4a10d 100644 --- a/lib/WebGUI/Storage.pm +++ b/lib/WebGUI/Storage.pm @@ -315,10 +315,7 @@ sub addFileFromFilesystem { return undef; } my $filename = (File::Spec->splitpath( $pathToFile ))[2]; - if (isIn($self->getFileExtension($filename), qw(pl perl sh cgi php asp pm))) { - $filename =~ s/\./\_/g; - $filename .= ".txt"; - } + $filename = $self->block_extensions($filename); $filename = $self->session->url->makeCompliant($filename); my $source; my $dest; @@ -383,11 +380,7 @@ sub addFileFromFormPost { if ($upload->size > 1024 * $self->session->setting->get("maxAttachmentSize")); $clientFilename =~ s/.*[\/\\]//; $clientFilename =~ s/^thumb-//; - my $type = $self->getFileExtension($clientFilename); - if (isIn($type, qw(pl perl sh cgi php asp html htm))) { # make us safe from malicious uploads - $clientFilename =~ s/\./\_/g; - $clientFilename .= ".txt"; - } + $clientFilename = $self->block_extensions($clientFilename); $filename = $session->url->makeCompliant($clientFilename); my $filePath = $self->getPath($filename); $attachmentCount++; @@ -452,10 +445,7 @@ The content to write to the file. sub addFileFromScalar { my ($self, $filename, $content) = @_; - if (isIn($self->getFileExtension($filename), qw(pl perl sh cgi php asp html htm))) { # make us safe from malicious uploads - $filename =~ s/\./\_/g; - $filename .= ".txt"; - } + $filename = $self->block_extensions($filename); $filename = $self->session->url->makeCompliant($filename); if (open(my $FILE, ">", $self->getPath($filename))) { print $FILE $content; @@ -501,6 +491,32 @@ sub adjustMaxImageSize { #------------------------------------------------------------------- +=head2 block_extensions ( $file ) + +Rename files so they can't be used for malicious purposes. The list of bad extensions +includs shell script, perl scripts, php, ASP, perl modules and HTML files. + +Any file found with a bad extension will be renamed from file.ext to file_ext.txt + +=head3 $file + +The file to check for bad extensions. + +=cut + +sub block_extensions { + my $self = shift; + my $file = shift; + my $extension = $self->getFileExtension($file); + if (isIn($extension, qw(pl perl sh cgi php asp pm html htm))) { + $file =~ s/\.$extension/\_$extension/; + $file .= ".txt"; + } + return $file; +} + +#------------------------------------------------------------------- + =head2 clear ( ) Clears a storage location of all files. If configured for CDN, add diff --git a/t/Storage.t b/t/Storage.t index 19039f3c0..36ac3f824 100644 --- a/t/Storage.t +++ b/t/Storage.t @@ -29,10 +29,14 @@ my $session = WebGUI::Test->session; my $cwd = Cwd::cwd(); -my ($extensionTests, $fileIconTests) = setupDataDrivenTests($session); +my ($extensionTests, $fileIconTests, $block_extension_tests) = setupDataDrivenTests($session); my $numTests = 140; # increment this value for each test you create -plan tests => $numTests + scalar @{ $extensionTests } + scalar @{ $fileIconTests }; +plan tests => 140 + + scalar @{ $extensionTests } + + scalar @{ $fileIconTests } + + scalar @{ $block_extension_tests } + ; my $uploadDir = $session->config->get('uploadsPath'); ok ($uploadDir, "uploadDir defined in config"); @@ -375,6 +379,17 @@ ok (-e $tempStor->getPath(), '... directory was created'); ok($tempStor->getHexId, '... getHexId returns something'); is($tempStor->getHexId, $session->id->toHex($tempStor->getId), '... returns the hexadecimal value of the GUID'); +#################################################### +# +# block_extensions +# +#################################################### + +##Run a set of extensions through and watch how the files get changed. +foreach my $extTest (@{ $block_extension_tests }) { + is( $storage1->block_extensions($extTest->{filename}), $extTest->{blockname}, $extTest->{comment} ); +} + #################################################### # # tar @@ -472,17 +487,17 @@ is($formStore->addFileFromFormPost('files'), undef, 'addFileFromFormPost returns $session->request->uploadFiles( 'oneFile', - [ WebGUI::Test->getTestCollateralPath('WebGUI.pm') ], + [ WebGUI::Test->getTestCollateralPath('littleTextFile') ], ); -is($formStore->addFileFromFormPost('oneFile'), 'WebGUI.pm', '... returns the name of the uploaded file'); -cmp_bag($formStore->getFiles, [ qw/WebGUI.pm/ ], '... adds the file to the storage location'); +is($formStore->addFileFromFormPost('oneFile'), 'littleTextFile', '... returns the name of the uploaded file'); +cmp_bag($formStore->getFiles, [ qw/littleTextFile/ ], '... adds the file to the storage location'); $session->request->uploadFiles( 'thumbFile', [ WebGUI::Test->getTestCollateralPath('thumb-thumb.gif') ], ); is($formStore->addFileFromFormPost('thumbFile'), 'thumb.gif', '... strips thumb- prefix from files'); -cmp_bag($formStore->getFiles, [ qw/WebGUI.pm thumb.gif/ ], '... adds the file to the storage location'); +cmp_bag($formStore->getFiles, [ qw/littleTextFile thumb.gif/ ], '... adds the file to the storage location'); #################################################### # @@ -748,6 +763,63 @@ sub setupDataDrivenTests { }, ]; + my $block_extension_tests = [ + { + filename => 'filename', + blockname => 'filename', + comment => 'no extension', + }, + { + filename => 'filename.pl', + blockname => 'filename_pl.txt', + comment => 'pl file', + }, + { + filename => 'filename.perl', + blockname => 'filename_perl.txt', + comment => 'perl file', + }, + { + filename => 'filename.cgi', + blockname => 'filename_cgi.txt', + comment => 'cgi file', + }, + { + filename => 'filename.php', + blockname => 'filename_php.txt', + comment => 'php file', + }, + { + filename => 'filename.asp', + blockname => 'filename_asp.txt', + comment => 'asp file', + }, + { + filename => 'filename.pm', + blockname => 'filename_pm.txt', + comment => 'perl module file', + }, + { + filename => 'filename.htm', + blockname => 'filename_htm.txt', + comment => 'htm file', + }, + { + filename => 'filename.html', + blockname => 'filename_html.txt', + comment => 'html file', + }, + { + filename => 'filename.pm.txt', + blockname => 'filename.pm.txt', + comment => 'internal .pm not touched', + }, + { + filename => 'filename.txt.pm', + blockname => 'filename.txt_pm.txt', + comment => 'double extension handled', + }, + ]; my $fileIconTests = [ { filename => 'filename', @@ -771,5 +843,5 @@ sub setupDataDrivenTests { }, ]; - return ($extensionTests, $fileIconTests) + return ($extensionTests, $fileIconTests, $block_extension_tests); }