From e84a90943ab1e4a254b2d33c7cabef18b718b456 Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Wed, 20 Mar 2019 17:52:31 +0100 Subject: Session fix when form + HTTP auth are used (#2286) https://github.com/Alkarex/FreshRSS/commit/bf51c82d55f6bf1af2a6464ca4f148d6c613d28f https://github.com/FreshRSS/FreshRSS/issues/2125#issuecomment-473873922 --- app/Models/Auth.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/Models/Auth.php') diff --git a/app/Models/Auth.php b/app/Models/Auth.php index 513a9cb2f..16a506f00 100644 --- a/app/Models/Auth.php +++ b/app/Models/Auth.php @@ -13,7 +13,7 @@ class FreshRSS_Auth { * This method initializes authentication system. */ public static function init() { - if (Minz_Session::param('REMOTE_USER', '') !== httpAuthUser()) { + if (isset($_SESSION['REMOTE_USER']) && $_SESSION['REMOTE_USER'] !== httpAuthUser()) { //HTTP REMOTE_USER has changed self::removeAccess(); } -- cgit v1.2.3 From ebd8c31c0272f135b1b55f0480d1c8c3875935fe Mon Sep 17 00:00:00 2001 From: Alexandre Alapetite Date: Fri, 22 Mar 2019 19:05:38 +0100 Subject: 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 --- app/Controllers/authController.php | 6 +++++- app/Controllers/userController.php | 1 + app/FreshRSS.php | 32 ++++++++++++++++++++------------ app/Models/Auth.php | 8 ++++---- p/scripts/main.js | 28 ++++++++++++++++++++++++++-- 5 files changed, 56 insertions(+), 19 deletions(-) (limited to 'app/Models/Auth.php') 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; } } diff --git a/p/scripts/main.js b/p/scripts/main.js index 521adc839..212bf804b 100644 --- a/p/scripts/main.js +++ b/p/scripts/main.js @@ -44,6 +44,12 @@ var context; }()); // +function badAjax() { + openNotification(context.i18n.notif_request_failed, 'bad'); + location.reload(); + return true; +} + function needsScroll(elem) { const winBottom = document.documentElement.scrollTop + document.documentElement.clientHeight, elemTop = elem.offsetParent.offsetTop + elem.offsetTop, @@ -165,6 +171,9 @@ function send_mark_read_queue(queue, asRead) { for (let i = queue.length - 1; i >= 0; i--) { delete pending_entries['flux_' + queue[i]]; } + if (this.status == 403) { + badAjax(); + } }; req.onload = function (e) { if (this.status != 200) { @@ -269,6 +278,9 @@ function mark_favorite(div) { req.onerror = function (e) { openNotification(context.i18n.notif_request_failed, 'bad'); delete pending_entries[div.id]; + if (this.status == 403) { + badAjax(); + } }; req.onload = function (e) { if (this.status != 200) { @@ -918,6 +930,9 @@ function init_stream(stream) { req.responseType = 'json'; req.onerror = function (e) { checkboxTag.checked = !isChecked; + if (this.status == 403) { + badAjax(); + } }; req.onload = function (e) { if (this.status != 200) { @@ -1008,6 +1023,9 @@ function updateFeed(feeds, feeds_count) { const req = new XMLHttpRequest(); req.open('POST', feed.url, true); req.onloadend = function (e) { + if (this.status != 200) { + return badAjax(); + } feed_processed++; const div = document.getElementById('actualizeProgress'); div.querySelector('.progress').innerHTML = feed_processed + ' / ' + feeds_count; @@ -1045,9 +1063,12 @@ function init_actualize() { context.ajax_loading = true; const req = new XMLHttpRequest(); - req.open('GET', './?c=javascript&a=actualize', true); + req.open('POST', './?c=javascript&a=actualize', true); req.responseType = 'json'; req.onload = function (e) { + if (this.status != 200) { + return badAjax(); + } const json = xmlHttpRequestJson(this); if (auto && json.feeds.length < 1) { auto = false; @@ -1078,7 +1099,10 @@ function init_actualize() { updateFeed(json.feeds, feeds_count); } }; - req.send(); + req.setRequestHeader('Content-Type', 'application/json'); + req.send(JSON.stringify({ + _csrf: context.csrf, + })); return false; }; -- cgit v1.2.3