Skip to content

More vulnerability fixes - #1523

Open
drgrice1 wants to merge 2 commits into
openwebwork:developfrom
drgrice1:more-vulnerability-fixes
Open

More vulnerability fixes#1523
drgrice1 wants to merge 2 commits into
openwebwork:developfrom
drgrice1:more-vulnerability-fixes

Conversation

@drgrice1

Copy link
Copy Markdown
Member

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

I will post problems that demonstrate these vulnerabilities in slack.

Note that this is built on top of #1522 since otherwise it would conflict with that pull request.

There will also be a paired pull request to webwork2 that removes the HTML::Parser package from the list of modules shared to PG.

@drgrice1

Copy link
Copy Markdown
Member Author

I believe that with this pull request and #1522, the safe compartment is now properly safe guarded against invalid file IO access.

@drgrice1

Copy link
Copy Markdown
Member Author

Clearly, we need to be more careful about sharing entire packages to the safe compartment.

@drgrice1
drgrice1 force-pushed the more-vulnerability-fixes branch from 9d0937e to d07a37a Compare August 23, 2026 21:36
drgrice1 and others added 2 commits August 23, 2026 16:58
…tment.

First enforce that the trusted Rserve host from the
`WeBWorK::PG::Environment` is used. The `Rserve` package gets this from
the secure `$WeBWorK::PG::IO::pg_envir` variable that is not exposed to
the safe compartment instead of from the `$main::Rserve` variable that
is. The `$main:::Rserve` variable can be modified by the problem author.

Second, prevent PG problems from bootstrapping arbitrary shared
libraries via `DynaLoader`.  The entire `DynaLoader` package was shared
into the safe compartment, so any .pg problem could call
`DynaLoader::dl_load_file` on any shared library already present on
disk, find and install one of its symbols as an XS sub via
`dl_find_symbol/dl_install_xsub`, and call it directly. Doing this with
`POSIX.so`, which ships with every Perl install, gives raw
POSIX::open/read/write/close, bypassing both the 'open' opcode
restriction and `WeBWorK::PG::IO`'s permitted_read_dir restriction
entirely.

The reason `DynaLoader` was shared is because it is in the `GD`
package's `@ISA` array.  The actual `DynaLoader` package is not needed
for PG's usage of `GD` via `WWPlot.pm`, but the `DynaLoader` symbol
can't simply be dropped as it is needed Perl's method resolution for
anything that walks the inheritance chain. So the new
`WWSafe::share_empty_package` creates an empty, disconnected stash under
the compartment's own root satisfying that structural requirement
without exposing any real DynaLoader functionality.

Third, prevent PG problems from reaching arbitrary file descriptors via
`IO::Handle`.  `IO::Handle` was shared wholesale into the safe
compartment for `lib/Rserve.pm`'s use, which means
`IO::Handle->new_from_fd($n, $mode)` can be called directly from problem
code. That constructor can wrap any file descriptor that happens to be
open in the current process. In a long-lived worker process that handles
many different requests over its lifetime, that can include things like
a cached database connection, a shared log file, or even the
`Mojolicous` rendering stream itself.

Sharing only the specific instance methods `lib/Rserve.pm` actually
calls (print, flush, read, close) isn't enough on its own. `Rserve.pm`
blesses its connection socket as an `IO::Handle` from code that runs
nested inside a live render, and a fresh `bless` executing there does
not resolve correctly against a stash that was merely populated with
copies of those subs.  It needs the class name, as seen from inside the
compartment, to actually be an alias to a real package's symbol table.

So first the new `WWSafe::share_package_as($name, $source_pkg)` method
aliases `$name`, as seen from inside the compartment, to a different
real package's symbol table instead of whatever real package happens to
be named `$name` outside it.  The `WeBWorK::PG::SafeIOHandle` package
then aliases only print/flush/ read/close from the real `IO::Handle`,
and nothing else. `WeBWorK::PG::Translator` then aliases the
compartment's view of `IO::Handle` to that narrow stand-in instead of
sharing the real `IO::Handle` package.

Fourth, deny `printf` usage in the safe compartment.  This is not denied
with `print`.  To deny it `prtf` must be added to the `deny` call. I
don't know of a serious vulnerability here.  The point is that `print`
is denied, and so `printf` should also be denied.  Otherwise you could
do what `print` does with `printf`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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>
@drgrice1
drgrice1 force-pushed the more-vulnerability-fixes branch from d07a37a to 1ac1182 Compare August 23, 2026 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant