Skip to content

Commit 0c28eae

Browse files
committed
Sanitize the courseID from either the URL path or the request parameters.
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 [email protected], 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.
1 parent 6395021 commit 0c28eae

File tree

2 files changed

+16
-8
lines changed

2 files changed

+16
-8
lines changed

lib/WeBWorK/CourseEnvironment.pm

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ sub new {
9696
$seedVars->{pg_dir} //= $WeBWorK::SeedCE{pg_dir} // $ENV{PG_ROOT};
9797

9898
$seedVars->{courseName} ||= '___'; # prevents extraneous error messages
99+
$seedVars->{courseName} =~ s/'.*$//;
99100

100101
# The following line is a work around for a bug that occurs on some systems. See
101102
# https://rt.cpan.org/Public/Bug/Display.html?id=77916 and

lib/WeBWorK/Utils/Routes.pm

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ my %routeParameters = (
296296
logout options instructor_tools problem_list)
297297
],
298298
module => 'ProblemSets',
299-
path => '/#courseID'
299+
path => { '/#courseID' => [ courseID => qr/[\w-]*/ ] }
300300
},
301301

302302
logout => {
@@ -429,7 +429,7 @@ my %routeParameters = (
429429
instructor_problem_grader => {
430430
title => x('Manual Grader'),
431431
module => 'Instructor::ProblemGrader',
432-
path => '/grader/#setID/#problemID'
432+
path => '/grader/#setID/<problemID:num>'
433433
},
434434
instructor_add_users => {
435435
title => x('Add Users'),
@@ -471,7 +471,7 @@ my %routeParameters = (
471471
instructor_problem_editor_withset_withproblem => {
472472
title => '[_3]',
473473
module => 'Instructor::PGProblemEditor',
474-
path => '/#problemID'
474+
path => '/<problemID:num>'
475475
},
476476
instructor_scoring => {
477477
title => x('Scoring Tools'),
@@ -503,7 +503,7 @@ my %routeParameters = (
503503
instructor_problem_statistics => {
504504
title => '[_3]',
505505
module => 'Instructor::Stats',
506-
path => '/#problemID'
506+
path => '/<problemID:num>'
507507
},
508508
instructor_user_statistics => {
509509
title => '[_1]',
@@ -570,7 +570,7 @@ my %routeParameters = (
570570
title => '[_3]',
571571
children => [qw(show_me_another)],
572572
module => 'Problem',
573-
path => '/#problemID',
573+
path => '/<problemID:num>',
574574
unrestricted => 1
575575
},
576576
show_me_another => {
@@ -617,15 +617,22 @@ sub setup_content_generator_routes_recursive {
617617
my $action = $routeParameters{$child}{action} // 'go';
618618

619619
if ($routeParameters{$child}{children}) {
620-
my $child_route = $route->under($routeParameters{$child}{path}, [ problemID => qr/\d+/ ])->name($child);
620+
my $child_route = $route->under(
621+
ref($routeParameters{$child}{path}) eq 'HASH'
622+
? %{ $routeParameters{$child}{path} }
623+
: $routeParameters{$child}{path})->name($child);
621624
$child_route->any($routeParameters{$child}{methods} // (), '/')->to("$routeParameters{$child}{module}#$action")
622625
->name($child);
623626
for (@{ $routeParameters{$child}{children} }) {
624627
setup_content_generator_routes_recursive($child_route, $_);
625628
}
626629
} else {
627-
$route->any($routeParameters{$child}{methods} // (), $routeParameters{$child}{path}, [ problemID => qr/\d+/ ])
628-
->to("$routeParameters{$child}{module}#$action")->name($child);
630+
$route->any(
631+
$routeParameters{$child}{methods} // (),
632+
ref($routeParameters{$child}{path}) eq 'HASH'
633+
? %{ $routeParameters{$child}{path} }
634+
: $routeParameters{$child}{path}
635+
)->to("$routeParameters{$child}{module}#$action")->name($child);
629636
}
630637

631638
return;

0 commit comments

Comments
 (0)