Skip to content

Harden system calls #1392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
drdrew42 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Harden system calls #1392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions lib/LaTeXImage.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,18 @@ sub draw {

sub use_svgMethod {
my ($self, $working_dir) = @_;
if ($self->svgMethod eq 'dvisvgm') {
system WeBWorK::PG::IO::externalCommand('dvisvgm')
. " $working_dir/image.dvi --no-fonts --output=$working_dir/image.svg > /dev/null 2>&1";

# Validate svgMethod against known SVG converters to prevent command injection.
Copy link
Copy Markdown
Member

@drgrice1 drgrice1 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The places that you are doing the validation in this file are not the right places for the validation. It should be done in the $self method when the svgMethod or convertOptions is set. That is defined on lines 32 - 44 of this file. Validation to protect system calls against injection is exactly the point of using a blessed $self method instead of a blessed $self hash, and it is already validating the ext field in this way.

I can't believe that I was this sloppy when the svgMethod and convertOptions were added by not taking care of validation. The ext validation was in the initial code, and those were added later.

my $method = $self->svgMethod;
if ($method eq 'dvisvgm') {
my $cmd = WeBWorK::PG::IO::externalCommand('dvisvgm');
system {$cmd} $cmd, "$working_dir/image.dvi", '--no-fonts', "--output=$working_dir/image.svg";
} elsif ($method eq 'pdf2svg') {
my $cmd = WeBWorK::PG::IO::externalCommand('pdf2svg');
system {$cmd} $cmd, "$working_dir/image.pdf", "$working_dir/image.svg";
Comment on lines +277 to +282
Copy link
Copy Markdown
Member

@drgrice1 drgrice1 Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looking closer at this I see that the svgMethod is not directly used in the system command. It is just the name for the lookup via the WeBWorK::PG::IO::externalCommand method. So if it is set to something that is not a valid externalProgram, then the WeBWorK::PG::IO::externalCommand method will just return undefined. So I don't think extra validation is needed. The elsif doesn't hurt to prevent an exception I suppose.

} else {
system WeBWorK::PG::IO::externalCommand($self->svgMethod)
. " $working_dir/image.pdf $working_dir/image.svg > /dev/null 2>&1";
warn "Unknown svgMethod '$method'. Must be 'dvisvgm' or 'pdf2svg'.";
return;
}
warn "Failed to generate svg file." unless -r "$working_dir/image.svg";

Expand All @@ -285,11 +291,24 @@ sub use_svgMethod {

sub use_convert {
my ($self, $working_dir, $ext) = @_;
system WeBWorK::PG::IO::externalCommand('convert')
. join('', map { " -$_ " . $self->convertOptions->{input}->{$_} } (keys %{ $self->convertOptions->{input} }))
. " $working_dir/image.pdf"
. join('', map { " -$_ " . $self->convertOptions->{output}->{$_} } (keys %{ $self->convertOptions->{output} }))
. " $working_dir/image.$ext > /dev/null 2>&1";

# Validate convertOptions keys and values to prevent shell injection.
# Only alphanumeric option names and simple values are permitted.
my @args;
for my $phase (qw(input output)) {
my $opts = $self->convertOptions->{$phase} // {};
for my $key (keys %$opts) {
warn("Invalid convert option name: $key"), next unless $key =~ /^[a-zA-Z][a-zA-Z0-9\-]*$/;
my $val = $opts->{$key};
warn("Invalid convert option value for $key"), next unless defined $val && $val =~ /^[a-zA-Z0-9.\-+:x%]+$/;
Comment on lines +301 to +303
Copy link
Copy Markdown
Member

@drgrice1 drgrice1 Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned this validation should be done in the $self method. Also,
this should be

Suggested change
warn("Invalid convert option name: $key"), next unless $key =~ /^[a-zA-Z][a-zA-Z0-9\-]*$/;
my $val = $opts->{$key};
warn("Invalid convert option value for $key"), next unless defined $val && $val =~ /^[a-zA-Z0-9.\-+:x%]+$/;
unless ($key =~ /^[a-zA-Z][a-zA-Z0-9\-]*$/) {
warn("Invalid convert option name: $key");
next;
}
my $val = $opts->{$key};
unless (defined $val && $val =~ /^[a-zA-Z0-9.\-+:x%]+$/) {
warn("Invalid convert option value for $key");
next;
}

Don't use commas to separate statements. This is bad practice. See https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitCommaSeparatedStatements.

push @args, "-$key", $val;
}
push @args, "$working_dir/image.pdf" if $phase eq 'input';
}
push @args, "$working_dir/image.$ext";

my $convert = WeBWorK::PG::IO::externalCommand('convert');
system {$convert} $convert, @args;
warn "Failed to generate $ext file." unless -r "$working_dir/image.$ext";

return;
Expand Down
2 changes: 1 addition & 1 deletion lib/PGalias.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ sub convert_file_to_png_for_tex {
$self->debug_message('convert filePath ', $sourceFilePath, "\n");

my $conversion_command = WeBWorK::PG::IO::externalCommand('convert');
my $returnCode = system "$conversion_command '${sourceFilePath}[0]' $targetFilePath";
my $returnCode = system {$conversion_command} $conversion_command, "${sourceFilePath}[0]", $targetFilePath;
Copy link
Copy Markdown
Member

@drgrice1 drgrice1 Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is really any benefit or added security by using system {$conversion_command} $conversion_command, "${sourceFilePath}[0]", $targetFilePath versus just system $conversion_command, "${sourceFilePath}[0]", $targetFilePath.

This is true for all of the places where you have done this. Since the program name is the same it won't actually do anything different. It is just making the code line longer.

There is benefit to switching from the pure string argument to the list argument since it avoids invoking the shell and thus avoids potential shell expansion, but adding the program name does not add anything since they are the same.

if ($returnCode || !-e $targetFilePath) {
$resource_object->warning_message(
qq{Failed to convert "$sourceFilePath" to "$targetFilePath" using "$conversion_command": $!});
Expand Down
39 changes: 31 additions & 8 deletions lib/WeBWorK/PG/IO.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,17 +316,40 @@ sub externalCommand {
# Isolate the call to the sage server in case we have to jazz it up.
sub query_sage_server {
my ($python, $url, $accepted_tos, $setSeed, $webworkfunc, $debug, $curlCommand) = @_;
my $sagecall =
qq{$curlCommand -i -k -sS -L }
. qq{--data-urlencode "accepted_tos=${accepted_tos}" }
. qq{--data-urlencode 'user_expressions={"WEBWORK":"_webwork_safe_json(WEBWORK)"}' }
. qq{--data-urlencode "code=${setSeed}${webworkfunc}$python" $url};

my $output = `$sagecall`;
# Validate url to prevent shell injection — must look like a URL.
if ($url && $url !~ m{^https?://[^\s;|&`\$]+$}) {
warn "query_sage_server: invalid url rejected: $url";
return;
}

# Use system LIST form via open-pipe to capture output without shell interpolation.
my @cmd = (
$curlCommand, '-i',
'-k', '-sS',
'-L', '--data-urlencode',
"accepted_tos=$accepted_tos", '--data-urlencode',
'user_expressions={"WEBWORK":"_webwork_safe_json(WEBWORK)"}', '--data-urlencode',
"code=${setSeed}${webworkfunc}$python", $url // (),
);

if ($debug) {
warn "debug is turned on in IO.pm. ";
warn "\n\nIO::query_sage_server(): SAGE CALL: ", $sagecall, "\n\n";
warn "\n\nRETURN from sage call \n", $output, "\n\n";
warn "\n\nIO::query_sage_server(): SAGE CMD: ", join(' ', @cmd), "\n\n";
}

my $output = '';
if (open(my $pipe, '-|', @cmd)) {
local $/;
$output = <$pipe>;
close $pipe;
} else {
warn "query_sage_server: failed to execute curl: $!";
return;
}

if ($debug) {
warn "\n\nRETURN from sage call \n", $output, "\n\n";
warn "\n\n END SAGE CALL";
}

Expand Down
Loading