Vulnerability fixes hotfix of #1522 and #1523. - #1524
Open
drgrice1 wants to merge 2 commits into
Open
Conversation
…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>
somiaj
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See #1522 and #1523 for details.