Skip to content

Commit

Permalink
Prevent protected files from being modified by unprivileged users.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@drgrice1
drgrice1 committed Jul 2, 2024
1 parent f146c11 commit 4470b83
Showing 1 changed file with 95 additions and 5 deletions.
100 changes: 95 additions & 5 deletions lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.'));
Expand Down Expand Up @@ -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)) {
Expand All @@ -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) };
Expand Down Expand Up @@ -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$/) {
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 <b>[_1]</b> already exists. Overwrite it, or rename it as:', $name))
$c->b(
$isProtected
? $c->maketext('File <b>[_1]</b> is protected and cannot be overwritten. Rename it as:',
$name)
: $c->maketext('File <b>[_1]</b> 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});
Expand Down

0 comments on commit 4470b83

Please sign in to comment.