Fix some security vulnerabilities.

These are security vulnerabilities that @Alex-Jordan identified using
Claude.

1. There was a possible SQL injection vulnerability in the `getDBextras`
   method in `lib/WeBWorK/Utils/ListingDB.pm`.  The `$path` and
   `$filename` could potentially come from user provided parameters and
   were interpolated directly into a double-quoted SQL string with no
   placeholders.

2. There was a potential path traversal vulnerability in
   `lib/WebworkWebservice/RenderProblem.pm` that made it possible to
   read a file outside of the course templates directory. A user with
   permission to use the `render_rpc` end point could potentially read
   the `/etc/passwd` by using a `sourceFilePath` such as
   `../../../../../etc/passwd`. Although, even if that is done I could
   not find a way to actually see the file's contents (other than a
   couple of characters when rendering of the file as a PG problem
   fails). By using a path such as `../../otherCourse/templates/otherCourseProblem.pg`
   an instructor could render a problem from another course, but I don't
   consider that a very high security vulnerability.

3. An instructor could engineer a request (by modifying parameters in
   the browser using the developer tools or using a script) to the PG
   problem editor to add a problem using the `add_problem` action and
   pass a `sourceFilePath` such `../../../../../etc/passwd` and a new
   problem would be added to the set with that file path. If the
   instructor then opens the problem in the set, the entire contents of
   the file will be revealed in the errors when the file fails to
   render as a PG problem.  This could also be used to render a problem
   from another course's templates directory, and it would even work in
   the assignment.  The possibiity of viewing a file such as
   `/etc/passwd` file is a high security vulnerability, but rendering a
   problem file from another course, probably not so much.

4. Restrict the `instructor_rpc` endpoint to POST requests only. The
   exploit that Claude describes is that an attacker could emails an
   instructor a URL like such as `https://server.edu/webwork2/instructor_rpc?rpc_command=putUserProblem&courseID=Math101&user_id=someStudent&...&status=1`.
   Clicking it sends the session cookie, and the change (grade/status
   edit, user add/delete, etc.) executes. Disabling GET requests
   prevents that. Claude also recommends adding a CSRF token, but I
   don't see that as necessary. It is contrary to the API like design
   of the Webwork web service, and is not a usual protection that is put
   into place by other systems for this sort of thing either.
   Furthermore, this would not actually add security.  It would mean
   that in order for webwork2 itself to use the end point, it would have
   to make the CSRF token available in the DOM for the JavaScript to be
   able to attach to any request. That is the same basic security
   vulnerability that could be exploited in the same way that the
   session key could be exploited when using `$session_management_via = 'key'`.

5. Use `Math::Random::Secure::irand` to generate the session key instead
   of `srand` and `rand` so the the session key is cryptpographically
   secure. The protects against the possibility of an attacker
   triggering many logins (including an unauthenticated guest login,
   which also calls `create_session`) and harvesting a large sample of
   session keys generated by the same worker process, and attempting a
   Mersenne-Twister state-recovery techniques to predict keys issued to
   other users on that worker.  Although, this may only be a
   vulnerability if guest logins are enabled for a course. Note that
   Claude also said to fix the identical code in `WebworkSOAP.pm`, and I
   did, but I plan to delete `WebworkSOAP.pm` for the next release. I
   would be willing to do that for this release if no one is opposed! I
   have it all set up in a local branch.

6. There was a possibility of an LDAP filter injection when LDAP
   authentication is enabled.  A username could be altered with a
   username such as `admin)(|(uid=*` which would change the DN the
   filter would resolve to. This allows an attacker to redirect which
   account's DN gets targeted, allowing for an account-confusion attack.
   The fix is to use the `Net::LDAP::Util::escape_filter_value` method
   on the `$uid` before it is passed to the `$ldap->search` call. Note
   that the `Net::LDAP::Util` module is part of with the
   `Net::LDAP` package, and so this is not a new dependency.

