Security vulnerabilities

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.