Skip to content

Fix a vulnerability in the Rserve code. #1229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: PG-2.20
Choose a base branch
from

Conversation

drgrice1
Copy link
Member

@drgrice1 drgrice1 commented Apr 14, 2025

This requires a change to the Statistics::R::IO::Rserve package. The vulnerability is directly in that package as its get_file method allows the caller to pass an arbitrary local file name, and it will retrieve the remove file and save it to that file as long as the serve has write permission to do so.

I have submitted a pull request to the GitHub repository for the Statistics::R::IO package that would work to prevent that, and also emailed the author directly but have received no response. Furthermore, the package has not had any changes since 2017. So at this point I think that we are going to need to consider the Statistics::R::IO package unmaintained and move on from that package.

As such this pull request adds PG's own implementation. It is pretty much the Statistics::R::IO::Rserve package, but all of the other things that aren't needed by WeBWorK and PG are removed.

Since this is part of PG the get_file method can save to the requested file name using the WeBWorK::PG::IO::saveDataFile method which refuses to save anything outside of the html temporary directory.

Note that this implementation does not use Class::Tiny or Class::Tiny::Antler, so those modules should be removed from the modules that are share to the safe compartment. That is done in the conf/pg_config.dist.yml file, but a separate pull request will be needed to do this for webwork2. It is not critical, the package is not dangerous to have shared. Just not needed.

There are unit tests for both the Rserve package and the RserveClient.pl macro. Some of the tests require an R connection. Those tests are skipped if that is not available. The r-base-core and r-cran-rserve Ubuntu packages have been added to the docker build and the GitHub unit test action. Rserve is started by a docker entrypoint script, and in the GitHub action so that the tests are run in the docker container and the GitHub action.

You can use the attached file to test the vulnerability with the develop branch.
r-serve-vulnerability.pg.txt

@drgrice1
Copy link
Member Author

Note this is built on top of #1213.

@drgrice1 drgrice1 force-pushed the rserve-update-plus-complete-rewrite branch from 6d94d6b to c647fd6 Compare April 16, 2025 02:20
@pstaabp
Copy link
Member

pstaabp commented Apr 26, 2025

I tried running the PG problem posted above on this branch and getting a Statistics::R::IO::Rserve not found error. Seems that this module is just Rserve now. Changing that resulted in other errors.

I was trying to test that the file writing is disallowed now. Do you have another version of this problem that will show that this fails to write a file.

@drgrice1
Copy link
Member Author

Yes, the problem is designed to work with the develop branch (or any other branch of webwork). It obviously won't work with this pull request because this removes the Statistics::R::IO::Rserve package from the safe compartment. To use it with this branch just change the Statistics::R::IO::Rserve package usage in the problem to just Rserve which is what the PG package that replaces the Statistics::R::IO::Rserve is called. You will also need to change the to_pl calls to to_perl because I changed that in the package (to_pl was a lame shortening). However, that will still just throw an exception and the problem won't render. You will get the error message ERRORS from evaluating PG file: Write path /your/given/filename is unsafe from within WeBWorK::PG::IO::saveDataToFile

@drgrice1 drgrice1 changed the base branch from develop to PG-2.20 April 29, 2025 12:26
@drgrice1 drgrice1 force-pushed the rserve-update-plus-complete-rewrite branch 3 times, most recently from 1543907 to c96c6ea Compare April 29, 2025 22:46
Copy link
Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the writing outside of the course vulnerability.

@drgrice1 drgrice1 force-pushed the rserve-update-plus-complete-rewrite branch from c96c6ea to 1ecb4ec Compare May 6, 2025 21:54
This requires a change to the `Statistics::R::IO::Rserve` package. The
vulnerability is directly in that package as its `get_file` method
allows the caller to pass an arbitrary local file name, and it will
retrieve the remove file and save it to that file as long as the serve
has write permission to do so.

I have submitted a pull request to the GitHub repository for the
`Statistics::R::IO` package that would work to preven that, and also
emailed the author directly but have received no response.  Furthermore,
the package has not had any changes since 2017.  So at this point I
think that we are going to need to consider the `Statistics::R::IO`
package unmaintained and move on from that package.

As such this pull request adds PG's own implementation. It is pretty
much the `Statistics::R::IO::Rserve` package, but all of the other
things that aren't needed by WeBWorK and PG are removed.

Since this is part of PG the `get_file` method can save to the requested
file name using the `WeBWorK::PG::IO::saveDataFile` method which refuses
to save anything outside of the html temporary directory.

Note that this implementation does not use `Class::Tiny` or
`Class::Tiny::Antler`, so those modules should be removed from the
modules that are share to the safe compartment.  That is done in the
`conf/pg_config.dist.yml` file, but a separate pull request will be
needed to do this for webwork2.  It is not critical, the package is not
dangerous to have shared.  Just not needed.

There are unit tests for both the `Rserve` package and the
`RserveClient.pl` macro. Some of the tests require an R connection.
Those tests are skipped if that is not available.  The `r-base-core` and
`r-cran-rserve` Ubuntu packages have been added to the docker build and
the GitHub unit test action. Rserve is started by a docker entrypoint
script, and in the GitHub action so that the tests are run in the docker
container and the GitHub action.
@drgrice1 drgrice1 force-pushed the rserve-update-plus-complete-rewrite branch from 1ecb4ec to 48e7ea8 Compare May 14, 2025 10:27
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.

None yet

2 participants