Skip to content

Commit e6aa246

Browse files
authored
Merge pull request openwebwork#1523 from drgrice1/more-vulnerability-fixes
More vulnerability fixes
2 parents af7a395 + 1ac1182 commit e6aa246

15 files changed

Lines changed: 251 additions & 16 deletions

conf/pg_config.dist.yml

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,7 @@ options:
187187
modules:
188188
- [Encode]
189189
- ['Encode::Encoding']
190-
- ['HTML::Parser']
191190
- ['HTML::Entities']
192-
- [DynaLoader]
193191
- [Exporter]
194192
- [GD]
195193
- [utf8]
@@ -233,7 +231,6 @@ modules:
233231
- ['Locale::Maketext']
234232
- ['WeBWorK::PG::Localize']
235233
- ['Mojo::JSON']
236-
- ['IO::Handle']
237234
- ['Rserve']
238235
- [DragNDrop]
239236
- ['Types::Serialiser']

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/Rserve.pm

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,17 @@ sub new {
4949

5050
$self->{_autoflush} //= 1;
5151

52+
# When no filehandle was already supplied (i.e. a new connection is about to be opened), the host to connect to is
53+
# always taken from the trusted PG environment configuration, never from whatever server/port a caller passed to
54+
# this constructor. A problem source file or macro file cannot override this, since $WeBWorK::PG::IO::pg_envir is
55+
# not visible to safe compartment code.
56+
if (!defined $self->{fh} && defined $WeBWorK::PG::IO::pg_envir) {
57+
my $host = $WeBWorK::PG::IO::pg_envir->{specialPGEnvironmentVars}{Rserve}{host};
58+
croak 'Rserve is not configured' unless $host;
59+
$self->{server} = $host;
60+
$self->{port} = $WeBWorK::PG::IO::pg_envir->{specialPGEnvironmentVars}{Rserve}{port} // 6311;
61+
}
62+
5263
$self->{server} //= 'localhost';
5364
die q{Attribute 'server' must be scalar value}
5465
if exists($self->{server}) && (!defined $self->server || ref($self->server));

lib/WWSafe.pm

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,33 @@ sub share_from {
265265
$obj->share_record($pkg, $vars) unless $no_record or !$vars;
266266
}
267267

268+
# Creates an empty package stash named $name directly under the compartment's root. This is not connected to any real
269+
# package. This is for packages that must exist so Perl's method resolution can walk through them (e.g. a shared
270+
# module's @ISA lists a package that must be resolvable, even though none of its actual methods should be reachable from
271+
# the compartment) without sharing that package's real contents.
272+
sub share_empty_package {
273+
my ($obj, $name) = @_;
274+
my $root = $obj->root();
275+
no strict 'refs';
276+
*{"${root}::${name}::"} = {};
277+
return;
278+
}
279+
280+
# Aliases $name, as seen from inside the compartment, to a real package's symbol table, rather than to whatever real
281+
# package happens to be named $name outside the compartment. Unlike share_from, which shares specific symbols but leaves
282+
# bless and method resolution for a class needing a fresh bless from code running nested inside a live reval
283+
# unresolvable unless that exact class name is itself an alias to a real package's symbol table, this lets $source_pkg
284+
# (e.g. a small, narrow package built on purpose) stand in for $name so that such a bless resolves correctly, without
285+
# exposing $name's real, unrestricted contents.
286+
sub share_package_as {
287+
my ($obj, $name, $source_pkg) = @_;
288+
my $root = $obj->root();
289+
no strict 'refs';
290+
croak("Package \"$source_pkg\" does not exist") unless keys %{"$source_pkg\::"};
291+
*{"${root}::${name}::"} = \%{"${source_pkg}::"};
292+
return;
293+
}
294+
268295
sub share_record {
269296
my $obj = shift;
270297
my $pkg = shift;

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/SafeIOHandle.pm

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package WeBWorK::PG::SafeIOHandle;
2+
3+
=head1 NAME
4+
5+
WeBWorK::PG::SafeIOHandle - A restricted stand-in for IO::Handle, shared into
6+
the safe compartment.
7+
8+
=head1 DESCRIPTION
9+
10+
L<Rserve> blesses its connection socket as an L<IO::Handle> and uses its
11+
C<print>, C<flush>, C<read>, and C<close> methods. If the C<IO::Handle> package
12+
is shared directly, then all of its methods are exposed. In particular the
13+
C<new_from_fd> method is exposed (in addition to other potentially dangerous
14+
methods such as C<new>, C<fdopen>, etc.) which allows a PG problem to wrap and
15+
read from or write to any file descriptor the process happens to have open. So
16+
instead this package is shared aliased as the C<IO::Handle> package (via
17+
C<WWSafe::share_package_as>) exposing only the necessary methods.
18+
19+
=cut
20+
21+
use strict;
22+
use warnings;
23+
24+
use IO::Handle;
25+
26+
BEGIN {
27+
no strict 'refs';
28+
*{"WeBWorK::PG::SafeIOHandle::$_"} = \&{"IO::Handle::$_"} for qw(print flush read close);
29+
use strict 'refs';
30+
}
31+
32+
1;

0 commit comments

Comments
 (0)