From e924bfb99996653fdfe2e097dc2bfe069af36df7 Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Thu, 17 Sep 2026 19:44:48 -0400 Subject: [PATCH] fix: treat a request parameter that is not a string as absent in auth Saving a user from the web ui took the whole auth path down: PHP Fatal error: Uncaught TypeError: strcasecmp(): Argument #1 ($string1) must be of type string, array given in includes/auth.php:197 #0 auth.php(197): strcasecmp() #1 auth.php(528): getAuthUser() #2 auth.php(687): userFromSession() reached from ?view=user&uid=2. That page's form posts user[Username], user[Password], user[Name] and the rest, so $_REQUEST['user'] is an array on every save from it, and getAuthUser() read that parameter as the username to filter on and handed it to strcasecmp(). Under PHP 8 a string function given an array is a TypeError rather than a warning, so the request died with a 500. The same shape arrives from anyone who cares to send it, and not only on a page that needs a session. userFromSession() reads user, pass, username, password and auth straight out of the request, and the credential branches run before anyone is logged in, so ?username[]=x&password[]=y reaches validateUser() with arrays on an install that has never seen the caller before. requestString() returns a parameter only when it is a string and null otherwise, which is what the callers already do with a parameter that was not sent. An array is not a username, a password or an auth hash. master no longer has the strcasecmp line the report names, so it does not fatal in that exact spot, but it reads the same unvalidated values: $filterUser is bound as a query parameter and the credentials still reach validateUser(). This fixes the class rather than the one line, and backports to 1.38 where the reported line lives. The test lifts requestString() out of auth.php and evaluates it alone, because including auth.php needs a database; test_auth_no_include_side_effects.php sidesteps the same dependency the same way. 8 cases, covering the form's array, the login parameters, a nested array and the strings that must still pass through. 5 of them fail without this change. Co-Authored-By: Claude Opus 5 (cherry picked from commit 1f4aa1a16c1b63fdf34a6f2c6aef6590dbd45acd) --- tests/php/test_auth_request_string.php | 83 ++++++++++++++++++++++++++ web/includes/auth.php | 45 ++++++++++---- 2 files changed, 118 insertions(+), 10 deletions(-) create mode 100644 tests/php/test_auth_request_string.php 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'));