From b14ee605eb9a2b603c36ada8dbdff16431ef8451 Mon Sep 17 00:00:00 2001 From: Jared Hancock Date: Tue, 12 Sep 2017 15:55:50 -0500 Subject: [PATCH 1/2] login: Clear session data on expiration This fixes an issue where the current session data is not retrieved from the database, because it is expired. However, the session-id is not reset either. Therefore, the session data with the new CSRF token is not updated in the database and the user may have trouble logging in. This problem manifests itself as a session expires. If a user clicks somewhere in the software and their session is now expired, a redirect to the login page is triggered. However, the CSRF token sent in the login page is not saved in the database. So when the user logs in, they are greeted with the CSRF failure message. This issue is addressed by retrieving the session data from the database, but clearing the content. Therefore it appears to the software as invalid and is properly reset and saved to the database, thereby avoiding the errors. --- include/class.ostsession.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/include/class.ostsession.php b/include/class.ostsession.php index dbb5cf6312..5e9fba886b 100644 --- a/include/class.ostsession.php +++ b/include/class.ostsession.php @@ -178,10 +178,15 @@ class DbSessionBackend function read($id) { try { - $this->data = SessionData::objects()->filter([ - 'session_id' => $id, - 'session_expire__gt' => SqlFunction::NOW(), - ])->one(); + $this->data = SessionData::objects() + ->filter(['session_id' => $id]) + ->annotate(['age' => SqlFunction::NOW()->minus(new SqlField('session_expire'))]) + ->one(); + if ($this->data->age > 0) { + // session_expire is in the past. Pretend it is expired and + // reset the data. This will assist with CSRF issues + $this->data->session_data=''; + } $this->id = $id; } catch (DoesNotExist $e) { From 00f0f21ae45e4fb385cd75ffbb420db08a60f209 Mon Sep 17 00:00:00 2001 From: Jared Hancock Date: Tue, 12 Sep 2017 16:00:33 -0500 Subject: [PATCH 2/2] login: Use AJAX to authenticate, add a shake effect This changes the staff authentication process to use AJAX to authentication (with a failsafe of using traditional page requests). If the authentication fails on the server because of CSRF problems, the page is refreshed in the browser fetching a new CSRF token. If the authentication fails because of password issues, the login box shakes and the user can retry. As before, the `forgot password` link shows up after the first failure. Upon success, a redirect is issued into the staff control panel. --- include/staff/login.tpl.php | 87 ++++++++++++++++++++++++++++++++++--- scp/css/login.css | 3 ++ scp/login.php | 50 ++++++++++++++++----- 3 files changed, 123 insertions(+), 17 deletions(-) diff --git a/include/staff/login.tpl.php b/include/staff/login.tpl.php index 228ec612b0..ca9e751af2 100644 --- a/include/staff/login.tpl.php +++ b/include/staff/login.tpl.php @@ -11,9 +11,9 @@ osTicket :: <?php echo __('Staff Control Panel');?> -

+

-
+
@@ -21,10 +21,11 @@ echo $info['userid']; ?>" placeholder="" autofocus autocorrect="off" autocapitalize="off"> - allowPasswordReset()) { ?> -

- -
@@ -61,11 +62,85 @@ document.getElementById('loginBox').style.backgroundColor = 'white'; } }); + + function attemptLoginAjax(e) { + var objectifyForm = function(formArray) { //serialize data function + var returnArray = {}; + for (var i = 0; i < formArray.length; i++) { + returnArray[formArray[i]['name']] = formArray[i]['value']; + } + return returnArray; + }; + if ($.fn.effect) { + // For some reason, JQuery-UI shake does not considere an element's + // padding when shaking. Looks like it might be fixed in 1.12. + // Thanks, https://stackoverflow.com/a/22302374 + var oldEffect = $.fn.effect; + $.fn.effect = function (effectName) { + if (effectName === "shake") { + var old = $.effects.createWrapper; + $.effects.createWrapper = function (element) { + var result; + var oldCSS = $.fn.css; + + $.fn.css = function (size) { + var _element = this; + var hasOwn = Object.prototype.hasOwnProperty; + return _element === element && hasOwn.call(size, "width") && hasOwn.call(size, "height") && _element || oldCSS.apply(this, arguments); + }; + + result = old.apply(this, arguments); + + $.fn.css = oldCSS; + return result; + }; + } + return oldEffect.apply(this, arguments); + }; + } + var form = $(e.target), + data = objectifyForm(form.serializeArray()) + data.ajax = 1; + $.ajax({ + url: form.attr('action'), + method: 'POST', + data: data, + cache: false, + success: function(json) { + if (!typeof(json) === 'object' || !json.status) + return; + switch (json.status) { + case 401: + if (json && json.redirect) + document.location.href = json.redirect; + if (json && json.message) + $('#login-message').text(json.message) + if (json && json.show_reset) + $('#reset-link').show() + if ($.fn.effect) { + $('#loginBox').effect('shake') + } + // Clear the password field + $('#pass').val('').focus(); + break + case 302: + if (json && json.redirect) + document.location.href = json.redirect; + break + } + }, + }); + e.preventDefault(); + e.stopPropagation(); + e.stopImmediatePropagation(); + return false; + } + diff --git a/scp/css/login.css b/scp/css/login.css index 71d6cb7fce..3bf8e12c55 100644 --- a/scp/css/login.css +++ b/scp/css/login.css @@ -353,3 +353,6 @@ input[type=password] { padding: 5px; font-size: 0.75em; } +.hidden { + display: none; +} diff --git a/scp/login.php b/scp/login.php index 0fc0d09914..6e1854456c 100644 --- a/scp/login.php +++ b/scp/login.php @@ -30,29 +30,57 @@ $msg = $msg ?: ($content ? $content->getLocalName() : __('Authentication Required')); $dest=($dest && (!strstr($dest,'login.php') && !strstr($dest,'ajax.php')))?$dest:'index.php'; $show_reset = false; -if($_POST) { +if ($_POST) { + $json = isset($_POST['ajax']) && $_POST['ajax']; + $respond = function($code, $message) use ($json, $ost) { + if ($json) { + $payload = is_array($message) ? $message + : array('message' => $message); + $payload['status'] = (int) $code; + Http::response(200, JSONDataEncoder::encode($payload), + 'application/json'); + } + else { + // Extract the `message` portion only + if (is_array($message)) + $message = $message['message']; + Http::response($code, $message); + } + }; + $redirect = function($url) use ($json) { + if ($json) + Http::response(200, JsonDataEncoder::encode(array( + 'status' => 302, 'redirect' => $url)), 'application/json'); + else + Http::redirect($url); + }; + // Check the CSRF token, and ensure that future requests will have to // use a different CSRF token. This will help ward off both parallel and // serial brute force attacks, because new tokens will have to be // requested for each attempt. - if (!$ost->checkCSRFToken()) - Http::response(400, __('Valid CSRF Token Required')); - - // Rotate the CSRF token (original cannot be reused) - $ost->getCSRF()->rotate(); + if (!$ost->checkCSRFToken()) { + $_SESSION['_staff']['auth']['msg'] = __('Valid CSRF Token Required'); + $redirect($_SERVER['REQUEST_URI']); + } // Lookup support backends for this staff $username = trim($_POST['userid']); if ($user = StaffAuthenticationBackend::process($username, $_POST['passwd'], $errors)) { - session_write_close(); - Http::redirect($dest); - require_once('index.php'); //Just incase header is messed up. - exit; + $redirect($dest); } - $msg = $errors['err']?$errors['err']:__('Invalid login'); + $msg = $errors['err'] ?: __('Invalid login'); $show_reset = true; + + if ($json) { + $respond(401, ['message' => $msg, 'show_reset' => $show_reset]); + } + else { + // Rotate the CSRF token (original cannot be reused) + $ost->getCSRF()->rotate(); + } } elseif ($_GET['do']) { switch ($_GET['do']) {