Conversation
drdrew42
force-pushed
the
fix/safe-compartment-shell-injection
branch
from
afe254d to
eebf54d
Compare
| 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. |
There was a problem hiding this comment.
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.
| 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"; |
There was a problem hiding this comment.
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.
|
|
||
| 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; |
There was a problem hiding this comment.
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.
| 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%]+$/; |
There was a problem hiding this comment.
As I mentioned this validation should be done in the $self method. Also,
this should be
| 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.
2 participants
Please consider this commit for hotfix.