Skip to content

Strip hooks problem code adds to shared packages in the safe compartment. - #1565

Open
drgrice1 wants to merge 1 commit into
openwebwork:developfrom
drgrice1:safe-better-clean-stash
Open

drgrice1 wants to merge 1 commit into
openwebwork:developfrom
drgrice1:safe-better-clean-stash

Conversation

@drgrice1

Copy link
Copy Markdown
Member

The _clean_stash method deletes the DESTROY, AUTOLOAD, and overload methods defined by compartment code after it runs, so that the code is not executed outside the safe compartment (for example when an object is destroyed), where it would run with the main:: symbol table and could reach privileged subroutines by name. It skipped the shared packages that are aliased into the compartment via share_from (such as the MathObjects Value:: tree), since deleting a hook from one of those would corrupt the package. But that left a problem or macro free to define a hook under a shared namespace (for example a DESTROY method in a new Value::Evil package, or by overwriting an existing overload such as Complex1::(+ in place), and that hook survived the cleanup.

To close this, snapshot_hooks now records the hooks that in shared packages, once the compartment is fully set up but before any problem or macro code runs. _clean_stash then uses that cache to restore any shared hook that problem or macro code overwrote, and strip all hooks that problem or macro code added. So hooks in packages defined inside the safe compartment are stripped as before, but also hooks that are defined inside the safe compartment using basename of the namespace of a shared package are stripped (so Value::Evil is stripped, but not Value::Real).

To keep this cheap, overwritten hooks are found by walking the recorded baseline hook list, and added hooks are looked for only in the shared packages whose key count changed since the baseline, so the (static) shared packages that problem code did not touch are not re-walked. The baseline is recorded at the end of the compartment setup (in the BEGIN block when rendering in a Mojolicious app, and at the end of unrestricted_load otherwise).

This also updates some out of date comments that described the cached safe compartment as being for the "standalone renderer". In general it is used whenever rendering in a Mojolicious app, which now includes webwork2.

drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Oct 23, 2026
This uses the same shared-package defense from the pg pull request
openwebwork/pg#1565.

The `snapshot_hooks` method can be called to record the hooks in the
shared packages once the compartment is fully set up but before the user
provided code runs, and `_clean_stash` then uses that to restore any
shared hook the user supplied code overwrites in place and strip all
hooks the user provided code adds (so a hook in a new `Mojo::Exception::Evil`
package is stripped, but `Mojo::Exception`'s overloads are left alone).
`snapshot_hooks` is called from the `AchievementNotification` task,
which is the only consumer that shares whole package stashes into a
compartment.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Oct 27, 2026
This uses the same shared-package defense from the pg pull request
openwebwork/pg#1565.

The `snapshot_hooks` method can be called to record the hooks in the
shared packages once the compartment is fully set up but before the user
provided code runs, and `_clean_stash` then uses that to restore any
shared hook the user supplied code overwrites in place and strip all
hooks the user provided code adds (so a hook in a new `Mojo::Exception::Evil`
package is stripped, but `Mojo::Exception`'s overloads are left alone).
`snapshot_hooks` is called from the `AchievementNotification` task,
which is the only consumer that shares whole package stashes into a
compartment.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ent.

The `_clean_stash` method deletes the `DESTROY`, `AUTOLOAD`, and
overload methods defined by compartment code after it runs, so that the
code is not executed outside the safe compartment (for example when an
object is destroyed), where it would run with the `main::` symbol table
and could reach privileged subroutines by name.  It skipped the shared
packages that are aliased into the compartment via `share_from` (such as
the MathObjects `Value::` tree), since deleting a hook from one of those
would corrupt the package.  But that left a problem or macro free to
define a hook under a shared namespace (for example a `DESTROY` method
in a new `Value::Evil` package, or by overwriting an existing overload
such as `Complex1::(+` in place), and that hook survived the cleanup.

To close this, `snapshot_hooks` now records the hooks that in shared
packages, once the compartment is fully set up but before any problem or
macro code runs.  `_clean_stash` then uses that cache to restore any
shared hook that problem or macro code overwrote, and strip all hooks
that problem or macro code added.  So hooks in packages defined inside
the safe compartment are stripped as before, but also hooks that are
defined inside the safe compartment using basename of the namespace of a
shared package are stripped (so `Value::Evil` is stripped, but not
`Value::Real`).

To keep this cheap, overwritten hooks are found by walking the recorded
baseline hook list, and added hooks are looked for only in the shared
packages whose key count changed since the baseline, so the (static)
shared packages that problem code did not touch are not re-walked.  The
baseline is recorded at the end of the compartment setup (in the BEGIN
block when rendering in a Mojolicious app, and at the end of
`unrestricted_load` otherwise).

This also updates some out of date comments that described the cached
safe compartment as being for the "standalone renderer". In general it
is used whenever rendering in a Mojolicious app, which now includes
webwork2.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@drgrice1
drgrice1 force-pushed the safe-better-clean-stash branch from 084d44d to 21a4142 Compare October 27, 2026 21:16
drgrice1 added a commit to drgrice1/webwork2 that referenced this pull request Oct 30, 2026
This uses the same shared-package defense from the pg pull request
openwebwork/pg#1565.

The `snapshot_hooks` method can be called to record the hooks in the
shared packages once the compartment is fully set up but before the user
provided code runs, and `_clean_stash` then uses that to restore any
shared hook the user supplied code overwrites in place and strip all
hooks the user provided code adds (so a hook in a new `Mojo::Exception::Evil`
package is stripped, but `Mojo::Exception`'s overloads are left alone).
`snapshot_hooks` is called from the `AchievementNotification` task,
which is the only consumer that shares whole package stashes into a
compartment.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
drgrice1 added a commit to drgrice1/pg that referenced this pull request Nov 2, 2026
The `WWSafe.pm` module is based on version 2.47 of the upstream
`Safe.pm` module, which includes a `_clean_stash` method that deletes
the `DESTROY`, `AUTOLOAD`, and overload methods defined by compartment
code after the compartment code is executed.  This is intended to
prevent that code from running outside of the safe compartment (for
instance when an object is destroyed, since destruction happens outside
the compartment), where it would run with the `main::` symbol table
rather than the compartment's isolated one.

However, this breaks macros that legitimately define such methods.  For
example, the `AUTOLOAD` method of `context::Extensions::Super` in
`contextExtensions.pl` (used by `contextFraction.pl`, `contextUnits.pl`,
and `contextSignificantFigures.pl`) is deleted, which breaks any context
that extends another context, such as using the Units context together
with the SignificantFigures context.

The other improvements from the 2.47 update (the `reval`/`rdo`
reentrancy handling and the wrapping of code references returned from
the compartment) are retained.

This is an alternative, perhaps slightly less secure but yet secure
enough, approach to fixing the issue created with the `contextExtensions.pl`
macro by the updated `WWSafe.pm` module to openwebwork#1564.  Note that this
completely negates openwebwork#1565 as well.  See my comments in
openwebwork#1564 (comment)
about why this is "secure enough".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
drgrice1 added a commit to drgrice1/pg that referenced this pull request Nov 2, 2026
The `WWSafe.pm` module is based on version 2.47 of the upstream
`Safe.pm` module, which includes a `_clean_stash` method that deletes
the `DESTROY`, `AUTOLOAD`, and overload methods defined by compartment
code after the compartment code is executed.  This is intended to
prevent that code from running outside of the safe compartment (for
instance when an object is destroyed, since destruction happens outside
the compartment), where it would run with the `main::` symbol table
rather than the compartment's isolated one.

However, this breaks macros that define such methods.  For example, the
`AUTOLOAD` method of `context::Extensions::Super` in `contextExtensions.pl`
(used by `contextFraction.pl`, `contextUnits.pl`, and `contextSignificantFigures.pl`)
is deleted, which breaks any context that extends another context, such
as using the Units context together with the SignificantFigures context.

The other improvements from the 2.47 update (the `reval`/`rdo`
reentrancy handling and the wrapping of code references returned from
the compartment) are retained.

This is an alternative, perhaps slightly less secure but yet secure
enough, approach to fixing the issue created with the `contextExtensions.pl`
macro by the updated `WWSafe.pm` module to openwebwork#1564.  Note that this
completely negates openwebwork#1565 as well.  See my comments in
openwebwork#1564 (comment)
about why this is "secure enough".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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