Rework CSRF interaction with sessions (#2290)

* Rework CSRF interaction with sessions

Fix https://github.com/FreshRSS/FreshRSS/issues/2288
Improve security in some edge cases
Maybe relevant for
https://github.com/FreshRSS/FreshRSS/issues/2125#issuecomment-474992671

* Forgotten mime type

4 files changed, 30 insertions, 17 deletions

diff --git a/app/Controllers/authController.php b/app/Controllers/authController.php
index 75d4acae0..ca44b1a96 100644
--- a/app/Controllers/authController.php
+++ b/app/Controllers/authController.php
@@ -69,7 +69,7 @@ class FreshRSS_auth_Controller extends Minz_ActionController {
 	 * the user is already connected.
 	 */
 	public function loginAction() {
-		if (FreshRSS_Auth::hasAccess()) {
+		if (FreshRSS_Auth::hasAccess() && Minz_Request::param('u', '') == '') {
 			Minz_Request::forward(array('c' => 'index', 'a' => 'index'), true);
 		}
 
@@ -133,6 +133,7 @@ class FreshRSS_auth_Controller extends Minz_ActionController {
 				// Set session parameter to give access to the user.
 				Minz_Session::_param('currentUser', $username);
 				Minz_Session::_param('passwordHash', $conf->passwordHash);
+				Minz_Session::_param('csrf');
 				FreshRSS_Auth::giveAccess();
 
 				// Set cookie parameter if nedded.
@@ -161,6 +162,8 @@ class FreshRSS_auth_Controller extends Minz_ActionController {
 				return;
 			}
 
+			FreshRSS_FormAuth::deleteCookie();
+
 			$conf = get_user_configuration($username);
 			if ($conf == null) {
 				return;
@@ -176,6 +179,7 @@ class FreshRSS_auth_Controller extends Minz_ActionController {
 			if ($ok) {
 				Minz_Session::_param('currentUser', $username);
 				Minz_Session::_param('passwordHash', $s);
+				Minz_Session::_param('csrf');
 				FreshRSS_Auth::giveAccess();
 
 				Minz_Request::good(_t('feedback.auth.login.success'),
diff --git a/app/Controllers/userController.php b/app/Controllers/userController.php
index 71172b9ef..be3787561 100644
--- a/app/Controllers/userController.php
+++ b/app/Controllers/userController.php
@@ -247,6 +247,7 @@ class FreshRSS_user_Controller extends Minz_ActionController {
 				$user_conf = get_user_configuration($new_user_name);
 				Minz_Session::_param('currentUser', $new_user_name);
 				Minz_Session::_param('passwordHash', $user_conf->passwordHash);
+				Minz_Session::_param('csrf');
 				FreshRSS_Auth::giveAccess();
 			}
 
diff --git a/app/FreshRSS.php b/app/FreshRSS.php
index 1dc80718e..ecf13e4cf 100644
--- a/app/FreshRSS.php
+++ b/app/FreshRSS.php
@@ -57,18 +57,26 @@ class FreshRSS extends Minz_FrontController {
 
 	private static function initAuth() {
 		FreshRSS_Auth::init();
-		if (Minz_Request::isPost() && !(is_referer_from_same_domain() && FreshRSS_Auth::isCsrfOk())) {
-			// Basic protection against XSRF attacks
-			FreshRSS_Auth::removeAccess();
-			$http_referer = empty($_SERVER['HTTP_REFERER']) ? '' : $_SERVER['HTTP_REFERER'];
-			Minz_Translate::init('en');	//TODO: Better choice of fallback language
-			Minz_Error::error(
-				403,
-				array('error' => array(
-					_t('feedback.access.denied'),
-					' [HTTP_REFERER=' . htmlspecialchars($http_referer, ENT_NOQUOTES, 'UTF-8') . ']'
-				))
-			);
+		if (Minz_Request::isPost()) {
+			if (!is_referer_from_same_domain()) {
+				// Basic protection against XSRF attacks
+				FreshRSS_Auth::removeAccess();
+				$http_referer = empty($_SERVER['HTTP_REFERER']) ? '' : $_SERVER['HTTP_REFERER'];
+				Minz_Translate::init('en');	//TODO: Better choice of fallback language
+				Minz_Error::error(403, array('error' => array(
+						_t('feedback.access.denied'),
+						' [HTTP_REFERER=' . htmlspecialchars($http_referer, ENT_NOQUOTES, 'UTF-8') . ']'
+					)));
+			}
+			if ((!FreshRSS_Auth::isCsrfOk()) &&
+				(Minz_Request::controllerName() !== 'auth' || Minz_Request::actionName() !== 'login')) {
+				// Token-based protection against XSRF attacks, except for the login form itself
+				Minz_Translate::init('en');	//TODO: Better choice of fallback language
+				Minz_Error::error(403, array('error' => array(
+						_t('feedback.access.denied'),
+						' [CSRF]'
+					)));
+			}
 		}
 	}
 
diff --git a/app/Models/Auth.php b/app/Models/Auth.php
index 16a506f00..6d079a01f 100644
--- a/app/Models/Auth.php
+++ b/app/Models/Auth.php
@@ -24,6 +24,7 @@ class FreshRSS_Auth {
 			$conf = Minz_Configuration::get('system');
 			$current_user = $conf->default_user;
 			Minz_Session::_param('currentUser', $current_user);
+			Minz_Session::_param('csrf');
 		}
 
 		if (self::$login_ok) {
@@ -56,6 +57,7 @@ class FreshRSS_Auth {
 				$current_user = trim($credentials[0]);
 				Minz_Session::_param('currentUser', $current_user);
 				Minz_Session::_param('passwordHash', trim($credentials[1]));
+				Minz_Session::_param('csrf');
 			}
 			return $current_user != '';
 		case 'http_auth':
@@ -63,6 +65,7 @@ class FreshRSS_Auth {
 			$login_ok = $current_user != '' && FreshRSS_UserDAO::exists($current_user);
 			if ($login_ok) {
 				Minz_Session::_param('currentUser', $current_user);
+				Minz_Session::_param('csrf');
 			}
 			return $login_ok;
 		case 'none':
@@ -196,13 +199,10 @@ class FreshRSS_Auth {
 	}
 	public static function isCsrfOk($token = null) {
 		$csrf = Minz_Session::param('csrf');
-		if ($csrf == '') {
-			return true;	//Not logged in yet
-		}
 		if ($token === null) {
 			$token = Minz_Request::fetchPOST('_csrf');
 		}
-		return $token === $csrf;
+		return $token != '' && $token === $csrf;
 	}
 }