From 4470b8342e93c2a797499c47dc061a16945ac4c1 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Tue, 11 Jun 2024 06:43:53 -0500 Subject: [PATCH] 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});