Skip to content

Commit 1ac1182

Browse files
drgrice1claude
andcommitted
Fix several more safe compartment vulnerabilities.
First, `HTML::Parser` was shared into the safe compartment. Its `parse_file` method opens and reads whatever path it is given, bypassing the permitted_read_dir restriction. Thus giving any PG problem arbitrary file read. It is not actually used by PG, and so that is removed from the modules that are shared. Note that the `HTML::Entities` package which is part of the `HTML::Parser` package on CPAN is still shared and is used. Second, the `PGloadfiles::compile_file` method compiles whatever file it is given, and so a problem could call it directly (bypassing `findMacroFile`'s restriction of `$filePath` to being in a directory in the `$macrosPath` array) to compile and execute an arbitrary file. So the `compile_file` method now validates the file it is asked to compile using the same restriction as the findMacroFile method. Although this cannot be checked against `$self->{envir}` and `$self->{pwd}`, since problem code can modify those. Instead `WeBWorK::PG` now saves the macrosPath and problem directory into `WeBWorK::PG::IO` which is not exposed to the safe compartment, and the `compile_file` method uses those. Also fix WeBWorK::PG::IO::path_is_subdir to reject an empty or undefined directory argument. Previously that normalized (via canonpath) to '/', which every absolute path matches, silently turning a "restrict to this directory" check into "allow anything". Third, restrict `PGalias::alias_for_tex` to reading files in allowed locations. `alias_for_tex`, used when a problem is rendered in hardcopy, did not check that an absolute path passed to it was located in an allowed location before using it, unlike `alias_for_html` which routes such paths through `create_link_to_tmp_file`'s `permitted_read_dir` check. Since `alias` reaches `alias_for_tex` directly for TeX mode, and methods like `image` embed its return value straight into \includegraphics, any problem could get an arbitrary file on the server embedded into its generated hardcopy PDF. Fourth, restrict the GD::Image file-path-taking methods to permitted_read_dir. GD is shared into the safe compartment for graphing macros. Several of its methods (new, newFromPng, newFromJpeg, newFromGd, newFromXpm, and others) open a given file path directly with no restriction, bypassing WeBWorK::PG::IO's permitted_read_dir. Add WeBWorK::PG::SafeGD, which patches the GD::Image symbol table once after GD loads so that these methods reject a path outside permitted_read_dir before touching the filesystem. In their current form these methods can be used in a problem to implement a file existence check, and furthermore can reveal if a file exists but the server user does not have permission to access them. Also, fix an incorrectly quoted string in `AnswerHash.pm`. The backtick quoted string would have been evaluated as a shell command. This only would occur if the `debug` key is set on an `AnswerHash` object. But it should not be backtick quoted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent ca2ea4d commit 1ac1182

8 files changed

Lines changed: 149 additions & 12 deletions

File tree

conf/pg_config.dist.yml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,7 +187,6 @@ options:
187187
modules:
188188
- [Encode]
189189
- ['Encode::Encoding']
190-
- ['HTML::Parser']
191190
- ['HTML::Entities']
192191
- [Exporter]
193192
- [GD]

lib/AnswerHash.pm

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -594,7 +594,7 @@ sub evaluate {
594594
}
595595
$rh_ans = $self->dereference_array_ans($rh_ans);
596596
# make sure that the student answer is not an array so that it is reported correctly in answer section.
597-
eval(q!main::DEBUG_MESSAGE( `<h4>final result: </h4>`, pretty_print($rh_ans,'html'))!)
597+
eval(q!main::DEBUG_MESSAGE('<h4>final result: </h4>', pretty_print($rh_ans,'html'))!)
598598
if defined($self->{debug})
599599
and $self->{debug} > 0;
600600
# re-reference $rh_ans;

lib/PGalias.pm

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,8 +211,14 @@ sub alias_for_tex {
211211
} elsif ($file_path =~ m|^$self->{htmlDirectory}|) {
212212
# File is in the course html directory.
213213
$resource_object->path($aux_file_id);
214-
} else {
214+
} elsif (WeBWorK::PG::IO::path_is_subdir(
215+
$file_path, $WeBWorK::PG::IO::pg_envir->{directories}{permitted_read_dir}, 1
216+
))
217+
{
215218
$resource_object->path($file_path);
219+
} else {
220+
$self->warning_message(qq{Unable to use the file "$file_path" because it is an unsafe path.});
221+
return '';
216222
}
217223

218224
if ($ext eq 'gif' || $ext eq 'svg') {

lib/PGloadfiles.pm

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,11 @@ sub compile_file {
209209
my $self = shift;
210210
my $filePath = shift;
211211

212+
# Only allow compilation of files that are in the macros path.
213+
my @allowedDirs = map { $_ eq '.' ? $WeBWorK::PG::IO::pwd : $_ } @{ $WeBWorK::PG::IO::macrosPath // [] };
214+
die "Refusing to compile $filePath as it is not located in an allowed location.\n"
215+
unless grep { WeBWorK::PG::IO::path_is_subdir($filePath, $_) } @allowedDirs;
216+
212217
warn "loading $filePath" if $debugON;
213218

214219
local $/ = undef; # allows us to treat the file as a single line

lib/WeBWorK/PG.pm

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,23 @@ sub defineProblemEnvironment ($pg_envir, $options = {}) {
192192
$specialPGEnvironmentVars->{$_} = $options->{specialPGEnvironmentVars}{$_}
193193
for keys %{ $options->{specialPGEnvironmentVars} };
194194

195+
# Save the macrosPath array and problem directory into WeBWorK::PG::IO. This is never exposed into the safe
196+
# compartment. So PGloadfiles::compile_file can use it to ensure it is not asked to compile files elsewhere.
197+
198+
# The contents of the $macrosPath array must be copied. $macrosPath itself is shared to the safe compartment,
199+
# and problem code can change its contents. So a reference is not sufficient.
200+
my $macrosPath = $options->{macrosPath} // $pg_envir->{directories}{macrosPath};
201+
$WeBWorK::PG::IO::macrosPath = [@$macrosPath];
202+
203+
# This is the same $pwd construction that PGloadfiles.pm uses.
204+
my $probFileName = $options->{sourceFilePath} // '';
205+
my $templateDirectory = $options->{templateDirectory} // '';
206+
my $pwd = $probFileName;
207+
$pwd =~ s!/[^/]*$!!;
208+
$pwd = $templateDirectory . $pwd unless substr($pwd, 0, 1) eq '/';
209+
$pwd =~ s!/tmpEdit/!/!;
210+
$WeBWorK::PG::IO::pwd = $pwd;
211+
195212
return {
196213
# This copies everything from the provided options that are not explicitly dealt with below.
197214
# With this the caller can add any desired key value pairs to the translator environment.
@@ -202,7 +219,7 @@ sub defineProblemEnvironment ($pg_envir, $options = {}) {
202219
# value, or just hard coded defaults.
203220

204221
# Problem information
205-
probFileName => $options->{sourceFilePath} // '',
222+
probFileName => $probFileName,
206223
displayMode => DISPLAY_MODES()->{ $options->{displayMode} || 'MathJax' } // 'HTML_MathJax',
207224
problemSeed => $options->{problemSeed} || 1234,
208225
psvn => $options->{psvn} // 1,
@@ -243,14 +260,14 @@ sub defineProblemEnvironment ($pg_envir, $options = {}) {
243260

244261
# Directories and URLs
245262
pgMacrosDir => "$pg_envir->{directories}{root}/macros",
246-
macrosPath => $options->{macrosPath} // $pg_envir->{directories}{macrosPath},
247-
htmlPath => $options->{htmlPath} // $pg_envir->{URLs}{htmlPath},
248-
imagesPath => $options->{imagesPath} // $pg_envir->{URLs}{imagesPath},
249-
htmlDirectory => $options->{htmlDirectory} // "$pg_envir->{directories}{html}/",
250-
htmlURL => $options->{htmlURL} // "$pg_envir->{URLs}{html}/",
251-
templateDirectory => $options->{templateDirectory} // '',
252-
tempURL => $options->{tempURL} // "$pg_envir->{URLs}{tempURL}/",
253-
localHelpURL => $options->{localHelpURL} // "$pg_envir->{URLs}{localHelpURL}/",
263+
macrosPath => $macrosPath,
264+
htmlPath => $options->{htmlPath} // $pg_envir->{URLs}{htmlPath},
265+
imagesPath => $options->{imagesPath} // $pg_envir->{URLs}{imagesPath},
266+
htmlDirectory => $options->{htmlDirectory} // "$pg_envir->{directories}{html}/",
267+
htmlURL => $options->{htmlURL} // "$pg_envir->{URLs}{html}/",
268+
templateDirectory => $templateDirectory,
269+
tempURL => $options->{tempURL} // "$pg_envir->{URLs}{tempURL}/",
270+
localHelpURL => $options->{localHelpURL} // "$pg_envir->{URLs}{localHelpURL}/",
254271

255272
# Other things ...
256273

lib/WeBWorK/PG/IO.pm

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,10 @@ sub remove_tree {
300300
sub path_is_subdir {
301301
my ($path, $dir, $allow_relative) = @_;
302302

303+
# An empty or undefined $dir normalizes via canonpath to '/', which every absolute path matches,
304+
# turning "restrict to this directory" into "allow anything". Reject up front instead.
305+
return 0 unless defined $dir && $dir ne '';
306+
303307
unless ($path =~ /^\//) {
304308
if ($allow_relative) {
305309
$path = "$dir/$path";

lib/WeBWorK/PG/SafeGD.pm

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
package WeBWorK::PG::SafeGD;
2+
3+
=head1 NAME
4+
5+
WeBWorK::PG::SafeGD - Restrict GD::Image new method file path arguments to
6+
permitted_read_dir.
7+
8+
=head1 DESCRIPTION
9+
10+
GD is shared into the safe compartment for graphing macros (via WWPlot and the
11+
PGgraphmacros.pl macro). Several of the GD::Image methods take a file path
12+
argument and open it directly, with no restriction (C<new>, C<newFromPng>,
13+
C<newFromJpeg>, C<newFromGif>, C<newFromTiff>, C<newFromXbm>, C<newFromWebp>,
14+
C<newFromHeif>, C<newFromWBMP>, C<newFromBmp>, C<newFromGd>, C<newFromGd2>,
15+
C<newFromGd2Part>, and C<newFromXpm>). The C<restrict> method in this package
16+
ensures that if those methods are called on an unsafe path (a path not in the
17+
C<permitted_read_dir>), the methods do not reveal anything about the existence
18+
or lack thereof for the file path argument. The C<WWPlot> package does not use
19+
these path-taking forms (only the numeric-size constructor is used).
20+
21+
C<restrict> patches the GD::Image symbol table in place, so it only needs to run
22+
once per process, after GD itself has been loaded.
23+
24+
=cut
25+
26+
use strict;
27+
use warnings;
28+
29+
use WeBWorK::PG::IO;
30+
31+
my $patched = 0;
32+
33+
# Only reject arguments that look like they're meant to be a path (a plain string, not an
34+
# already-open filehandle/IO object) and that GD would otherwise try to open unrestricted.
35+
sub _unsafe_path {
36+
my $path = shift;
37+
return 0 if ref $path;
38+
return 0 unless defined $path && length $path;
39+
return !WeBWorK::PG::IO::path_is_subdir($path, $WeBWorK::PG::IO::pg_envir->{directories}{permitted_read_dir});
40+
}
41+
42+
sub restrict {
43+
return if $patched || !GD::Image->can('_make_filehandle');
44+
$patched = 1;
45+
46+
no warnings qw(redefine prototype);
47+
48+
# Every newFrom* method implemented in GD/Image.pm other than the XS methods (Png, Jpeg, Gif, Tiff, Xbm, Webp, Heif,
49+
# WBMP, and Bmp) call _make_filehandle. The new method does as well, but is wrapped separately below, since it
50+
# touches the filesystem before calling this.
51+
my $orig_make_filehandle = \&GD::Image::_make_filehandle;
52+
*GD::Image::_make_filehandle = sub {
53+
die "GD: refusing to open \"$_[1]\" as it is not in an allowed location.\n" if _unsafe_path($_[1]);
54+
goto &$orig_make_filehandle;
55+
};
56+
57+
# The single argument form of new executes -f tests on the given file path argument before it calls
58+
# _make_filehandle. Skip the check only when the argument is recognized as raw image data rather than a path at all,
59+
# since then no file access happens anywhere. Otherwise $! is set for non-existent files, and so this can be used
60+
# for a file existence test in a problem.
61+
my $orig_new = \&GD::Image::new;
62+
*GD::Image::new = sub {
63+
die "GD: refusing to open \"$_[1]\" as it is not in an allowed location.\n"
64+
if @_ == 2 && !ref $_[1] && !GD::Image::_image_type($_[1]) && _unsafe_path($_[1]);
65+
goto &$orig_new;
66+
};
67+
68+
# These are implemented in XS and take a file path directly, bypassing _make_filehandle.
69+
no strict 'refs';
70+
for my $method (qw(newFromGd newFromGd2 newFromGd2Part newFromXpm)) {
71+
next unless GD::Image->can($method);
72+
my $orig = \&{"GD::Image::$method"};
73+
*{"GD::Image::$method"} = sub {
74+
die "GD: refusing to open \"$_[1]\" as it is not in an allowed location.\n" if _unsafe_path($_[1]);
75+
goto &$orig;
76+
};
77+
}
78+
use strict 'refs';
79+
80+
# stringFT's font file argument is only restricted when it looks like an absolute path, since it may legitimately be
81+
# a relative name or fontconfig pattern (e.g. after useFontConfig) instead of a path.
82+
if (GD::Image->can('stringFT')) {
83+
my $orig_string_ft = \&GD::Image::stringFT;
84+
*GD::Image::stringFT = sub {
85+
die "GD: refusing to open \"$_[2]\" as it is not in an allowed location.\n"
86+
if defined $_[2] && !ref $_[2] && $_[2] =~ m{^/} && _unsafe_path($_[2]);
87+
goto &$orig_string_ft;
88+
};
89+
# stringTTF is a plain alias for stringFT set up when GD::Image was loaded, so it still points
90+
# to the original, unwrapped sub unless it is re-aliased here.
91+
*GD::Image::stringTTF = \&GD::Image::stringFT;
92+
}
93+
94+
use warnings qw(redefine prototype);
95+
96+
return;
97+
}
98+
99+
1;

lib/WeBWorK/PG/Translator.pm

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ use WWSafe;
5757
use PGUtil qw(pretty_print);
5858
use WeBWorK::PG::IO qw(fileFromPath);
5959
use WeBWorK::PG::SafeIOHandle;
60+
use WeBWorK::PG::SafeGD;
6061

6162
BEGIN {
6263
# Setup the safe compartment for the standalone renderer.
@@ -98,6 +99,9 @@ BEGIN {
9899
# functions of any installed shared library directly.
99100
$safeCache->share_empty_package('DynaLoader');
100101

102+
# Restrict the GD::Image methods that take a file path argument (new, newFromPng, etc.).
103+
WeBWorK::PG::SafeGD::restrict();
104+
101105
my $store_mask = $safeCache->mask();
102106
$safeCache->mask(Opcode::empty_opset());
103107
my $safe_cmpt_package_name = $safeCache->root();
@@ -283,6 +287,9 @@ sub initialize {
283287
unless (exists($ENV{MOJO_MODE})) {
284288
$safe_cmpt->share_from('main', $self->{ra_included_modules});
285289
$safe_cmpt->share_empty_package('DynaLoader');
290+
291+
# Restrict the GD::Image methods that take a file path argument (new, newFromPng, etc.).
292+
WeBWorK::PG::SafeGD::restrict();
286293
}
287294

288295
return;

0 commit comments

Comments
 (0)