Fix a vulnerability in the Rserve code. #1229
Open
Conversation
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
|
Note this is built on top of #1213. |
The primary change in this pull request is how the `rserve_get_file` method in the `RserveClient.pl` macro works. Previously it accepted a second optional local filename parameter. That parameter is no longer honored. If it is given it is ignored. I don't think that there are any problems in the OPL or Contrib that pass that argument, but even if they do it will still work. The filename just won't be the chosen one. The filename should not be a problem author choice. Furthermore, previously the method fell back to using the Rserve remote filename. That is also no longer used. Instead, the `getUniqueName` method is used that creates a filename that is dependent on the problem seed, psvn, problem UUID, etc. That means that the filename will not change each time that the problem is loaded. Unfortunately, the file is still downloaded every time the problem is loaded and overwrites the existing file (with the same contents), and that can't really be fixed without a major revision of the Rserve setup. Note that all changes in this pull request are backwards compatible, and all existing problems will continue to function correctly. However, the courses html temporary directory will no longer be filled with files that are only used once before another copy of the file is created by another name the next time the problem is rendered (due to a preview, answer submission, etc.). In addition there are two new methods added that are more convenient than the previous methods for generating R images or csv files. Those are the `rserve_data_url` and `rserve_plot` methods. These handle the details of creating these things in a much nicer way. See the added POD and examples in the POD for the usage and comparison to the old approaches. The macro code and POD are also rather heavily clean up. The `Rserve.pm` module is also cleaned up. The `_inherits` method is removed from the `Rserve.pm` module, because contrary to the comments the `inherits` method of the `$rserve` object does work in the safe compartment, and so the `_inherits` method is not needed. There are a couple of other related changes. First, the `htmlLink` method now has a new calling convention for adding additional attributes to the generated `a` tag. Previously these were added by passing a single string argument. For example, `htmlLink($url, 'dataset', 'download="dataset.csv"')`. That will still work, but passing the attributes in that way should be considered deprecated. Instead the attributes should be passed as additional attribute/value pairs after the url and link text. For example, `htmlLink($url, 'dataset', download => 'dataset.csv')`. Of course, this is useful for Rserve csv files to specify the filename that will be offered when the user clicks to download a csv file. Particularly since the default will now be something like `ed721060-00b2-37fb-87c4-e3eb36c7ced2___5643740d-c34f-37c8-afc5-c2369fec26e1.csv`. Previously it would have been something like `file1231asdfasdf.csv`. The new alias is worse, but that wasn't good either.
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
force-pushed
the
rserve-update-plus-complete-rewrite
branch
from
6d94d6b to
c647fd6
Compare
|
I tried running the PG problem posted above on this branch and getting a 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. |
|
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This requires a change to the
Statistics::R::IO::Rservepackage. The vulnerability is directly in that package as itsget_filemethod 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::IOpackage 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 theStatistics::R::IOpackage 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::Rservepackage, but all of the other things that aren't needed by WeBWorK and PG are removed.Since this is part of PG the
get_filemethod can save to the requested file name using theWeBWorK::PG::IO::saveDataFilemethod which refuses to save anything outside of the html temporary directory.Note that this implementation does not use
Class::TinyorClass::Tiny::Antler, so those modules should be removed from the modules that are share to the safe compartment. That is done in theconf/pg_config.dist.ymlfile, 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
Rservepackage and theRserveClient.plmacro. Some of the tests require an R connection. Those tests are skipped if that is not available. Ther-base-coreandr-cran-rserveUbuntu 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