Skip to content

Commit c77d08a

Browse files
drgrice1drgrice1
committed
Harden system calls plus.
This does the things that I asked @drdrew42 to address in openwebwork#1392. Since @drdrew42 seems to not have time to deal with that (due to the lack of response to my comments) I am opening this pull request. This also addresses the fourth security vulnerability that @drdrew42 mentioned in the Slack securityresponseteam channel. Basically, it only allows symlinks to be created to a file that is in the `$WeBWorK::PG::IO::pg_envir->{directories}{permitted_read_dir}`. That does allow following symlinks, so for webwork2 that includes anything in or linked to in the course directory. That means OPL static image files will work, or files in the `webwork2/assets/pg/Student_Orientation` directory (for example). One case that was allowed before that this doesn't allow is a file in the `webwork2/htdocs/images` directory. I don't really see a need to allow those files though. For the standalone renderer that is the root directory of the standalone renderer app (by default). This could be considered for a hotfix as @drdrew42 mentioned in openwebwork#1392.
1 parent c379851 commit c77d08a

2 files changed

Lines changed: 53 additions & 32 deletions

File tree

‎lib/LaTeXImage.pm‎

Lines changed: 32 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,29 @@ sub new {
3636
# be passed to the command line in the system call it is used in.
3737
if ($field eq 'ext') {
3838
$data->{ext} = $value if $value && ($value =~ /^(png|gif|svg|pdf|tgz)$/);
39+
} elsif ($field eq 'convertOptions') {
40+
if (ref $value eq 'HASH') {
41+
# Validate convertOptions keys and values to prevent shell injection.
42+
# Only alphanumeric option names and simple values are permitted.
43+
my $options = { input => {}, output => {} };
44+
for my $phase (keys %$options) {
45+
next unless ref $value->{$phase} eq 'HASH';
46+
for my $key (keys %{ $value->{$phase} }) {
47+
unless ($key =~ /^[a-zA-Z][a-zA-Z0-9\-]*$/) {
48+
warn "Invalid convert option name: $key";
49+
next;
50+
}
51+
unless (defined $value->{$phase}{$key}
52+
&& $value->{$phase}{$key} =~ /^[a-zA-Z0-9.\-+:x%]+$/)
53+
{
54+
warn "Invalid convert option value for $key";
55+
next;
56+
}
57+
$options->{$phase}{$key} = $value->{$phase}{$key};
58+
}
59+
}
60+
$data->{convertOptions} = $options;
61+
}
3962
} else {
4063
$data->{$field} = $value;
4164
}
@@ -277,11 +300,10 @@ sub use_svgMethod {
277300
# Validate svgMethod against known SVG converters to prevent command injection.
278301
my $method = $self->svgMethod;
279302
if ($method eq 'dvisvgm') {
280-
my $cmd = WeBWorK::PG::IO::externalCommand('dvisvgm');
281-
system {$cmd} $cmd, "$working_dir/image.dvi", '--no-fonts', "--output=$working_dir/image.svg";
303+
system WeBWorK::PG::IO::externalCommand('dvisvgm'), "$working_dir/image.dvi", '--no-fonts',
304+
"--output=$working_dir/image.svg";
282305
} elsif ($method eq 'pdf2svg') {
283-
my $cmd = WeBWorK::PG::IO::externalCommand('pdf2svg');
284-
system {$cmd} $cmd, "$working_dir/image.pdf", "$working_dir/image.svg";
306+
system WeBWorK::PG::IO::externalCommand('pdf2svg'), "$working_dir/image.pdf", "$working_dir/image.svg";
285307
} else {
286308
warn "Unknown svgMethod '$method'. Must be 'dvisvgm' or 'pdf2svg'.";
287309
return;
@@ -294,23 +316,12 @@ sub use_svgMethod {
294316
sub use_convert {
295317
my ($self, $working_dir, $ext) = @_;
296318

297-
# Validate convertOptions keys and values to prevent shell injection.
298-
# Only alphanumeric option names and simple values are permitted.
299-
my @args;
300-
for my $phase (qw(input output)) {
301-
my $opts = $self->convertOptions->{$phase} // {};
302-
for my $key (keys %$opts) {
303-
warn("Invalid convert option name: $key"), next unless $key =~ /^[a-zA-Z][a-zA-Z0-9\-]*$/;
304-
my $val = $opts->{$key};
305-
warn("Invalid convert option value for $key"), next unless defined $val && $val =~ /^[a-zA-Z0-9.\-+:x%]+$/;
306-
push @args, "-$key", $val;
307-
}
308-
push @args, "$working_dir/image.pdf" if $phase eq 'input';
309-
}
310-
push @args, "$working_dir/image.$ext";
311-
312-
my $convert = WeBWorK::PG::IO::externalCommand('convert');
313-
system {$convert} $convert, @args;
319+
my $convertOptions = $self->convertOptions;
320+
system WeBWorK::PG::IO::externalCommand('convert'),
321+
(map { ("-$_" => $convertOptions->{input}{$_}) } keys %{ $convertOptions->{input} }),
322+
"$working_dir/image.pdf",
323+
(map { ("-$_" => $convertOptions->{output}{$_}) } keys %{ $convertOptions->{output} }),
324+
"$working_dir/image.$ext";
314325
warn "Failed to generate $ext file." unless -r "$working_dir/image.$ext";
315326

316327
return;

‎lib/PGalias.pm‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -238,17 +238,27 @@ sub create_link_to_tmp_file {
238238
my $linkPath = $self->surePathToTmpFile($link);
239239

240240
if (-e $resource_object->path) {
241-
if (-e $linkPath) {
242-
# Destroy the old link.
243-
unlink($linkPath) or $self->warning_message(qq{Unable to unlink alias file at "$linkPath".});
244-
}
245-
246-
# Create a new link, and the URI to this link.
247-
if (symlink($resource_object->path, $linkPath)) {
248-
$resource_object->uri(($self->{tempURL} =~ s|/$||r) . "/$link");
241+
if (WeBWorK::PG::IO::path_is_subdir(
242+
$resource_object->path, $WeBWorK::PG::IO::pg_envir->{directories}{permitted_read_dir}, 1
243+
))
244+
{
245+
if (-e $linkPath) {
246+
# Destroy the old link.
247+
unlink($linkPath) or $self->warning_message(qq{Unable to unlink alias file at "$linkPath".});
248+
}
249+
250+
# Create a new link, and the URI to this link.
251+
if (symlink($resource_object->path, $linkPath)) {
252+
$resource_object->uri(($self->{tempURL} =~ s|/$||r) . "/$link");
253+
} else {
254+
$self->warning_message(
255+
qq{The macro alias cannot create a link from "$linkPath" to "} . $resource_object->path . '"');
256+
}
249257
} else {
250-
$self->warning_message(
251-
qq{The macro alias cannot create a link from "$linkPath" to "} . $resource_object->path . '"');
258+
$self->warning_message('Unable to create link to the file "'
259+
. $resource_object->path . '" '
260+
. 'because it is an unsafe path.');
261+
252262
}
253263
} else {
254264
$self->warning_message('Cannot find the file: "'
@@ -270,7 +280,7 @@ sub convert_file_to_png_for_tex {
270280
$self->debug_message('convert filePath ', $sourceFilePath, "\n");
271281

272282
my $conversion_command = WeBWorK::PG::IO::externalCommand('convert');
273-
my $returnCode = system {$conversion_command} $conversion_command, "${sourceFilePath}[0]", $targetFilePath;
283+
my $returnCode = system $conversion_command, "${sourceFilePath}[0]", $targetFilePath;
274284
if ($returnCode || !-e $targetFilePath) {
275285
$resource_object->warning_message(
276286
qq{Failed to convert "$sourceFilePath" to "$targetFilePath" using "$conversion_command": $!});

0 commit comments

Comments
 (0)