From 2b90bf15a6d357819b16c56dcf24dec2baf5892d Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Fri, 22 Feb 2019 09:43:38 -0500 Subject: [PATCH] Improve session (#2487) * Introduce ZM_COOKIE_LIFETIME which sets the life of the SESSION cookie, instead of using what is in php.ini * Use zm specific session functions, which are now located in includes/session.php. Be more agressive about clearing session on logout. * Move session code to includes/session.php * remove duplicate line * Move is_session_open to session.php. Move code to clear a session into session.php * improve debug line when there is a problem updating config entry * split description into description and help text for COOKIE_LIFETIME * Remove redirect on line. We do it in javascript on postlogin view so that we can say logging in before switching to console * If there is a username in the session, then we are logged in, but we need to load the user object from the db. We can't just trust it from the session. The user may have been deleted and having that data in the session can be a security risk. So load the user object on every request. * Use session_regenerate_id instead of our broken code to do the same * Move auth code to includes/auth.php * add autocomplete tags to username and password inputs * Don't redirect to login if we are already viewing login. Put auth before including skin includes * need to include session.php in auth.php * update to php namespace --- .../ZoneMinder/lib/ZoneMinder/Config.pm.in | 2 +- .../lib/ZoneMinder/ConfigData.pm.in | 8 +++ web/includes/actions/login.php | 1 - web/includes/auth.php | 56 +++++++++------- web/includes/session.php | 65 +++++++++++++++++++ web/index.php | 57 +++++++--------- 6 files changed, 131 insertions(+), 58 deletions(-) create mode 100644 web/includes/session.php diff --git a/scripts/ZoneMinder/lib/ZoneMinder/Config.pm.in b/scripts/ZoneMinder/lib/ZoneMinder/Config.pm.in index 92085b07b..3fdeb722c 100644 --- a/scripts/ZoneMinder/lib/ZoneMinder/Config.pm.in +++ b/scripts/ZoneMinder/lib/ZoneMinder/Config.pm.in @@ -257,7 +257,7 @@ sub saveConfigToDB { $option->{category}, $option->{readonly} ? 1 : 0, $option->{db_requires} - ) or croak( "Can't execute: ".$sth->errstr() ); + ) or croak("Can't execute when updating config entry $$option{name}: ".$sth->errstr() ); } # end foreach option $sth->finish(); diff --git a/scripts/ZoneMinder/lib/ZoneMinder/ConfigData.pm.in b/scripts/ZoneMinder/lib/ZoneMinder/ConfigData.pm.in index 536389062..cd53d6754 100644 --- a/scripts/ZoneMinder/lib/ZoneMinder/ConfigData.pm.in +++ b/scripts/ZoneMinder/lib/ZoneMinder/ConfigData.pm.in @@ -3937,6 +3937,14 @@ our @options = ( type => $types{string}, category => 'mail', }, + { + name => 'ZM_COOKIE_LIFETIME', + default => '3600', + description => q`The maximum life of a COOKIE used when setting up PHP's session handler.`, + help => q`This will affect how long a session will be valid for since the last request. Keeping this short helps prevent session hijacking. Keeping it long allows you to stay logged in longer without refreshing the view.`, + type => $types{integer}, + category => 'system', + } ); our %options_hash = map { ( $_->{name}, $_ ) } @options; diff --git a/web/includes/actions/login.php b/web/includes/actions/login.php index 5e35987c8..48257fa28 100644 --- a/web/includes/actions/login.php +++ b/web/includes/actions/login.php @@ -29,7 +29,6 @@ if ( $action == 'login' && isset($_REQUEST['username']) && ( ZM_AUTH_TYPE == 're $view = 'login'; } else { $view = 'postlogin'; - $redirect = '?view=console'; } } ?> diff --git a/web/includes/auth.php b/web/includes/auth.php index 9f12a2b8f..c16a1e7b8 100644 --- a/web/includes/auth.php +++ b/web/includes/auth.php @@ -17,6 +17,8 @@ // along with this program; if not, write to the Free Software // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. // +// +require_once('session.php'); function userLogin($username='', $password='', $passwordHashed=false) { global $user; @@ -88,12 +90,12 @@ function userLogin($username='', $password='', $passwordHashed=false) { $_SESSION['remoteAddr'] = $_SERVER['REMOTE_ADDR']; // To help prevent session hijacking if ( $dbUser = dbFetchOne($sql, NULL, $sql_values) ) { ZM\Info("Login successful for user \"$username\""); - $_SESSION['user'] = $user = $dbUser; + $user = $dbUser; unset($_SESSION['loginFailed']); if ( ZM_AUTH_TYPE == 'builtin' ) { $_SESSION['passwordHash'] = $user['Password']; } - session_regenerate_id(); + zm_session_regenerate_id(); } else { ZM\Warning("Login denied for user \"$username\""); $_SESSION['loginFailed'] = true; @@ -107,10 +109,8 @@ function userLogin($username='', $password='', $passwordHashed=false) { function userLogout() { global $user; ZM\Info('User "'.$user['Username'].'" logged out'); - session_start(); - unset($_SESSION['user']); unset($user); - session_destroy(); + zm_session_clear(); } function getAuthUser($auth) { @@ -164,7 +164,7 @@ function generateAuthHash($useRemoteAddr, $force=false) { } else { $authKey = ZM_AUTH_HASH_SECRET.$_SESSION['username'].$_SESSION['passwordHash'].$local_time[2].$local_time[3].$local_time[4].$local_time[5]; } - #Logger::Debug("Generated using hour:".$local_time[2] . ' mday:' . $local_time[3] . ' month:'.$local_time[4] . ' year: ' . $local_time[5] ); + #ZM\Logger::Debug("Generated using hour:".$local_time[2] . ' mday:' . $local_time[3] . ' month:'.$local_time[4] . ' year: ' . $local_time[5] ); $auth = md5($authKey); if ( !$force ) { $close_session = 0; @@ -178,9 +178,9 @@ function generateAuthHash($useRemoteAddr, $force=false) { } else { return $auth; } - #Logger::Debug("Generated new auth $auth at " . $_SESSION['AuthHashGeneratedAt']. " using $authKey" ); + #ZM\Logger::Debug("Generated new auth $auth at " . $_SESSION['AuthHashGeneratedAt']. " using $authKey" ); #} else { - #Logger::Debug("Using cached auth " . $_SESSION['AuthHash'] ." beacuse generatedat:" . $_SESSION['AuthHashGeneratedAt'] . ' < now:'. $time . ' - ' . ZM_AUTH_HASH_TTL . ' * 1800 = '. $mintime); + #ZM\Logger::Debug("Using cached auth " . $_SESSION['AuthHash'] ." beacuse generatedat:" . $_SESSION['AuthHashGeneratedAt'] . ' < now:'. $time . ' - ' . ZM_AUTH_HASH_TTL . ' * 1800 = '. $mintime); } # end if AuthHash is not cached return $_SESSION['AuthHash'.$_SESSION['remoteAddr']]; } # end if using AUTH and AUTH_RELAY @@ -205,31 +205,39 @@ function canEdit($area, $mid=false) { return ( $user[$area] == 'Edit' && ( !$mid || visibleMonitor($mid) )); } -function is_session_started() { - if ( php_sapi_name() !== 'cli' ) { - if ( version_compare(phpversion(), '5.4.0', '>=') ) { - return session_status() === PHP_SESSION_ACTIVE ? TRUE : FALSE; - } else { - return session_id() === '' ? FALSE : TRUE; - } - } else { - ZM\Warning("php_sapi_name === 'cli'"); - } - return FALSE; -} - if ( ZM_OPT_USE_AUTH ) { - if ( ZM_AUTH_HASH_LOGINS && empty($user) && ! empty($_REQUEST['auth']) ) { + if ( isset($_SESSION['username']) ) { + # Need to refresh permissions and validate that the user still exists + $sql = 'SELECT * FROM Users WHERE Enabled=1 AND Username=?'; + $user = dbFetchOne($sql, NULL, array($_SESSION['username'])); + } + + $close_session = 0; + if ( !is_session_started() ) { + session_start(); + $close_session = 1; + } + + if ( ZM_AUTH_RELAY == 'plain' ) { + // Need to save this in session + $_SESSION['password'] = $password; + } + $_SESSION['remoteAddr'] = $_SERVER['REMOTE_ADDR']; // To help prevent session hijacking + + if ( ZM_AUTH_HASH_LOGINS && empty($user) && !empty($_REQUEST['auth']) ) { if ( $authUser = getAuthUser($_REQUEST['auth']) ) { userLogin($authUser['Username'], $authUser['Password'], true); } - } - else if ( isset($_REQUEST['username']) and isset($_REQUEST['password']) ) { + } else if ( isset($_REQUEST['username']) and isset($_REQUEST['password']) ) { userLogin($_REQUEST['username'], $_REQUEST['password'], false); } if ( !empty($user) ) { // generate it once here, while session is open. Value will be cached in session and return when called later on generateAuthHash(ZM_AUTH_HASH_IPS); } + if ( $close_session ) + session_write_close(); +} else { + $user = $defaultUser; } ?> diff --git a/web/includes/session.php b/web/includes/session.php new file mode 100644 index 000000000..7da3ee4fb --- /dev/null +++ b/web/includes/session.php @@ -0,0 +1,65 @@ +=') ) { + return session_status() === PHP_SESSION_ACTIVE ? TRUE : FALSE; + } else { + return session_id() === '' ? FALSE : TRUE; + } + } else { + Warning("php_sapi_name === 'cli'"); + } + return FALSE; +} // function is_session_started() + +function zm_session_clear() { + session_start(); + $_SESSION = array(); + if ( ini_get('session.use_cookies') ) { + $p = session_get_cookie_params(); + # Update the cookie to expire in the past. + setcookie(session_name(), '', time() - 31536000, $p['path'], $p['domain'], $p['secure'], $p['httponly']); + } + session_unset(); + session_destroy(); +} // function zm_session_clear() +?> diff --git a/web/index.php b/web/index.php index dfc460f17..a2d50ae00 100644 --- a/web/index.php +++ b/web/index.php @@ -45,6 +45,7 @@ if ( false ) { } require_once('includes/config.php'); +require_once('includes/session.php'); require_once('includes/logger.php'); require_once('includes/Server.php'); require_once('includes/Storage.php'); @@ -115,39 +116,28 @@ if ( !file_exists(ZM_SKIN_PATH) ) Fatal("Invalid skin '$skin'"); $skinBase[] = $skin; -$currentCookieParams = session_get_cookie_params(); -//Logger::Debug('Setting cookie parameters to lifetime('.$currentCookieParams['lifetime'].') path('.$currentCookieParams['path'].') domain ('.$currentCookieParams['domain'].') secure('.$currentCookieParams['secure'].') httpOnly(1)'); -session_set_cookie_params( - $currentCookieParams['lifetime'], - $currentCookieParams['path'], - $currentCookieParams['domain'], - $currentCookieParams['secure'], - true -); +zm_session_start(); -ini_set('session.name', 'ZMSESSID'); - -session_start(); - -if ( !isset($_SESSION['skin']) || isset($_REQUEST['skin']) || !isset($_COOKIE['zmSkin']) || $_COOKIE['zmSkin'] != $skin ) { +if ( + !isset($_SESSION['skin']) || + isset($_REQUEST['skin']) || + !isset($_COOKIE['zmSkin']) || + $_COOKIE['zmSkin'] != $skin +) { $_SESSION['skin'] = $skin; setcookie('zmSkin', $skin, time()+3600*24*30*12*10); } -if ( !isset($_SESSION['css']) || isset($_REQUEST['css']) || !isset($_COOKIE['zmCSS']) || $_COOKIE['zmCSS'] != $css ) { +if ( + !isset($_SESSION['css']) || + isset($_REQUEST['css']) || + !isset($_COOKIE['zmCSS']) || + $_COOKIE['zmCSS'] != $css +) { $_SESSION['css'] = $css; setcookie('zmCSS', $css, time()+3600*24*30*12*10); } -if ( ZM_OPT_USE_AUTH ) { - if ( isset($_SESSION['user']) ) { - $user = $_SESSION['user']; - } else { - unset($user); - } -} else { - $user = $defaultUser; -} # Only one request can open the session file at a time, so let's close the session here to improve concurrency. # Any file/page that sets session variables must re-open it. session_write_close(); @@ -180,12 +170,14 @@ $request = null; if ( isset($_REQUEST['request']) ) $request = detaintPath($_REQUEST['request']); -foreach ( getSkinIncludes('skin.php') as $includeFile ) - require_once $includeFile; - # User Login will be performed in auth.php require_once('includes/auth.php'); +foreach ( getSkinIncludes('skin.php') as $includeFile ) { + #Logger::Debug("including $includeFile"); + require_once $includeFile; +} + if ( isset($_REQUEST['action']) ) $action = detaintPath($_REQUEST['action']); @@ -221,19 +213,20 @@ if ( $action ) { } # If I put this here, it protects all views and popups, but it has to go after actions.php because actions.php does the actual logging in. -if ( ZM_OPT_USE_AUTH and !isset($user) ) { +if ( ZM_OPT_USE_AUTH and !isset($user) and ($view != 'login') ) { Logger::Debug('Redirecting to login'); - $view = 'login'; + $view = 'none'; + $redirect = ZM_BASE_URL.$_SERVER['PHP_SELF'].'?view=login'; $request = null; } else if ( ZM_SHOW_PRIVACY && ($view != 'privacy') && ($view != 'options') && (!$request) && canEdit('System') ) { - Logger::Debug('Redirecting to privacy'); - $view = 'privacy'; - $request = null; + $view = 'none'; + $redirect = ZM_BASE_URL.$_SERVER['PHP_SELF'].'?view=privacy'; } CSPHeaders($view, $cspNonce); if ( $redirect ) { + Logger::Debug("Redirecting to $redirect"); header('Location: '.$redirect); return; }