Merge pull request #1269 from taniwallach/cookie-refactor-same-site-v2

Cookie refactor same site v2 - replacing PR #1149
openwebwork · Mar 22, 2021 · adde29d · adde29d
2 parents 23d9b8d + 786f87c
commit adde29d
Show file tree

Hide file tree

Showing 7 changed files with 146 additions and 128 deletions.
diff --git a/Dockerfile b/Dockerfile
@@ -82,6 +82,8 @@ RUN git clone --single-branch --branch master --depth 1 https://github.com/mathj
 # we need to change FROM before setting the ENV variables
 
 FROM ubuntu:18.04
+# Once ubuntu 20.04 is used, CGI.pm and CGI::Cookie will be new enough to
+# drop the cpanm install of CGI::Cookie which is needed to upgrade it.
 
 ENV WEBWORK_URL=/webwork2 \
     WEBWORK_ROOT_URL=http://localhost \
@@ -276,7 +278,9 @@ RUN echo "PATH=$PATH:$APP_ROOT/webwork2/bin" >> /root/.bashrc \
 
 # Phase 6 - install additional Perl modules from CPAN (not packaged for Ubuntu or outdated in Ubuntu)
 
-RUN cpanm install Statistics::R::IO \
+# Ubuntu 18.04 has CGI.pm 4.38-1 which is too old to support the cookie samesite attribute added in CGI.pm 4.45 - so install CGI::Cookie here to get an upgraded version.
+
+RUN cpanm install Statistics::R::IO CGI::Cookie \
     && rm -fr ./cpanm /root/.cpanm /tmp/*
 
 # Now installed from Ubuntu packages:

diff --git a/bin/check_modules.pl b/bin/check_modules.pl
@@ -32,7 +32,6 @@
 
 my @apache2ModulesList = qw(
 	Apache2::Request
-	Apache2::Cookie
 	Apache2::ServerRec
 	Apache2::ServerUtil
 );
@@ -45,6 +44,7 @@
 	Benchmark
 	Carp
 	CGI
+	CGI::Cookie
 	Class::Accessor
 	Data::Dump
 	Data::Dumper

diff --git a/conf/defaults.config b/conf/defaults.config
@@ -928,6 +928,32 @@ $session_management_via = "session_cookie";
 # options when enabling Caliper.
 $caliper{enabled} = 0;
 
+################################################################################
+# Cookie control settings
+################################################################################
+
+# The following variables can be set to control cookie behavior.
+
+# Set the value of the samesite attribute of the WeBWorK cookie:
+#    See: https://blog.chromium.org/2019/10/developers-get-ready-for-new.html
+#         https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite
+#         https://tools.ietf.org/html/draft-west-cookie-incrementalism-00
+
+$CookieSameSite = "Strict";
+
+# Set the value of the secure cookie attribute:
+$CookieSecure = 0; # Default is 0 here, as 1 will not work without https
+
+# At present the CookieLifeTime setting only effect how long the
+# browser is to supposed to retain the cookie.
+#     https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
+$CookieLifeTime = "+7d";
+
+# NOTE: In general the cookie lifespan settings use the CGI.pm relative time settings.
+# Search for "30 seconds from now" at https://metacpan.org/pod/CGI to see the various options.
+# A WW option is to set it to "session", in which case the Cookie will expire when the
+# browser session ends (a "session cookie").
+
 ################################################################################
 # PG subsystem options
 ################################################################################

diff --git a/conf/localOverrides.conf.dist b/conf/localOverrides.conf.dist
@@ -498,6 +498,35 @@ $mail{feedbackRecipients}    = [
 
 #$session_management_via = "key";
 
+################################################################################
+# Cookie control settings
+################################################################################
+
+# The following variables can be set to control cookie behavior.
+
+# Set the value of the samesite attribute of the WeBWorK cookie:
+#    See: https://blog.chromium.org/2019/10/developers-get-ready-for-new.html
+#         https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite
+#         https://tools.ietf.org/html/draft-west-cookie-incrementalism-00
+
+#$CookieSameSite = "None";
+#$CookieSameSite = "Strict";
+#$CookieSameSite = "Lax";
+
+# Set the value of the secure cookie attribute:
+#$CookieSecure = 1;
+
+# At present the CookieLifeTime setting only effect how long the
+# browser is to supposed to retain the cookie.
+#     https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie
+#$CookieLifeTime = "+7d";
+#$CookieLifeTime = "session";
+
+# NOTE: In general the cookie lifespan settings use the CGI.pm relative time settings.
+# Search for "30 seconds from now" at https://metacpan.org/pod/CGI to see the various options.
+# A WW option is to set it to "session", in which case the Cookie will expire when the
+# browser session ends (a "session cookie").
+
 ################################################################################
 # Searching for set.def files to import
 ################################################################################

diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm
@@ -1,13 +1,12 @@
 ################################################################################
 # WeBWorK Online Homework Delivery System
-# Copyright &copy; 2000-2018 The WeBWorK Project, http://openwebwork.sf.net/
-# $CVSHeader: webwork2/lib/WeBWorK/Authen.pm,v 1.63 2012/06/06 22:03:15 wheeler Exp $
-# 
+# Copyright &copy; 2000-2020 The WeBWorK Project, https://openwebworkorg.wordpress.com/
+#
 # This program is free software; you can redistribute it and/or modify it under
 # the terms of either: (a) the GNU General Public License as published by the
 # Free Software Foundation; either version 2, or (at your option) any later
 # version, or (b) the "Artistic License" which comes with this package.
-# 
+#
 # This program is distributed in the hope that it will be useful, but WITHOUT
 # ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
 # FOR A PARTICULAR PURPOSE.  See either the GNU General Public License or the
@@ -77,7 +76,6 @@ our $GENERIC_ERROR_MESSAGE = "";  # define in new
 ## WeBWorK-tr end modification 
 #####################
 
-use constant COOKIE_LIFESPAN => 60*60*24*30; # 30 days
 #use constant GENERIC_ERROR_MESSAGE => "Invalid user ID or password.";
 
 
@@ -378,32 +376,11 @@ sub get_credentials {
 			#croak ("cookieUser = $cookieUser and paramUser = ". $r->param("user") . " are different.");
 			$self->maybe_kill_cookie; # use parameter "user" rather than cookie "user";
 		}
-# I don't understand this next segment.
-# If both key and $cookieKey exist then why not just ignore the cookieKey?
-
-# 		if (defined $cookieKey and defined $r->param("key")) {
-# 			$self -> {user_id} = $cookieUser;
-# 			$self -> {password} = $r->param("passwd");
-# 			$self -> {login_type} = "normal";
-# 			$self -> {credential_source} = "params_and_cookie";
-# 			$self -> {session_key} = $cookieKey;
-# 			$self -> {cookie_timestamp} = $cookieTimeStamp;
-# 			if ($cookieKey ne $r->param("key")) {
-# 				warn ("cookieKey = $cookieKey and param key = " . $r -> param("key") . " are different, perhaps"
-# 					 ." because you opened several windows for the same site and then backed up from a newer one to an older one."
-# 					 ."  Avoid doing so.");
-# 			$self -> {credential_source} = "conflicting_params_and_cookie";
-# 			}
-# 			debug("params and cookie user '", $self->{user_id}, "' credential_source = '", $self->{credential_source},
-# 				"' params and cookie session key = '", $self->{session_key}, "' cookie_timestamp '", $self->{cookieTimeStamp}, "'");
-# 			return 1;
-# 		} els
-
 # Use session key for verification
 # else   use cookieKey for verification
 # else    use cookie user name but use password provided by request.
 
-		if (defined $r -> param("key")) {
+		if (defined $r->param("key")) {
 			$self->{user_id} = $r->param("user");
 			$self->{session_key} = $r->param("key");
 			$self->{password} = $r->param("passwd");
@@ -420,19 +397,24 @@ sub get_credentials {
 			$self->{login_type} = "normal";
 			$self->{credential_source} = "cookie";
 			$self->{user_id}     = trim($self->{user_id});
-			debug("cookie user '", $self->{user_id}, "' key '", $self->{session_key}, "' cookie_timestamp '", $self->{cookieTimeStamp}, "'");
+			debug(  "cookie user '", $self->{user_id},
+				"' key '", $self->{session_key},
+				"' cookie_timestamp '", $self->{cookieTimeStamp}, "' "
+			);
 			return 1;
 		} else {
-			$self -> {user_id} = $cookieUser;
-			$self -> {session_key} = $cookieKey; # will be undefined
-			$self -> {password} = $r->param("passwd");
-			$self -> {cookie_timestamp} = $cookieTimeStamp;
-			$self -> {login_type} = "normal";
-			$self -> {credential_source} = "params_and_cookie";
+			$self->{user_id} = $cookieUser;
+			$self->{session_key} = $cookieKey; # will be undefined
+			$self->{password} = $r->param("passwd");
+			$self->{cookie_timestamp} = $cookieTimeStamp;
+			$self->{login_type} = "normal";
+			$self->{credential_source} = "params_and_cookie";
 			$self->{user_id}     = trim($self->{user_id});
 			$self->{password}    = trim($self->{password});
-			debug("params and cookie user '", $self->{user_id}, "' params and cookie session key = '",
-				 $self->{session_key}, "' cookie_timestamp '", $self->{cookieTimeStamp}, "'");
+			debug(  "params and cookie user '", $self->{user_id},
+				"' params and cookie session key = '", $self->{session_key},
+				"' cookie_timestamp '", $self->{cookieTimeStamp}, "' "
+			);
 			return 1;
 		}	
 	}
@@ -457,7 +439,10 @@ sub get_credentials {
 		$self->{login_type} = "normal";
 		$self->{credential_source} = "cookie";
 		$self->{user_id}     = trim($self->{user_id});
-		debug("cookie user '", $self->{user_id}, "' key '", $self->{session_key}, "' cookie_timestamp '", $self->{cookieTimeStamp}, "'");
+		debug(  "cookie user '", $self->{user_id},
+			"' key '", $self->{session_key},
+			"' cookie_timestamp '", $self->{cookieTimeStamp}, "' "
+		);
 		return 1;
 	}
 }
@@ -806,16 +791,22 @@ sub check_session {
 
 	my $Key = $db->getKey($userID); # checked
 	return 0 unless defined $Key;
+
 	my $keyMatches = (defined $possibleKey and $possibleKey eq $Key->key);
-
-	my $timestampValid=0;
+
+	my $time_now = time();
+
+	# Want key not be too old. Use timestamp from DB and
+	# sessionKeyTimeout to determine this even when using cookies
+	# as we do not trust the timestamp provided by a user's browser.
+	my $timestampValid  = ( $time_now <= $Key->timestamp() + $ce->{sessionKeyTimeout} );
+
 # first part of if clause is disabled for now until we figure out long term fix for using cookies
 # safely (see pull request #576)   This means that the database key time is always being used
 # even when in "session_cookie" mode
 #	if ($ce -> {session_management_via} eq "session_cookie" and defined($self->{cookie_timestamp})) {
 #		$timestampValid = (time <= $self -> {cookie_timestamp} + $ce->{sessionKeyTimeout});
 #	} else {
-		$timestampValid = (time <= $Key->timestamp()+$ce->{sessionKeyTimeout});
 		if ($keyMatches and $timestampValid and $updateTimestamp) {
 			$Key->timestamp(time);
 			$db->putKey($Key);
@@ -842,14 +833,14 @@ sub killSession {
 		$caliper_sensor->sendEvents($self->{r}, [$login_event]);
 	}
 
-	$self -> forget_verification;
-	if ($ce -> {session_management_via} eq "session_cookie")  {
-		$self -> killCookie();
+	$self->forget_verification;
+	if ($ce->{session_management_via} eq "session_cookie")  {
+		$self->killCookie();
 	}
 
 	my $userID = $r -> param("user");
 	if (defined($userID)) {
-		 $db -> deleteKey($userID);
+		 $db->deleteKey($userID);
 	}
 }
 
@@ -866,42 +857,17 @@ sub fetchCookie {
 
 	my $courseID = $urlpath->arg("courseID");
 
-	# AP2 - Apache2::Cookie needs $r, Apache::Cookie doesn't
-    #my %cookies = WeBWorK::Cookie->fetch( MP2 ? $r : () );
-    #my $cookie = $cookies{"WeBWorKCourseAuthen.$courseID"};
-
 	my $cookie = undef;
-	if (MP2) {
-
-		my $jar = undef;
- 		eval {
-       			$jar = $r->jar; #table of cookies
-  		};
-  		if (ref $@ and $@->isa("APR::Request::Error") ) {
-			debug("Error parsing cookies, will use a partial result");
-     			$jar = $@->jar; # table of successfully parsed cookies
-  		};
-		if ($jar) {
-			$cookie = uri_unescape($jar->get("WeBWorKCourseAuthen.$courseID"));
-		};
-	} else {
-		my %cookies = WeBWorK::Cookie->fetch();
-		$cookie = $cookies{"WeBWorKCourseAuthen.$courseID"};
-		if ($cookie) {
-			debug("found a cookie for this course: '", $cookie->as_string, "'");
-			$cookie = $cookie->value;
-		}
-	}
+	my %cookies = WeBWorK::Cookie->fetch();
+	$cookie = $cookies{"WeBWorKCourseAuthen.$courseID"};
 
 	if ($cookie) {
-        #debug("found a cookie for this course: '", $cookie->as_string, "'");
-        #debug("cookie has this value: '", $cookie->value, "'");
-        #my ($userID, $key) = split "\t", $cookie->value;
-        debug("cookie has this value: '", $cookie, "'");
-        my ($userID, $key, $timestamp) = split "\t", $cookie;
+		#debug("found a cookie for this course: '", $cookie->as_string, "'");
+	        debug("cookie has this value: '", $cookie->value, "'");
+		my ($userID, $key, $timestamp) = split "\t", $cookie->value;
 		if (defined $userID and defined $key and $userID ne "" and $key ne "") {
 			debug("looks good, returning userID='$userID' key='$key'");
- 			return $userID, $key, $timestamp;
+			return( $userID, $key, $timestamp );
 		} else {
 			debug("malformed cookie. returning nothing.");
 			return;
@@ -916,27 +882,37 @@ sub sendCookie {
 	my ($self, $userID, $key) = @_;
 	my $r = $self->{r};
 	my $ce = $r->ce;
-	
+
 	my $courseID = $r->urlpath->arg("courseID");
-	
+
  	my $timestamp = time();
-
-	my $cookie = WeBWorK::Cookie->new($r,
-		-name    => "WeBWorKCourseAuthen.$courseID",
- 		-value   => "$userID\t$key\t$timestamp",
-		-path    => $ce->{webworkURLRoot},
-		-secure  => 0,
+
+	my $cookie = WeBWorK::Cookie->new(
+		-name     => "WeBWorKCourseAuthen.$courseID",
+		-value    => "$userID\t$key\t$timestamp",
+		-path     => $ce->{webworkURLRoot},
+		-samesite => $ce->{CookieSameSite};
+		-secure   => $ce->{CookieSecure};    # Warning: use 1 only if using https
 	);
 
-	if ($ce->{session_management_via} ne "session_cookie") {
-		my $expires = time2str("%a, %d-%h-%Y %H:%M:%S %Z", time+COOKIE_LIFESPAN, "GMT");
-		$cookie -> expires($expires);
-	}
+	# Set how long the browser should retain the cookie. Using max_age is now recommended,
+	# and overrides expires, but some very old browser only support expires.
+	my $lifetime  = $ce->{CookieLifeTime};
+	if ( $lifetime ne 'session' ) {
+		$cookie->expires( $lifetime );
+		$cookie->max_age( $lifetime );
+	} # as when $lifetime eq 'session' the cookie should be a "session cookie"
+	  # and expire when the browser session is closed.
+	# At present the CookieLifeTime setting only effects how long the browser is to told to retain the cookie.
+	# Ideally, when $ce->{session_management_via} eq "session_cookie", and if the timestamp in the cookie was
+	# secured again client-side tampering, the timestamp and lifetime could be used to provide ongoing session
+	# authentication.
+
  	if ($r->hostname ne "localhost" && $r->hostname ne "127.0.0.1") {
- 		$cookie -> domain($r->hostname);    # if $r->hostname = "localhost" or "127.0.0.1", then this must be omitted.
+		$cookie->domain($r->hostname);    # if $r->hostname = "localhost" or "127.0.0.1", then this must be omitted.
 	}
-	
-	#debug("about to add Set-Cookie header with this string: '", $cookie->as_string, "'");
+
+#	debug("about to add Set-Cookie header with this string: '", $cookie->as_string, "'");
  	eval {$r->headers_out->set("Set-Cookie" => $cookie->as_string);};
  	if ($@) {croak $@; }
 }
@@ -945,21 +921,22 @@ sub killCookie {
 	my ($self) = @_;
 	my $r = $self->{r};
 	my $ce = $r->ce;
-	
+
 	my $courseID = $r->urlpath->arg("courseID");
-
-	my $expires = time2str("%a, %d-%h-%Y %H:%M:%S %Z", time-60*60*24, "GMT");
-	my $cookie = WeBWorK::Cookie->new($r,
-		-name => "WeBWorKCourseAuthen.$courseID",
-		-value => "\t",
-		-expires => $expires,
-		-path => $ce->{webworkURLRoot},
-		-secure => 0,
+
+	my $cookie = WeBWorK::Cookie->new(
+		-name      => "WeBWorKCourseAuthen.$courseID",
+		-value     => "\t",
+		'-max-age' => "-1d", # 1 day ago
+		-expires   => "-1d", # 1 day ago
+		-path      => $ce->{webworkURLRoot},
+		-samesite  => $ce->{CookieSameSite};
+		-secure    => $ce->{CookieSecure};    # Warning: use 1 only if using https
 	);
  	if ($r->hostname ne "localhost" && $r->hostname ne "127.0.0.1") {
- 		$cookie -> domain($r->hostname);  # if $r->hostname = "localhost" or "127.0.0.1", then this must be omitted.
+		$cookie -> domain($r->hostname);  # if $r->hostname = "localhost" or "127.0.0.1", then this must be omitted.
 	}
-	
+
 	#debug( "killCookie is about to set an expired cookie");
 	#debug("about to add Set-Cookie header with this string: '", $cookie->as_string, "'");
  	eval {$r->headers_out->set("Set-Cookie" => $cookie->as_string);};