From 69ab151ec3f6795f63d229c03e01a763f2d61035 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Tue, 11 Jun 2024 06:43:53 -0500 Subject: [PATCH 1/3] Prevent protected files from being modified by unprivileged users. By "protected files" I mean the files listed in the `$uneditableCourseFiles` list in the course environemt that is originally defined in `defaults.config`. This usually contains the files `course.conf` and `simple.conf`. As before, only users with the "edit_restricted_files" permission can edit these files. The only change here is that `Mojo::File::realpath` is used instead of the `File::Spec::canonpath`. This is a more reliable method since it properly resolves to a path without `..` (canonpath does not do this). It is possible to use `..` to create a filename that wouild still resolve to one of the protected files. For instance, `templates/../course.conf` could be used to overwrite `course.conf`. The canonpath method would not correctly realize that those are the same file paths. If a user that does not have the permission to "edit_restricted_files", and the user attempts to upload a file in the list, then the user will be forced to choose a new name for the upload, or cancel. The overwrite option will not be given. Users with the "edit_restricted_files" permission can still overwrite the file. If a user that does not have the permission to "edit_restricted_files" extracts an archive that would overwrite a file in the list, then that file will be skipped and will not be extracted. A message will notify the user that this file from the archive is not extracted. Deletion or renaming of one of the files in the list is not allowed. Not even for users that have the "edit_restricted_files" permission. These files are required, and there is no reason that this should ever be done. Finally, required course directories (all directories in the `courseDirs` hash in the course environment) are not allowed to be deleted or renamed by any user. These directories are required, and so there is not reason that even a user with the "edit_restricted_files" permission should be allowed to do this. --- .../Instructor/FileManager.pm | 100 +++++++++++++++++- 1 file changed, 95 insertions(+), 5 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm index 8b1fa2042f..b38ee55a04 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm @@ -198,7 +198,7 @@ sub Edit ($c) { # If its a restricted file, dont allow the web editor to edit it unless that option has been set for the course. for my $restrictedFile (@{ $c->ce->{uneditableCourseFiles} }) { - if (File::Spec->canonpath($file) eq File::Spec->canonpath("$c->{courseRoot}/$restrictedFile") + if (Mojo::File->new($file)->realpath eq Mojo::File->new("$c->{courseRoot}/$restrictedFile")->realpath && !$c->authz->hasPermissions($c->param('user'), 'edit_restricted_files')) { $c->addbadmessage($c->maketext('You do not have permission to edit this file.')); @@ -306,6 +306,18 @@ sub Rename ($c) { return '' unless $original; my $oldfile = "$dir/$original"; + my $realpath = Mojo::File->new($oldfile)->realpath; + if (grep { $realpath eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } @{ $c->ce->{uneditableCourseFiles} }) { + $c->addbadmessage($c->maketext('The file "[_1]" is protected and can not be renamed.', $original)); + return $c->Refresh(); + } + + if (grep { $realpath eq $_ } values %{ $c->ce->{courseDirs} }) { + $c->addbadmessage($c->maketext( + 'The directory "[_1]" is a required course directory and cannot be renamed.', $original)); + return $c->Refresh(); + } + if ($c->param('confirmed')) { my $newfile = $c->param('name'); if ($newfile = $c->verifyPath($newfile, $original)) { @@ -332,9 +344,44 @@ sub Delete ($c) { } my $dir = "$c->{courseRoot}/$c->{pwd}"; + + my @course_dirs = values %{ $c->ce->{courseDirs} }; + + # If only one file is selected and it is one of the uneditable course files, + # then don't show the deletion confirmation page. Just warn about it now. + if (@files == 1) { + my $realpath = Mojo::File->new("$dir/$files[0]")->realpath; + if (grep { $realpath eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } @{ $c->ce->{uneditableCourseFiles} }) + { + $c->addbadmessage($c->maketext('The file "[_1]" is protected and cannot be deleted.', $files[0])); + return $c->Refresh(); + } + if (grep { $realpath eq $_ } @course_dirs) { + $c->addbadmessage($c->maketext( + 'The directory "[_1]" is a required course directory and cannot be deleted.', + $files[0] + )); + return $c->Refresh(); + } + } + if ($c->param('confirmed')) { # If confirmed, go ahead and delete the files for my $file (@files) { + my $realpath = Mojo::File->new("$dir/$file")->realpath; + if (grep { $realpath eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } + @{ $c->ce->{uneditableCourseFiles} }) + { + $c->addbadmessage($c->maketext('The file "[_1]" is protected and cannot be deleted.', $file)); + next; + } + if (grep { $realpath eq $_ } @course_dirs) { + $c->addbadmessage($c->maketext( + 'The directory "[_1]" is a required course directory and cannot be deleted.', $file + )); + next; + } + if (defined $c->checkPWD("$c->{pwd}/$file", 1)) { if (-d "$dir/$file" && !-l "$dir/$file") { my $removed = eval { rmtree("$dir/$file", 0, 1) }; @@ -463,7 +510,7 @@ sub UnpackArchive ($c) { sub unpack_archive ($c, $archive) { my $dir = Mojo::File->new($c->{courseRoot}, $c->{pwd}); - my (@members, @existing_files, @outside_files); + my (@members, @existing_files, @outside_files, @forbidden_files); my $num_extracted = 0; if ($archive =~ m/\.zip$/) { @@ -486,6 +533,14 @@ sub unpack_archive ($c, $archive) { next; } + if (!$c->authz->hasPermissions($c->param('user'), 'edit_restricted_files') + && grep { $out_file eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } + @{ $c->ce->{uneditableCourseFiles} }) + { + push(@forbidden_files, $_->fileName); + next; + } + if (!$c->param('overwrite') && -e $out_file) { push(@existing_files, $_->fileName); next; @@ -513,6 +568,14 @@ sub unpack_archive ($c, $archive) { next; } + if (!$c->authz->hasPermissions($c->param('user'), 'edit_restricted_files') + && grep { $out_file eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } + @{ $c->ce->{uneditableCourseFiles} }) + { + push(@forbidden_files, $_); + next; + } + if (!$c->param('overwrite') && -e $dir->child($_)) { push(@existing_files, $_); next; @@ -557,6 +620,21 @@ sub unpack_archive ($c, $archive) { ); } + # There aren't many of these, so all of them can be reported. + if (@forbidden_files) { + $c->addbadmessage( + $c->tag( + 'p', + $c->maketext( + 'The following [plural,_1,file] found in the archive [plural,_1,is,are]' + . ' protected and were not extracted.', + scalar(@forbidden_files), + ) + ) + . $c->tag('div', $c->tag('ul', $c->c((map { $c->tag('li', $_) } @forbidden_files))->join(''))) + ); + } + if (@existing_files) { $c->addbadmessage( $c->tag( @@ -653,21 +731,33 @@ sub Upload ($c) { } if (-e "$dir/$name") { + my $isProtected = !$c->authz->hasPermissions($c->param('user'), 'edit_restricted_files') + && grep { Mojo::File->new("$dir/$name")->realpath eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } + @{ $c->ce->{uneditableCourseFiles} }; + unless ($c->param('overwrite') || $action eq 'Overwrite' || $action eq $c->maketext('Overwrite')) { return $c->c( $c->Confirm( $c->tag( 'p', - $c->b($c->maketext( - 'File [_1] already exists. Overwrite it, or rename it as:', $name)) + $c->b( + $isProtected + ? $c->maketext('File [_1] is protected and cannot be overwritten. Rename it as:', + $name) + : $c->maketext('File [_1] already exists. Overwrite it, or rename it as:', $name) + ) ), uniqueName($dir, $name), $c->maketext('Rename'), - $c->maketext('Overwrite') + $isProtected ? '' : $c->maketext('Overwrite') ), $c->hidden_field(action => 'Upload'), $c->hidden_field(file => $fileIDhash) )->join(''); + } elsif ($isProtected) { + $c->addbadmessage($c->maketext('The file "[_1]" is protected and cannot be overwritten.', $name)); + $upload->dispose; + return $c->Refresh; } } $c->checkFileLocation($name, $c->{pwd}); From 165fa8e51528d6f53136d8a17d6949381cd64371 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Thu, 13 Jun 2024 10:04:00 -0500 Subject: [PATCH 2/3] Rework the `checkName` method of the FileManager to return a message for invalid file names, rather than attempting to fix and return a workable file name. Also, instead of just giving a message if a user attempts to upload a protected file, give the user a chance to rename it. --- .../Instructor/FileManager.pm | 63 +++++++++++++------ 1 file changed, 43 insertions(+), 20 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm index b38ee55a04..d6bea54915 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm @@ -100,9 +100,12 @@ sub pre_header_initialize ($c) { } # Download a given file -sub downloadFile ($c, $filename, $directory = '') { - my $file = checkName($filename); - my $pwd = $c->checkPWD($directory || $c->param('pwd') || HOME); +sub downloadFile ($c, $file, $directory = '') { + if (my $invalidFilenameMessage = $c->checkName($file)) { + $c->addbadmessage($invalidFilenameMessage); + return; + } + my $pwd = $c->checkPWD($directory || $c->param('pwd') || HOME); return unless $pwd; $pwd = $c->{ce}{courseDirs}{root} . '/' . $pwd; unless (-e "$pwd/$file") { @@ -720,28 +723,51 @@ sub Upload ($c) { my ($id, $hash) = split(/\s+/, $fileIDhash); my $upload = WeBWorK::Upload->retrieve($id, $hash, dir => $c->{ce}{webworkDirs}{uploadCache}); - my $name = checkName($upload->filename); + my $name = $upload->filename; + my $invalidUploadFilenameMsg = $c->checkName($name); + my $action = $c->param('formAction') || 'Cancel'; if ($c->param('confirmed')) { if ($action eq 'Cancel' || $action eq $c->maketext('Cancel')) { $upload->dispose; return $c->Refresh; } - $name = checkName($c->param('name')) if ($action eq 'Rename' || $action eq $c->maketext('Rename')); + if ($action eq 'Rename' || $action eq $c->maketext('Rename')) { + if (!$c->param('name') || $c->param('name') eq $name) { + $c->addbadmessage($c->maketext('You must specify a new file name.')); + } elsif (my $invalidFileNameMsg = $c->checkName($c->param('name'))) { + $c->addbadmessage($invalidFileNameMsg); + } else { + $name = $c->param('name'); + + # In this case the upload file name is still invalid, but it isn't going to be used anyway. So setting + # this to 0 lets the new name go through (if it doesn't already exist). + $invalidUploadFilenameMsg = 0; + } + } } - if (-e "$dir/$name") { + $c->addbadmessage($invalidUploadFilenameMsg) if $invalidUploadFilenameMsg; + + if (-e "$dir/$name" || $invalidUploadFilenameMsg) { my $isProtected = !$c->authz->hasPermissions($c->param('user'), 'edit_restricted_files') && grep { Mojo::File->new("$dir/$name")->realpath eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } @{ $c->ce->{uneditableCourseFiles} }; - unless ($c->param('overwrite') || $action eq 'Overwrite' || $action eq $c->maketext('Overwrite')) { + if (($invalidUploadFilenameMsg || $isProtected) + || !$c->param('overwrite') + || $action ne 'Overwrite' + || $action ne $c->maketext('Overwrite')) + { return $c->c( $c->Confirm( $c->tag( 'p', $c->b( - $isProtected + $invalidUploadFilenameMsg + ? $c->maketext('[_1] is an invalid file name and must be renamed. Rename it as:', + $name) + : $isProtected ? $c->maketext('File [_1] is protected and cannot be overwritten. Rename it as:', $name) : $c->maketext('File [_1] already exists. Overwrite it, or rename it as:', $name) @@ -749,15 +775,11 @@ sub Upload ($c) { ), uniqueName($dir, $name), $c->maketext('Rename'), - $isProtected ? '' : $c->maketext('Overwrite') + $isProtected || $invalidUploadFilenameMsg ? '' : $c->maketext('Overwrite') ), $c->hidden_field(action => 'Upload'), $c->hidden_field(file => $fileIDhash) )->join(''); - } elsif ($isProtected) { - $c->addbadmessage($c->maketext('The file "[_1]" is protected and cannot be overwritten.', $name)); - $upload->dispose; - return $c->Refresh; } } $c->checkFileLocation($name, $c->{pwd}); @@ -991,13 +1013,14 @@ sub checkFileLocation ($c, $extension, $dir) { return; } -# Check a name for bad characters, etc. -sub checkName ($file) { - $file =~ s!.*[/\\]!!; # remove directory - $file =~ s/[^-_.a-zA-Z0-9 ]/_/g; # no illegal characters - $file =~ s/^\./_/; # no initial dot - $file = 'newfile.txt' unless $file; # no blank names - return $file; +# Check a file name for bad characters, etc. This returns a message explaining what is wrong with the file name if +# something is wrong, and undefined if all is good. +sub checkName ($c, $file) { + return $c->maketext('No filename specified.') unless $file; + return $c->maketext('"[_1]" contains a path component which is not allowed.', $file) if $file =~ /\//; + return $c->maketext('"[_1]" contains invalid characters.', $file) if $file =~ m![^-_.a-zA-Z0-9 /]!; + return $c->maketext('"[_1]" begins with a period which is not allowed.', $file) if $file =~ m/^\./; + return; } # Get a unique name (in case it already exists) From 25c9129afb364c20bed6aabf054cffc20fba11f6 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Tue, 30 Jul 2024 13:31:19 -0500 Subject: [PATCH 3/3] Fix incorrect logic for determining when an uploaded file can be overwritten. --- lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm index d6bea54915..ffe587ff43 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm @@ -754,10 +754,12 @@ sub Upload ($c) { && grep { Mojo::File->new("$dir/$name")->realpath eq Mojo::File->new("$c->{courseRoot}/$_")->realpath } @{ $c->ce->{uneditableCourseFiles} }; - if (($invalidUploadFilenameMsg || $isProtected) - || !$c->param('overwrite') - || $action ne 'Overwrite' - || $action ne $c->maketext('Overwrite')) + if ( + ($invalidUploadFilenameMsg || $isProtected) + || (!$c->param('overwrite') + && $action ne 'Overwrite' + && $action ne $c->maketext('Overwrite')) + ) { return $c->c( $c->Confirm(