7. Both LTI authentication modules save the first and last name of a
   user that authenticates via LTI to the database.  Those are things
   that in some cases a user can set on their own (depending on if the
   institution chooses to allow that), and a user could set one of those
   to include a `<script>` tag with malicious code. The first and last
   name are displayed in several templates unescaped, and so a user
   could potential exploit this to perform an XSS attack on their
   instructor. I tested this with my mock LMS using 1.3 authentication,
   and the script I set for the user's last name was executed on the
   user detail page (the sets assigned to user page that opens when you
   click on the "Assigned Sets" column in the "Accounts Manager").
   Testing with Moodle I observed that Moodle immediately strips off the
   `<script>` tag and any special characters, and so that never makes it to
   webwork. Testing this with Canvas I observed that Canvas does not,
   and the script executes when opening the user detail page for the
   user.

   In any case Claude's suggestion to escape these in the templates
   isn't going to work as that would break the things that are being done
   in those templates. So instead this just strips any ampersands, less
   thans, greater thans, or quotes from the first and last name when they
   are received during LTI authentication.  Single quotes are left since
   they will not be a problem and are common in names.

Note that I did not use Claude to fix them, although in some cases the
fixes are just what Claude recommended.

There were 2 other vulnerabilities that were listed that were not fixed.

Both relate to symbolic links in a course templates directory.
The first is basically allowing any symbolic link to be extracted in the
file manager when a tarball is extracted.  The second is that the
PGProblemEditor can read files linked to by a symbolic link in the
course templates directory.  The second is really only a vulnerability
in combination with the first, assuming that the only links in the
course templates directory are the ones that the webwork system
administrator wants to be there (such as Library, Contrib,
Student_Orientation, or other local libraries), and permissions are
correctly set for the files in the directories linked to. This is
because currently extracting a tarball (and maybe a zip archive as well
which Claude didn't notice) that contains a symbolic link to somewhere
else on the system is the only way that such a link can be created by an
instructor. So to fix both issues we will have to stop allowing symbolic
links in extracted archives.  This would be a change to what we have
allowed to this point, but is probably what we are going to need to do.
So the only allowed symbolic links in a course templates directory would
be links created by the webwork2 server administrator.

Author: drgrice1

lib/WeBWorK/Authen.pm

@@ -36,8 +36,9 @@ use strict;
 use warnings;
 
 use Date::Format;
-use Scalar::Util qw(weaken);
-use Mojo::Util   qw(b64_encode b64_decode);
+use Scalar::Util         qw(weaken);
+use Mojo::Util           qw(b64_encode b64_decode);
+use Math::Random::Secure qw(irand);
 
 use WeBWorK::Debug;
 use WeBWorK::Utils       qw(x runtime_use utf8Crypt);
@@ -705,8 +706,7 @@ sub create_session {
 
 	if (!$c->stash->{'webwork2.database_session'} || !$c->stash->{'webwork2.database_session'}{user_id}) {
 		my @chars = @{ $ce->{sessionKeyChars} };
-		srand;
-		$newKey = join('', @chars[ map rand(@chars), 1 .. $ce->{sessionKeyLength} ]);
+		$newKey = join('', @chars[ map irand(@chars), 1 .. $ce->{sessionKeyLength} ]);
 		$c->stash->{'webwork2.database_session'} =
 			{ user_id => $userID, key => $newKey, timestamp => time, session => {} };
 	} else {

lib/WeBWorK/Authen/LDAP.pm

@@ -4,8 +4,9 @@ use base qw/WeBWorK::Authen/;
 use strict;
 use warnings;
 
-use WeBWorK::Debug qw(debug);
-use Net::LDAP      qw(LDAP_INVALID_CREDENTIALS);
+use WeBWorK::Debug  qw(debug);
+use Net::LDAP       qw(LDAP_INVALID_CREDENTIALS);
+use Net::LDAP::Util qw(escape_filter_value);
 
 sub checkPassword {
 	my ($self, $userID, $possibleClearPassword) = @_;
@@ -69,7 +70,7 @@ sub ldap_authen_uid {
 	}
 
 	# look up user's DN
-	$msg = $ldap->search(base => $base, filter => "$rdn=$uid");
+	$msg = $ldap->search(base => $base, filter => "$rdn=" . escape_filter_value($uid));
 	if ($msg->is_error) {
 		warn "AUTH LDAP: search error ", $msg->code, ": ", $msg->error_text, ".\n", $searchdn, "\n", $base, "\n", $uid,
 			"\n";

lib/WeBWorK/Authen/LTIAdvanced.pm

@@ -203,6 +203,13 @@ sub get_credentials {
 			[ 'recitation',         'custom_recitation' ],
 		);
 
+		# The last_name and first_name are things that the user might be able to configure in the LMS, and the user
+		# could set one of them to something that includes malicious JavaScript code. So any ampersands, less thans,
+		# greater thans, and quotes are stripped from these values in case something like that has been done. Note that
+		# a single quote is left because that is not uncommon in names and will not cause a problem in any case.
+		# Usually names will not actually be modified by this.
+		$self->{$_} =~ s/([&<>"])//e for 'last_name', 'first_name';
+
 		if (defined($ce->{LTI}{v1p1}{preferred_source_of_student_id})
 			&& defined($c->param($ce->{preferred_source_of_student_id})))
 		{

lib/WeBWorK/Authen/LTIAdvantage.pm

@@ -184,6 +184,13 @@ sub get_credentials ($self) {
 				[ recitation => 'https://purl.imsglobal.org/spec/lti/claim/custom#recitation' ],
 			);
 
+		# The last_name and first_name are things that the user might be able to configure in the LMS, and the user
+		# could set one of them to something that includes malicious JavaScript code. So any ampersands, less thans,
+		# greater thans, and quotes are stripped from these values in case something like that has been done. Note that
+		# a single quote is left because that is not uncommon in names and will not cause a problem in any case.
+		# Usually names will not actually be modified by this.
+		$self->{$_} =~ s/([&<>"])//e for 'last_name', 'first_name';
+
 		$self->{student_id} =
 			$ce->{LTI}{v1p3}{preferred_source_of_student_id}
 			? ($extract_claim->($ce->{LTI}{v1p3}{preferred_source_of_student_id}) // '')

lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor.pm

@@ -850,6 +850,14 @@ sub add_problem_handler ($c) {
 	my $templatesPath  = $c->ce->{courseDirs}{templates};
 	my $sourceFilePath = $c->{editFilePath} =~ s|^$templatesPath/||r;
 
+	unless (path_is_subdir($sourceFilePath, $c->ce->{courseDirs}{templates}, 1)) {
+		$c->addbadmessage($c->maketext(
+			'The source file path [_1] is not contained in the course templates directory and cannot be added.',
+			$c->{editFilePath}
+		));
+		return;
+	}
+
 	my $targetSetName  = $c->param('action.add_problem.target_set');
 	my $targetFileType = $c->param('action.add_problem.file_type');
 

lib/WeBWorK/Utils/ListingDB.pm

@@ -115,10 +115,11 @@ sub getDBextras ($c, $path) {
 	$path =~ s|^Library/||;
 	my $filename = basename $path;
 	$path = dirname $path;
-	my $query =
+	my @res = $dbh->selectrow_array(
 		"SELECT pgfile.MO, pgfile.static FROM `$OPLtables{pgfile}` pgfile, `$OPLtables{path}` p "
-		. "WHERE p.path=\"$path\" AND pgfile.path_id=p.path_id AND pgfile.filename=\"$filename\"";
-	my @res = $dbh->selectrow_array($query);
+			. 'WHERE p.path = ? AND pgfile.path_id = p.path_id AND pgfile.filename = ?',
+		{}, $path, $filename
+	);
 	if (@res) {
 		$mo     = $res[0];
 		$static = $res[1];

lib/WeBWorK/Utils/Routes.pm

@@ -172,9 +172,10 @@ my %routeParameters = (
 		path   => '/render_rpc'
 	},
 	instructor_rpc => {
-		title  => 'instructor_rpc',
-		module => 'InstructorRPCHandler',
-		path   => '/instructor_rpc'
+		title   => 'instructor_rpc',
+		module  => 'InstructorRPCHandler',
+		path    => '/instructor_rpc',
+		methods => ['POST']
 	},
 
 	ltiadvanced_content_selection => {

lib/WebworkSOAP.pm

@@ -2,6 +2,8 @@ package WebworkSOAP;
 
 use strict;
 
+use Math::Random::Secure qw(irand);
+
 use WeBWorK::Utils::CourseManagement qw(listCourses);
 use WeBWorK::DB;
 use WeBWorK::DB::Utils qw(initializeUserProblem);
@@ -129,8 +131,7 @@ sub login_user {
 	my $timestamp = time;
 	my @chars     = @{ $soapEnv->{ce}->{sessionKeyChars} };
 	my $length    = $soapEnv->{ce}->{sessionKeyLength};
-	srand;
-	$newKey = join("", @chars[ map rand(@chars), 1 .. $length ]);
+	$newKey = join("", @chars[ map irand(@chars), 1 .. $length ]);
 	my $Key = $soapEnv->{db}->newKey(user_id => $userID, key => $newKey, timestamp => $timestamp);
 	eval { $soapEnv->{db}->deleteKey($userID) };
 	eval { $soapEnv->{db}->addKey($Key) };

lib/WebworkWebservice/RenderProblem.pm

@@ -12,7 +12,7 @@ use WeBWorK::CourseEnvironment;
 use WeBWorK::DB;
 use WeBWorK::DB::Utils        qw(global2user fake_set fake_problem);
 use WeBWorK::Utils            qw(decode_utf8_base64);
-use WeBWorK::Utils::Files     qw(readFile);
+use WeBWorK::Utils::Files     qw(readFile path_is_subdir);
 use WeBWorK::Utils::Rendering qw(renderPG);
 
 our $UNIT_TESTS_ON = 0;
@@ -26,19 +26,30 @@ async sub renderProblem {
 	# is enabled.  That is an expensive method to always call here.
 	debug(pretty_print_rh($rh)) if $WeBWorK::Debug::Enabled;
 
-	# If the problem source is provided, check user is allow to render problem source.
-	if (!$ws->authz->hasPermissions($rh->{user}, 'webservice_render_source')
-		&& ($rh->{problemSource} || $rh->{rawProblemSource} || $rh->{uriEncodedProblemSource}))
-	{
-		$ws->error_string(__PACKAGE__ . ": User $rh->{user} does not have permission to render problem source.");
-		return {};
+	my $ce = $ws->ce;
+
+	if ($rh->{problemSource} || $rh->{rawProblemSource} || $rh->{uriEncodedProblemSource}) {
+		# If the problem source is provided, check user is allow to render problem source.
+		unless ($ws->authz->hasPermissions($rh->{user}, 'webservice_render_source')) {
+			$ws->error_string(__PACKAGE__ . ": User $rh->{user} does not have permission to render problem source.");
+			return {};
+		}
+	} elsif (defined $rh->{sourceFilePath} && $rh->{sourceFilePath} =~ /\S/) {
+		# If the source file path is provided, ensure it is contained in the course's templates directory.
+		unless (path_is_subdir(
+			$ce->{courseDirs}{templates} . '/' . $rh->{sourceFilePath},
+			$ce->{courseDirs}{templates}
+		))
+		{
+			$ws->error_string(__PACKAGE__ . ": Source file path is unsafe.");
+			return {};
+		}
 	}
 
 	my $problemSeed = $rh->{problemSeed} // '1234';
 
 	my $beginTime = Benchmark->new;
 
-	my $ce = $ws->ce;
 	my $db = $ws->db;
 
 	# Determine an effective user for this interaction or create one if it is not given.