Sanitize the courseID from either the URL path or the request parameters.#2691
Merged
Merged
Conversation
…ers. This ensures that only the allowed characters for a courseID are allowed in the URL path. This restriction occurs at the Mojolicious router level. This means that a request like `https://server.edu/webwork2/courseID%22%3E%3Cscript%3Ealert('hello')%3C/script%3E` is just sent to the vanilla "Page not found" page. Note that the URL above (or one like it) comes from a security scan that someone ran, and that this is NOT a security vulnerability. However, it is not handled the best. Currently the above URL results in numerous warnings about the use of an uninitialized value in string concatenation, and then eventually an exception because of a missing database table. None of that is harmful and a URL like that shown can not be manipulated to do anything harmful, but the warnings and exception can be cleaned up. That is dealt with by allowing routes to specify restrictive placeholders that are passed to the `any` or `under` method of the `Mojolicious::Routes::Route` object. For the courseID the restriction is that that portion of the URL must match `qr/[\w-]*/` which is the same restriction used for the courseID when creating a course. I also changed the problemID to use the built in `:num` placeholder type which is equivalent to what was being done before (i.e., `qr/\d+\`). By the way you can run `./bin/webwork2 routes -v` to see all webwork2 routes and the regular expressions that are used to match them. Furthermore, since the courseID can be specified in a request parameter (via the RPC endpoints), the `WeBWorK::CourseEnvironment` chops off any single quotes and everything after them that occur in the passed in `courseName` in the `$seedVars`. The problem is that when the seed variables are `reval`ed into the course environment safe compartment with `$safe->reval("\$$var = '$val';")`, any single quotes in `courseName` end the first single quote in that statement causing a syntax error. The exception from that is ignored because the errors from that `reval` are not caught, but it results in the `courseName` in the course environment being undefined. That is what causes all of the unininitialized warnings mentioned above, and furthermore the exception from the missing database table (because the login page is loaded and tries to look up guest users for the course, but the user table doesn't exist for this undefined course name). This is all the result of investigating the suspected vulnerability posted about in https://webwork.maa.org/moodle/mod/forum/discuss.php?d=8686. The information was emailed to thewebworkproject@gmail.com, and relayed to @dlgin and myself. I have thoroughly analyzed and tested the suspected vulnerability, and can see that it is not. I believe the issue that caused the server to go down was simply the security scanning tool overwhelming the server with to many requests, and the server not being configured properly for rather meager memory limitations.
drgrice1
force-pushed
the
sanitize-course-name
branch
from
April 5, 2025 23:59
3587462 to
0c28eae
Compare
pstaabp
approved these changes
Apr 8, 2025
somiaj
approved these changes
Apr 8, 2025
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
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 ensures that only the allowed characters for a courseID are allowed in the URL path. This restriction occurs at the Mojolicious router level. This means that a request like
https://server.edu/webwork2/courseID%22%3E%3Cscript%3Ealert('hello')%3C/script%3Eis just sent to the vanilla "Page not found" page.
Note that the URL above (or one like it) comes from a security scan that someone ran, and that this is NOT a security vulnerability. However, it is not handled the best. Currently the above URL results in numerous warnings about the use of an uninitialized value in string concatenation, and then eventually an exception because of a missing database table. None of that is harmful and a URL like that shown can not be manipulated to do anything harmful, but the warnings and exception can be cleaned up.
That is dealt with by allowing routes to specify restrictive placeholders that are passed to the
anyorundermethod of theMojolicious::Routes::Routeobject. For the courseID the restriction is that that portion of the URL must matchqr/[\w-]*/which is the same restriction used for the courseID when creating a course. I also changed the problemID to use the built in:numplaceholder type which is equivalent to what was being done before (i.e.,qr/\d+\). By the way you can run./bin/webwork2 routes -vto see all webwork2 routes and the regular expressions that are used to match them.Furthermore, since the courseID can be specified in a request parameter (via the RPC endpoints), the
WeBWorK::CourseEnvironmentchops off any single quotes and everything after them that occur in the passed incourseNamein the$seedVars. The problem is that when the seed variables arerevaled into the course environment safe compartment with$safe->reval("\$$var = '$val';"), any single quotes incourseNameend the first single quote in that statement causing a syntax error. The exception from that is ignored because the errors from thatrevalare not caught, but it results in thecourseNamein the course environment being undefined. That is what causes all of the unininitialized warnings mentioned above, and furthermore the exception from the missing database table (because the login page is loaded and tries to look up guest users for the course, but the user table doesn't exist for this undefined course name).This is all the result of investigating the suspected vulnerability posted about in https://webwork.maa.org/moodle/mod/forum/discuss.php?d=8686. The information was emailed to thewebworkproject@gmail.com, and relayed to @dlgin and myself. I have thoroughly analyzed and tested the suspected vulnerability, and can see that it is not. I believe the issue that caused the server to go down was simply the security scanning tool overwhelming the server with to many requests, and the server not being configured properly for rather meager memory limitations.