diff --git a/tests/php/test_auth_request_string.php b/tests/php/test_auth_request_string.php new file mode 100644 index 000000000..d095c1349 --- /dev/null +++ b/tests/php/test_auth_request_string.php @@ -0,0 +1,83 @@ + 'admin'); +check('string passes through', requestString('user'), 'admin'); + +// The empty string is still a string: callers decide what empty means, and the +// login paths already test for it with empty(). +$_REQUEST = array('user' => ''); +check('empty string is kept', requestString('user'), ''); + +// A parameter that was never sent. +$_REQUEST = array(); +check('missing parameter is null', requestString('user'), null); + +// The shape the user edit form posts. This is the reported crash. +$_REQUEST = array('user' => array('Username' => 'admin', 'Password' => 'secret')); +check('form array is refused', requestString('user'), null); + +// The shape an attacker sends, on the paths that need no session. +$_REQUEST = array('username' => array('admin'), 'password' => array('x')); +check('username array is refused', requestString('username'), null); +check('password array is refused', requestString('password'), null); + +// An auth hash is used to look up a user and compared against the session copy. +$_REQUEST = array('auth' => array('deadbeef')); +check('auth array is refused', requestString('auth'), null); + +// Anything else PHP can put in a request parameter. +$_REQUEST = array('user' => array(array('nested'))); +check('nested array is refused', requestString('user'), null); + +echo "\n$passes passed, $failures failed\n"; +exit($failures ? 1 : 0); diff --git a/web/includes/auth.php b/web/includes/auth.php index ffeb25985..e32df6ae4 100644 --- a/web/includes/auth.php +++ b/web/includes/auth.php @@ -29,6 +29,25 @@ require_once('Role_Monitor_Permission.php'); require_once(__DIR__.'/../vendor/autoload.php'); use \Firebase\JWT\JWT; +// A request parameter as a string, or null when the client did not send one it +// can be used as. Request values are whatever arrived: "?user[]=x" makes +// $_REQUEST['user'] an array, and the first string operation on an array is a +// fatal TypeError under PHP 8. +// +// Not only hostile input. The user edit form posts user[Username], +// user[Password] and the rest, so $_REQUEST['user'] is an array on every save +// from that page, which was enough to take the whole auth path down with +// "strcasecmp(): Argument #1 must be of type string, array given". +// +// An array is not a username, a password or an auth hash, so callers treat it +// the same as a parameter that was never sent. +function requestString($key) { + if (!isset($_REQUEST[$key])) { + return null; + } + return is_string($_REQUEST[$key]) ? $_REQUEST[$key] : null; +} + function password_type($password) { if (!$password || $password === '') { return 'plain'; @@ -226,7 +245,8 @@ function getAuthUser($auth) { // Prefer the username from the URL (matches what zms uses) so PHP and the // C++ side query the same row. Fall back to the session username for // page-internal calls that don't carry user= on the URL. - $requestedUser = !empty($_REQUEST['user']) ? $_REQUEST['user'] : null; + $requestedUser = requestString('user'); + if ($requestedUser === '') $requestedUser = null; $sessionUser = isset($_SESSION['username']) ? $_SESSION['username'] : null; $filterUser = $requestedUser !== null ? $requestedUser : $sessionUser; @@ -640,28 +660,33 @@ function zm_authenticate_request() { zm_session_start(); } - if (ZM_AUTH_HASH_LOGINS && empty($user) && !empty($_REQUEST['auth'])) { - $user = getAuthUser($_REQUEST['auth']); + $requestAuth = requestString('auth'); + $requestUser = requestString('user'); + $requestPass = requestString('pass'); + $requestUsername = requestString('username'); + $requestPassword = requestString('password'); + if (ZM_AUTH_HASH_LOGINS && empty($user) && !empty($requestAuth)) { + $user = getAuthUser($requestAuth); if ($user) { $remoteAddr = ZM_AUTH_HASH_IPS ? $_SESSION['remoteAddr'] : ''; - if (isset($_SESSION['AuthHash'.$remoteAddr]) and ($_SESSION['AuthHash'.$remoteAddr] != $_REQUEST['auth'])) { + if (isset($_SESSION['AuthHash'.$remoteAddr]) and ($_SESSION['AuthHash'.$remoteAddr] != $requestAuth)) { unset($_SESSION['AuthHashGeneratedAt']); unset($_SESSION['AuthHash'.$remoteAddr]); } $_SESSION['username'] = $user->Username(); } - } else if (!(empty($_REQUEST['user']) or empty($_REQUEST['pass']))) { + } else if (!(empty($requestUser) or empty($requestPass))) { # The shortened versions are used in auth_relay = PLAIN - $ret = validateUser($_REQUEST['user'], $_REQUEST['pass']); + $ret = validateUser($requestUser, $requestPass); if (!$ret[0]) { ZM\Warning($ret[1]); $user = null; // null, not unset: in a function unset() drops only the local binding return; } $user = $ret[0]; - } else if (!(empty($_REQUEST['username']) or empty($_REQUEST['password']))) { + } else if (!(empty($requestUsername) or empty($requestPassword))) { # Longer versions are used on login page - $ret = validateUser($_REQUEST['username'], $_REQUEST['password']); + $ret = validateUser($requestUsername, $requestPassword); if (!$ret[0]) { ZM\Warning($ret[1]); $user = null; // null, not unset: in a function unset() drops only the local binding @@ -710,8 +735,8 @@ function zm_authenticate_request() { # Drop the pre-auth session and issue a fresh id in a single Set-Cookie zm_session_regenerate_id_login(); - $username = $_REQUEST['username']; - $password = $_REQUEST['password']; + $username = $requestUsername; + $password = $requestPassword; ZM\Info("Login successful for user \"$username\""); #ZM\Audit("user=$username action=login id=".$user->Id()." from=".($_SERVER['REMOTE_ADDR'] ?? 'local'));