From 475a750e377ec56f76702161d412481ab8024131 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 24 Aug 2026 05:20:17 -0400 Subject: [PATCH 01/10] compat(php86): drop imagedestroy() calls deprecated in PHP 8.5 A GdImage frees itself when its last reference goes away (PHP 8.0+), so imagedestroy() has been a no-op for years and PHP 8.5 deprecates it. Delete the calls in image.php, the image CAPTCHA renderer and Protector's module_icon.php, unsetting where a variable or property held the image. The CAPTCHA renderer's branch-on-destroy-failure unwinds to a plain unset, and its required-GD-function check no longer lists the function. The remaining occurrences are inside the vendored TCPDF, which is upstream's to change. --- docs/changelog.270.txt | 3 +-- htdocs/class/captcha/image.php | 1 - htdocs/class/captcha/image/scripts/image.php | 11 +++++------ htdocs/image.php | 11 ++++++----- htdocs/xoops_lib/modules/protector/module_icon.php | 2 +- 5 files changed, 13 insertions(+), 15 deletions(-) diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index ec31e9c56..c9ce9fdf8 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -14,6 +14,7 @@ Compatibility: - keep code-block whitespace intact and scope the trailing-break trim to XOOPS' own boxes (mamba) in #150 - drop deprecated curl_close() calls for PHP 8.5 (mamba) in #153 - replace the deprecated is_long() alias with is_int() in FunctionsUserTest, the lone repository-wide occurrence; PHP 8.6 deprecates the is_long/is_integer/is_double/doubleval type-alias functions and first-party htdocs/ code has none (mamba) +- drop the imagedestroy() calls, deprecated in PHP 8.5 and no-ops since 8.0 (a GdImage frees itself when its last reference goes away): image.php, the image CAPTCHA renderer (whose required-GD-function check no longer lists it, and whose branch-on-destroy-failure unwinds to a plain unset), and Protector's module_icon.php; the remaining occurrences are inside the vendored TCPDF, which is upstream's to change (mamba) - use bare returns in the four early-exit constructors (class/file/file.php, class/uploader.php, class/xoopsform/formdhtmltextarea.php, class/xoopsform/formselectuser.php): PHP 8.6 deprecates returning a value from __construct() at compile time; new never delivered these values, so the change is behavior-preserving. Verified by token scan - 7 constructor value-returns before, 0 after - and pinned by a direct-invocation test that observes the yielded value (mamba) - drop ReflectionProperty::setAccessible() from the test bootstrap; a no-op since 8.1 and deprecated in 8.5, and emitted during bootstrap it contaminated the output of every test asserting on a clean stream (mamba) in #157 - complete the session save-handler contract ahead of PHP 8.6: add create_sid() (with SessionIdInterface) so the 8.6 deprecation for object handlers lacking it never fires - PHP 9.0 makes the method mandatory; teach updateTimestamp() to insert the row a brand-new empty session is missing, because 8.6 routes such a session there instead of write(), and a bare UPDATE would never create it, leaving strict mode to reject the ID and regenerate it on every request - gated on read() having found no row for the ID, so a session destroyed by a parallel logout keeps its timestamp-only UPDATE and cannot be resurrected with stale data; pin session.use_strict_mode to 1, the 8.6 default, on 8.2-8.5 as well, reporting via E_USER_WARNING when the pin is impossible because output preceded session bootstrap (mamba) in #169 @@ -37,8 +38,6 @@ Security: - guard getByDirname() on the module uninstall and update confirm screens (mamba) in #156 - harden the tplsets template browser, editor, restore, and save endpoints: drop the second urldecode() (PHP already decoded the request once; re-decoding re-materialized %00 and %2f as live bytes) and reject interior NUL bytes explicitly at ingestion at all four sites - getString()'s trim() silently drops edge NULs (so such a value arrives as a valid NUL-free path and faces the same containment checks as any other input) while an interior one passes intact, verified by execution; the accessors themselves stay as they were, because Xmf's PATH filter truncates at the first byte outside its ASCII set, which would break accented or CJK theme paths and turn a Windows restore path into a bare drive letter - with realpath() inside try and ValueError caught as a backstop at all four sites (browser, editor, restore, and the save in main.php), false realpath() results refused, root-plus-separator containment so a sibling like themes2 cannot satisfy a bare prefix check, is_file() required in the editor, save, and restore paths so a directory (even the themes root via path_file=/) never reaches the file handlers, the folder listing switched to the canonical path it just validated, reads pinned to POST to match the JS callers, the template-extension allowlist enforced at the editor as well as the save, the legacy escape() dropped from the tree JS (jQuery already encodes once, so spaced and non-ASCII folder names finally reach the server intact - the removed second decode had been load-bearing for that double-encode), the editor/restore AJAX bodies switched to object form so paths containing & or + survive, restore switched to the validated relative path (ending the server-path leak into admin HTML and the Windows absolute-path special case), a failed backup copy() now aborts the save, the file-listing onclick arguments JS-encoded with json_encode() before HTML-escaping (htmlentities() alone left an apostrophe filename free to break the JS literal), the editor form kept to its single CSRF token with the restore validating it via check(false) so the save can still spend it, the hidden path_file field re-derived from the validated path, the dead file/ext fields dropped, the backup delegated to the shared xoops_write_file_atomically() helper, whose atomic rename() replaces a planted symlink at .back instead of following it (copy() follows an existing link and PHP's emulated fopen('x') follows a dangling one, both verified by execution; an attacker who could exploit the helper's residual reopen-by-name window already has write access inside themes/ and could replace the templates directly), a symlinked .back refused by the restore and its delete-then-rename replaced by a single atomic rename() so a failed restore can no longer leave neither file, the listing/editor output aligned on htmlspecialchars(ENT_QUOTES | ENT_SUBSTITUTE, UTF-8) with json_encode(JSON_INVALID_UTF8_SUBSTITUTE) for the JS arguments - without the SUBSTITUTE flags a non-UTF-8 filename produced a JS parse error or an empty editor textarea that a save would write over the real file, both verified by execution - and tpls_edit_file() slimmed to the two parameters it actually uses (mamba) in #176 - extract the tplsets path-containment contract into the shared PathGuard class (resolveDir/resolveFile: NUL refusal with a ValueError backstop, false-realpath refusal, boundary-aware root containment so a sibling like themes2 never satisfies a prefix check, is_dir/is_file by mode, case-insensitive extension allowlist, canonical result), refactor all four endpoints onto the one implementation, and pin the contract with a truth-table PHPUnit suite that runs against a real fixture tree - traversal, NUL masks, a directory named like a stylesheet, symlink escape refused and an internal symlink canonicalized - and harden the four tpls_generate_surcharge write loops through one checked writer: fopen() verified (an unchecked false handle is a PHP 8 TypeError at fwrite()) and fwrite() compared against false so an empty template source still counts as written (mamba) in #178 - -Database: - render an empty IN list as a constant predicate. Criteria::render() emitted the literal IN (), which MySQL rejects as a syntax error; an empty set is now 1=0 for IN and 1=1 for NOT IN. Returning an empty fragment would be wrong, since CriteriaCompo drops empty fragments and the condition would silently match every row. This also fixes search.php, which built the fragment "mid IN ()" for any user whose module-read permission list was empty (mamba) in #154 - preserve the caller's PHP type in an IN array: an int renders as an SQL integer, anything else as a quoted string literal. Previously a numeric-looking string was cast, so a text-column search for '001' rendered IN (1), which MySQL evaluates numerically and matches against '1', '01' and ' 1'; a list of '001' and '1' collapsed to IN (1,1). Integer columns are unaffected (mamba) in #154 - let the legacy-IN diagnostic be switched on from debug.php. Criteria read XOOPS_DB_LEGACY_LOG only, and that constant lives in mainfile.php, which an upgrade leaves alone - so setting 'legacy_log' in xoops_data/data/debug.php did nothing on an upgraded site, and the warning that tells a developer to migrate a legacy IN list was silent on exactly the installations most likely to contain one. The configuration is read alongside the constant, and the caller backtrace and the E_USER_DEPRECATED notice inside that block now ask xoops_isDebugEnabled() rather than reading XOOPS_DEBUG, which was false on the same sites for the same reason (mamba) diff --git a/htdocs/class/captcha/image.php b/htdocs/class/captcha/image.php index c133e8eca..975b4b1e7 100644 --- a/htdocs/class/captcha/image.php +++ b/htdocs/class/captcha/image.php @@ -42,7 +42,6 @@ public function isActive() 'imagecolorallocate', 'imagefilledrectangle', 'imagejpeg', - 'imagedestroy', 'imageftbbox', ]; foreach ($required_functions as $func) { diff --git a/htdocs/class/captcha/image/scripts/image.php b/htdocs/class/captcha/image/scripts/image.php index aca34f458..039b8d955 100644 --- a/htdocs/class/captcha/image/scripts/image.php +++ b/htdocs/class/captcha/image/scripts/image.php @@ -191,10 +191,9 @@ public function createImageGd() return false; } - if (!imagedestroy($this->oImage)) { - // Log or handle the error as you see fit - return false; - } + // GdImage frees itself when the last reference goes away (PHP 8.0+); + // imagedestroy() is a no-op there and deprecated in PHP 8.5. + unset($this->oImage); return true; } @@ -225,7 +224,7 @@ public function setImageSize() } } } - imagedestroy($oImage); + unset($oImage); // frees the GdImage; imagedestroy() is deprecated in PHP 8.5 $this->height = $MaxCharHeight + 2; $this->spacing = (int)(($this->config['num_chars'] * $MaxCharWidth) / $this->config['num_chars']); @@ -272,7 +271,7 @@ public function createFromFile() } if (!empty($BackgroundImage)) { imagecopyresized($this->oImage, $BackgroundImage, 0, 0, 0, 0, imagesx($this->oImage), imagesy($this->oImage), imagesx($BackgroundImage), imagesy($BackgroundImage)); - imagedestroy($BackgroundImage); + unset($BackgroundImage); // frees the GdImage; imagedestroy() is deprecated in PHP 8.5 } else { $this->drawBars(); } diff --git a/htdocs/image.php b/htdocs/image.php index 708387d75..44d2b4c64 100644 --- a/htdocs/image.php +++ b/htdocs/image.php @@ -183,8 +183,9 @@ function imageCreateCorners($sourceImage, $radii) imagefilledrectangle($destinationImage, 0, 0, $imageWidth, $imageHeight, $alphaColor); imagecopyresampled($destinationImage, $workingImage, 0, 0, 0, 0, $imageWidth, $imageHeight, $workingWidth, $workingHeight); - // imagedestroy($sourceImage); - imagedestroy($workingImage); + // GdImage frees itself when the last reference goes away (PHP 8.0+); + // imagedestroy() is a no-op there and deprecated in PHP 8.5. + unset($workingImage); return $destinationImage; } @@ -712,9 +713,9 @@ function imageFilenameCheck($imageUrl) // Update $image_created_time $imageCreatedTime = time(); -// Clean up the memory -imagedestroy($sourceImage); -imagedestroy($destination_image); +// Clean up the memory — dropping the last reference frees the GdImage on +// PHP 8.0+; imagedestroy() is a no-op there and deprecated in PHP 8.5. +unset($sourceImage, $destination_image); /* * Write the just edited image into the Xoops cache diff --git a/htdocs/xoops_lib/modules/protector/module_icon.php b/htdocs/xoops_lib/modules/protector/module_icon.php index 28993403d..65e32a3e3 100644 --- a/htdocs/xoops_lib/modules/protector/module_icon.php +++ b/htdocs/xoops_lib/modules/protector/module_icon.php @@ -54,7 +54,7 @@ $px = (92 - 6 * strlen($mydirname)) / 2; imagestring($im, 3, $px, 34, $mydirname, $color); imagepng($im); - imagedestroy($im); + unset($im); // frees the GdImage; imagedestroy() is deprecated in PHP 8.5 } else { readfile($icon_fullpath); } From c6a4825309d5d06fcfc72e8eef9647f886453422 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 24 Aug 2026 05:20:27 -0400 Subject: [PATCH 02/10] security(image): close image.php's remote-image branch With ONLY_LOCAL_IMAGES flipped to false, the raw request URL reached getimagesize() and file_get_contents(), honoring http://, phar:// and data:// -- an SSRF and phar-deserialization surface. The branch had also never worked for anyone: it truncated every rooted URL to a single character (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant), so no real-world behavior is lost. The constant stays and the branch now refuses the request, so a fork that flips it fails closed instead of fatal. --- docs/changelog.270.txt | 1 + htdocs/image.php | 17 +++++++++-------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index c9ce9fdf8..e597c1117 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -38,6 +38,7 @@ Security: - guard getByDirname() on the module uninstall and update confirm screens (mamba) in #156 - harden the tplsets template browser, editor, restore, and save endpoints: drop the second urldecode() (PHP already decoded the request once; re-decoding re-materialized %00 and %2f as live bytes) and reject interior NUL bytes explicitly at ingestion at all four sites - getString()'s trim() silently drops edge NULs (so such a value arrives as a valid NUL-free path and faces the same containment checks as any other input) while an interior one passes intact, verified by execution; the accessors themselves stay as they were, because Xmf's PATH filter truncates at the first byte outside its ASCII set, which would break accented or CJK theme paths and turn a Windows restore path into a bare drive letter - with realpath() inside try and ValueError caught as a backstop at all four sites (browser, editor, restore, and the save in main.php), false realpath() results refused, root-plus-separator containment so a sibling like themes2 cannot satisfy a bare prefix check, is_file() required in the editor, save, and restore paths so a directory (even the themes root via path_file=/) never reaches the file handlers, the folder listing switched to the canonical path it just validated, reads pinned to POST to match the JS callers, the template-extension allowlist enforced at the editor as well as the save, the legacy escape() dropped from the tree JS (jQuery already encodes once, so spaced and non-ASCII folder names finally reach the server intact - the removed second decode had been load-bearing for that double-encode), the editor/restore AJAX bodies switched to object form so paths containing & or + survive, restore switched to the validated relative path (ending the server-path leak into admin HTML and the Windows absolute-path special case), a failed backup copy() now aborts the save, the file-listing onclick arguments JS-encoded with json_encode() before HTML-escaping (htmlentities() alone left an apostrophe filename free to break the JS literal), the editor form kept to its single CSRF token with the restore validating it via check(false) so the save can still spend it, the hidden path_file field re-derived from the validated path, the dead file/ext fields dropped, the backup delegated to the shared xoops_write_file_atomically() helper, whose atomic rename() replaces a planted symlink at .back instead of following it (copy() follows an existing link and PHP's emulated fopen('x') follows a dangling one, both verified by execution; an attacker who could exploit the helper's residual reopen-by-name window already has write access inside themes/ and could replace the templates directly), a symlinked .back refused by the restore and its delete-then-rename replaced by a single atomic rename() so a failed restore can no longer leave neither file, the listing/editor output aligned on htmlspecialchars(ENT_QUOTES | ENT_SUBSTITUTE, UTF-8) with json_encode(JSON_INVALID_UTF8_SUBSTITUTE) for the JS arguments - without the SUBSTITUTE flags a non-UTF-8 filename produced a JS parse error or an empty editor textarea that a save would write over the real file, both verified by execution - and tpls_edit_file() slimmed to the two parameters it actually uses (mamba) in #176 - extract the tplsets path-containment contract into the shared PathGuard class (resolveDir/resolveFile: NUL refusal with a ValueError backstop, false-realpath refusal, boundary-aware root containment so a sibling like themes2 never satisfies a prefix check, is_dir/is_file by mode, case-insensitive extension allowlist, canonical result), refactor all four endpoints onto the one implementation, and pin the contract with a truth-table PHPUnit suite that runs against a real fixture tree - traversal, NUL masks, a directory named like a stylesheet, symlink escape refused and an internal symlink canonicalized - and harden the four tpls_generate_surcharge write loops through one checked writer: fopen() verified (an unchecked false handle is a PHP 8 TypeError at fwrite()) and fwrite() compared against false so an empty template source still counts as written (mamba) in #178 +- close image.php's remote-image branch. With ONLY_LOCAL_IMAGES flipped to false the raw request URL reached getimagesize() and file_get_contents(), honoring http://, phar:// and data:// — an SSRF and phar-deserialization surface — and the branch had never worked for anyone anyway: it truncated every rooted URL to a single character (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant), so no real-world behavior is lost. The constant stays and the branch now refuses the request, so a fork that flips it fails closed instead of fatal (mamba) - render an empty IN list as a constant predicate. Criteria::render() emitted the literal IN (), which MySQL rejects as a syntax error; an empty set is now 1=0 for IN and 1=1 for NOT IN. Returning an empty fragment would be wrong, since CriteriaCompo drops empty fragments and the condition would silently match every row. This also fixes search.php, which built the fragment "mid IN ()" for any user whose module-read permission list was empty (mamba) in #154 - preserve the caller's PHP type in an IN array: an int renders as an SQL integer, anything else as a quoted string literal. Previously a numeric-looking string was cast, so a text-column search for '001' rendered IN (1), which MySQL evaluates numerically and matches against '1', '01' and ' 1'; a list of '001' and '1' collapsed to IN (1,1). Integer columns are unaffected (mamba) in #154 - let the legacy-IN diagnostic be switched on from debug.php. Criteria read XOOPS_DB_LEGACY_LOG only, and that constant lives in mainfile.php, which an upgrade leaves alone - so setting 'legacy_log' in xoops_data/data/debug.php did nothing on an upgraded site, and the warning that tells a developer to migrate a legacy IN list was silent on exactly the installations most likely to contain one. The configuration is read alongside the constant, and the caller backtrace and the E_USER_DEPRECATED notice inside that block now ask xoops_isDebugEnabled() rather than reading XOOPS_DEBUG, which was false on the same sites for the same reason (mamba) diff --git a/htdocs/image.php b/htdocs/image.php index 44d2b4c64..ec2816ac4 100644 --- a/htdocs/image.php +++ b/htdocs/image.php @@ -334,19 +334,20 @@ function imageFilenameCheck($imageUrl) exitInvalidRequest(); } } else { - if ($imageUrl[0] === '/') { - $imageUrl = substr($imageUrl, 0, 1); - } - $imagePath = $imageUrl; + // The remote-image branch is gone (2.7.3): it handed the raw request URL + // to getimagesize()/file_get_contents(), honoring http://, phar:// and + // data:// (SSRF and phar-deserialization surface) — and it had never + // worked anyway, since it truncated any rooted URL to a single character + // (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant). The + // constant stays so a fork that flips it fails closed instead of fatal. + exitInvalidRequest(); } // Get the size and MIME type of the requested image $imageFilename = basename($imagePath); // image filename $imagesize = getimagesize($imagePath); // Reject before decode if the source is unreadable or would decode to an - // excessive pixel count (M-14). The byte cap only applies to a local file — - // if ONLY_LOCAL_IMAGES is ever disabled, $imagePath may be a remote URL, where - // is_file()/filesize() do not apply (the pixel cap from getimagesize() still - // does). For a local file a stat failure is a rejection, not a pass. + // excessive pixel count (M-14). $imagePath is always a local file here (the + // remote branch above is closed), so a stat failure is a rejection, not a pass. $isLocalSource = is_file($imagePath); $srcBytes = $isLocalSource ? filesize($imagePath) : false; if (false === $imagesize From 84b478c1e318c07aab324eb0379a2e79856e648a Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 24 Aug 2026 05:20:36 -0400 Subject: [PATCH 03/10] security(profile): require a session token to log out profile's user.php tore the session down on a bare GET, so any third-party page could end a visitor's session at will (forced-logout CSRF). A tokenless request now renders a POST confirmation carrying the token instead of acting immediately, which keeps every existing user.php?op=logout link working -- including the ones baked into cached theme and block templates -- at the cost of one extra click. Adds _US_SURETOLOGOUT to the core user language file and records it in lang_diff.txt. --- docs/changelog.270.txt | 1 + docs/lang_diff.txt | 4 ++++ htdocs/language/english/user.php | 1 + htdocs/modules/profile/user.php | 13 +++++++++++++ 4 files changed, 19 insertions(+) diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index e597c1117..fc2fe9352 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -39,6 +39,7 @@ Security: - harden the tplsets template browser, editor, restore, and save endpoints: drop the second urldecode() (PHP already decoded the request once; re-decoding re-materialized %00 and %2f as live bytes) and reject interior NUL bytes explicitly at ingestion at all four sites - getString()'s trim() silently drops edge NULs (so such a value arrives as a valid NUL-free path and faces the same containment checks as any other input) while an interior one passes intact, verified by execution; the accessors themselves stay as they were, because Xmf's PATH filter truncates at the first byte outside its ASCII set, which would break accented or CJK theme paths and turn a Windows restore path into a bare drive letter - with realpath() inside try and ValueError caught as a backstop at all four sites (browser, editor, restore, and the save in main.php), false realpath() results refused, root-plus-separator containment so a sibling like themes2 cannot satisfy a bare prefix check, is_file() required in the editor, save, and restore paths so a directory (even the themes root via path_file=/) never reaches the file handlers, the folder listing switched to the canonical path it just validated, reads pinned to POST to match the JS callers, the template-extension allowlist enforced at the editor as well as the save, the legacy escape() dropped from the tree JS (jQuery already encodes once, so spaced and non-ASCII folder names finally reach the server intact - the removed second decode had been load-bearing for that double-encode), the editor/restore AJAX bodies switched to object form so paths containing & or + survive, restore switched to the validated relative path (ending the server-path leak into admin HTML and the Windows absolute-path special case), a failed backup copy() now aborts the save, the file-listing onclick arguments JS-encoded with json_encode() before HTML-escaping (htmlentities() alone left an apostrophe filename free to break the JS literal), the editor form kept to its single CSRF token with the restore validating it via check(false) so the save can still spend it, the hidden path_file field re-derived from the validated path, the dead file/ext fields dropped, the backup delegated to the shared xoops_write_file_atomically() helper, whose atomic rename() replaces a planted symlink at .back instead of following it (copy() follows an existing link and PHP's emulated fopen('x') follows a dangling one, both verified by execution; an attacker who could exploit the helper's residual reopen-by-name window already has write access inside themes/ and could replace the templates directly), a symlinked .back refused by the restore and its delete-then-rename replaced by a single atomic rename() so a failed restore can no longer leave neither file, the listing/editor output aligned on htmlspecialchars(ENT_QUOTES | ENT_SUBSTITUTE, UTF-8) with json_encode(JSON_INVALID_UTF8_SUBSTITUTE) for the JS arguments - without the SUBSTITUTE flags a non-UTF-8 filename produced a JS parse error or an empty editor textarea that a save would write over the real file, both verified by execution - and tpls_edit_file() slimmed to the two parameters it actually uses (mamba) in #176 - extract the tplsets path-containment contract into the shared PathGuard class (resolveDir/resolveFile: NUL refusal with a ValueError backstop, false-realpath refusal, boundary-aware root containment so a sibling like themes2 never satisfies a prefix check, is_dir/is_file by mode, case-insensitive extension allowlist, canonical result), refactor all four endpoints onto the one implementation, and pin the contract with a truth-table PHPUnit suite that runs against a real fixture tree - traversal, NUL masks, a directory named like a stylesheet, symlink escape refused and an internal symlink canonicalized - and harden the four tpls_generate_surcharge write loops through one checked writer: fopen() verified (an unchecked false handle is a PHP 8 TypeError at fwrite()) and fwrite() compared against false so an empty template source still counts as written (mamba) in #178 - close image.php's remote-image branch. With ONLY_LOCAL_IMAGES flipped to false the raw request URL reached getimagesize() and file_get_contents(), honoring http://, phar:// and data:// — an SSRF and phar-deserialization surface — and the branch had never worked for anyone anyway: it truncated every rooted URL to a single character (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant), so no real-world behavior is lost. The constant stays and the branch now refuses the request, so a fork that flips it fails closed instead of fatal (mamba) +- require a valid session token to log out. profile's user.php tore the session down on a bare GET, so any third-party page could end a visitor's session at will (forced-logout CSRF). A tokenless request now renders a POST confirmation carrying the token instead of acting immediately, which keeps every existing user.php?op=logout link working — including the ones baked into cached theme and block templates — at the cost of one extra click (mamba) - render an empty IN list as a constant predicate. Criteria::render() emitted the literal IN (), which MySQL rejects as a syntax error; an empty set is now 1=0 for IN and 1=1 for NOT IN. Returning an empty fragment would be wrong, since CriteriaCompo drops empty fragments and the condition would silently match every row. This also fixes search.php, which built the fragment "mid IN ()" for any user whose module-read permission list was empty (mamba) in #154 - preserve the caller's PHP type in an IN array: an int renders as an SQL integer, anything else as a quoted string literal. Previously a numeric-looking string was cast, so a text-column search for '001' rendered IN (1), which MySQL evaluates numerically and matches against '1', '01' and ' 1'; a list of '001' and '1' collapsed to IN (1,1). Integer columns are unaffected (mamba) in #154 - let the legacy-IN diagnostic be switched on from debug.php. Criteria read XOOPS_DB_LEGACY_LOG only, and that constant lives in mainfile.php, which an upgrade leaves alone - so setting 'legacy_log' in xoops_data/data/debug.php did nothing on an upgraded site, and the warning that tells a developer to migrate a legacy IN list was silent on exactly the installations most likely to contain one. The configuration is read alongside the constant, and the caller backtrace and the E_USER_DEPRECATED notice inside that block now ask xoops_isDebugEnabled() rather than reading XOOPS_DEBUG, which was false on the same sites for the same reason (mamba) diff --git a/docs/lang_diff.txt b/docs/lang_diff.txt index b936cc2ee..9412e67f1 100644 --- a/docs/lang_diff.txt +++ b/docs/lang_diff.txt @@ -6,6 +6,10 @@ Below are language differences from a version to next version. ================================ 2026/08: Version 2.7.3 ================================ +/htdocs/language/english/user.php +- added define('_US_SURETOLOGOUT', 'Are you sure you want to log out?'); + (logout now asks for confirmation when the request carries no session token) + /htdocs/class/xoopseditor/sceditor/language/english.php (new file — SCEditor editor) - added define('_XOOPS_EDITOR_SCEDITOR', 'SCEditor (BBCode)'); - added define('_XOOPS_EDITOR_SCEDITOR_STRIKE', 'Strikethrough'); diff --git a/htdocs/language/english/user.php b/htdocs/language/english/user.php index a5a9fb748..77d81b915 100644 --- a/htdocs/language/english/user.php +++ b/htdocs/language/english/user.php @@ -84,6 +84,7 @@ define('_US_EXTRAINFO', 'Extra Info'); define('_US_EDITPROFILE', 'Edit Profile'); define('_US_LOGOUT', 'Logout'); +define('_US_SURETOLOGOUT', 'Are you sure you want to log out?'); define('_US_INBOX', 'Inbox'); define('_US_MEMBERSINCE', 'Member Since'); define('_US_RANK', 'Rank'); diff --git a/htdocs/modules/profile/user.php b/htdocs/modules/profile/user.php index 0543554ec..2bb033fa6 100644 --- a/htdocs/modules/profile/user.php +++ b/htdocs/modules/profile/user.php @@ -93,6 +93,19 @@ } if ($op === 'logout') { + // A logout must present a valid session token: with none required, any + // third-party page could end the visitor's session through a bare GET + // (forced-logout CSRF). Existing tokenless links keep working — they land + // on a POST confirmation (which carries the token) instead of acting + // immediately, so the many cached theme and block templates that emit + // user.php?op=logout links need no change. + if (!$GLOBALS['xoopsSecurity']->check()) { + include $GLOBALS['xoops']->path('header.php'); + include __DIR__ . '/header.php'; + xoops_confirm(['op' => 'logout'], 'user.php', _US_SURETOLOGOUT); + include __DIR__ . '/footer.php'; + exit(); + } $message = ''; // Regenerate a new session id and destroy old session $GLOBALS['sess_handler']->regenerate_id(true); From bf27118083d99dab4b282fa8e23061209185f74b Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 24 Aug 2026 05:20:46 -0400 Subject: [PATCH 04/10] security(redirects): allowlist the query string reflected into Location headers The pm and profile preload events and profile register.php's activation redirect appended $_SERVER['QUERY_STRING'] verbatim to their Location targets -- eight sites. PHP already blocks CRLF in headers, so this was never header splitting, but an attacker-shaped query string reflected unchecked invites cache-poisoning and phishing parameter injection. The string is now appended only when it fits a conservative character allowlist (all core xoops_redirect producers urlencode their values, so legitimate flows pass unchanged) and is dropped otherwise. --- docs/changelog.270.txt | 1 + htdocs/modules/pm/preloads/core.php | 22 +++++++++++++++++++--- htdocs/modules/profile/preloads/core.php | 24 ++++++++++++++++++++---- htdocs/modules/profile/register.php | 8 +++++++- 4 files changed, 47 insertions(+), 8 deletions(-) diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index fc2fe9352..721776213 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -40,6 +40,7 @@ Security: - extract the tplsets path-containment contract into the shared PathGuard class (resolveDir/resolveFile: NUL refusal with a ValueError backstop, false-realpath refusal, boundary-aware root containment so a sibling like themes2 never satisfies a prefix check, is_dir/is_file by mode, case-insensitive extension allowlist, canonical result), refactor all four endpoints onto the one implementation, and pin the contract with a truth-table PHPUnit suite that runs against a real fixture tree - traversal, NUL masks, a directory named like a stylesheet, symlink escape refused and an internal symlink canonicalized - and harden the four tpls_generate_surcharge write loops through one checked writer: fopen() verified (an unchecked false handle is a PHP 8 TypeError at fwrite()) and fwrite() compared against false so an empty template source still counts as written (mamba) in #178 - close image.php's remote-image branch. With ONLY_LOCAL_IMAGES flipped to false the raw request URL reached getimagesize() and file_get_contents(), honoring http://, phar:// and data:// — an SSRF and phar-deserialization surface — and the branch had never worked for anyone anyway: it truncated every rooted URL to a single character (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant), so no real-world behavior is lost. The constant stays and the branch now refuses the request, so a fork that flips it fails closed instead of fatal (mamba) - require a valid session token to log out. profile's user.php tore the session down on a bare GET, so any third-party page could end a visitor's session at will (forced-logout CSRF). A tokenless request now renders a POST confirmation carrying the token instead of acting immediately, which keeps every existing user.php?op=logout link working — including the ones baked into cached theme and block templates — at the cost of one extra click (mamba) +- stop reflecting the raw QUERY_STRING into redirect Location headers at all eight sites (three pm preload events, four profile preload events, and profile's register.php activation redirect). PHP already blocks CRLF in headers, so this was never header splitting, but an attacker-shaped query string reflected verbatim invites cache-poisoning and phishing parameter injection; the string is now appended only when it fits a conservative character allowlist (all core xoops_redirect producers urlencode, so legitimate flows pass), and dropped otherwise (mamba) - render an empty IN list as a constant predicate. Criteria::render() emitted the literal IN (), which MySQL rejects as a syntax error; an empty set is now 1=0 for IN and 1=1 for NOT IN. Returning an empty fragment would be wrong, since CriteriaCompo drops empty fragments and the condition would silently match every row. This also fixes search.php, which built the fragment "mid IN ()" for any user whose module-read permission list was empty (mamba) in #154 - preserve the caller's PHP type in an IN array: an int renders as an SQL integer, anything else as a quoted string literal. Previously a numeric-looking string was cast, so a text-column search for '001' rendered IN (1), which MySQL evaluates numerically and matches against '1', '01' and ' 1'; a list of '001' and '1' collapsed to IN (1,1). Integer columns are unaffected (mamba) in #154 - let the legacy-IN diagnostic be switched on from debug.php. Criteria read XOOPS_DB_LEGACY_LOG only, and that constant lives in mainfile.php, which an upgrade leaves alone - so setting 'legacy_log' in xoops_data/data/debug.php did nothing on an upgraded site, and the warning that tells a developer to migrate a legacy IN list was silent on exactly the installations most likely to contain one. The configuration is read alongside the constant, and the caller backtrace and the E_USER_DEPRECATED notice inside that block now ask xoops_isDebugEnabled() rather than reading XOOPS_DEBUG, which was false on the same sites for the same reason (mamba) diff --git a/htdocs/modules/pm/preloads/core.php b/htdocs/modules/pm/preloads/core.php index 117fa372a..fda88a4b3 100644 --- a/htdocs/modules/pm/preloads/core.php +++ b/htdocs/modules/pm/preloads/core.php @@ -29,12 +29,28 @@ */ class PmCorePreload extends XoopsPreloadItem { + /** + * The current request's query string, '?' included, ready to append to a + * redirect target — or '' when it is absent or contains anything outside a + * conservative allowlist. The raw QUERY_STRING is attacker-controlled, and + * reflecting it unchecked into a Location header invites cache-poisoning + * and phishing parameter injection (CRLF itself is already blocked by PHP). + * + * @return string + */ + private static function filteredQueryString() + { + $queryString = $_SERVER['QUERY_STRING'] ?? ''; + + return preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; + } + /** * @param $args */ public static function eventCorePmliteStart($args) { - header('location: ./modules/pm/pmlite.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + header('location: ./modules/pm/pmlite.php' . self::filteredQueryString()); exit(); } @@ -43,7 +59,7 @@ public static function eventCorePmliteStart($args) */ public static function eventCoreReadpmsgStart($args) { - header('location: ./modules/pm/readpmsg.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + header('location: ./modules/pm/readpmsg.php' . self::filteredQueryString()); exit(); } @@ -52,7 +68,7 @@ public static function eventCoreReadpmsgStart($args) */ public static function eventCoreViewpmsgStart($args) { - header('location: ./modules/pm/viewpmsg.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + header('location: ./modules/pm/viewpmsg.php' . self::filteredQueryString()); exit(); } diff --git a/htdocs/modules/profile/preloads/core.php b/htdocs/modules/profile/preloads/core.php index e10712d77..df5ad9f1f 100644 --- a/htdocs/modules/profile/preloads/core.php +++ b/htdocs/modules/profile/preloads/core.php @@ -31,6 +31,22 @@ */ class ProfileCorePreload extends XoopsPreloadItem { + /** + * The current request's query string, '?' included, ready to append to a + * redirect target — or '' when it is absent or contains anything outside a + * conservative allowlist. The raw QUERY_STRING is attacker-controlled, and + * reflecting it unchecked into a Location header invites cache-poisoning + * and phishing parameter injection (CRLF itself is already blocked by PHP). + * + * @return string + */ + private static function filteredQueryString() + { + $queryString = $_SERVER['QUERY_STRING'] ?? ''; + + return preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; + } + /** * @param $args */ @@ -44,7 +60,7 @@ public static function eventCoreUserStart($args) } $from = Request::getString('from', '', 'GET'); if ($op !== 'login' && $from !== 'profile') { - header('location: ./modules/profile/user.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + header('location: ./modules/profile/user.php' . self::filteredQueryString()); exit(); } } @@ -54,7 +70,7 @@ public static function eventCoreUserStart($args) */ public static function eventCoreEdituserStart($args) { - header('location: ./modules/profile/edituser.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + header('location: ./modules/profile/edituser.php' . self::filteredQueryString()); exit(); } @@ -74,7 +90,7 @@ public static function eventCoreLostpassStart($args) */ public static function eventCoreRegisterStart($args) { - header('location: ./modules/profile/register.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + header('location: ./modules/profile/register.php' . self::filteredQueryString()); exit(); } @@ -83,7 +99,7 @@ public static function eventCoreRegisterStart($args) */ public static function eventCoreUserinfoStart($args) { - header('location: ./modules/profile/userinfo.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + header('location: ./modules/profile/userinfo.php' . self::filteredQueryString()); exit(); } } diff --git a/htdocs/modules/profile/register.php b/htdocs/modules/profile/register.php index 69e9eb7cb..f52d8f117 100644 --- a/htdocs/modules/profile/register.php +++ b/htdocs/modules/profile/register.php @@ -29,7 +29,13 @@ $regOp = Request::getString('op', '', 'GET'); if ($regOp !== '' && in_array($regOp, ['actv', 'activate'])) { - header('location: ./activate.php' . (empty($_SERVER['QUERY_STRING']) ? '' : '?' . $_SERVER['QUERY_STRING'])); + // Append the query string only when it fits a conservative allowlist: the + // raw QUERY_STRING is attacker-controlled, and reflecting it unchecked into + // a Location header invites cache-poisoning and phishing parameter + // injection (CRLF itself is already blocked by PHP). + $queryString = $_SERVER['QUERY_STRING'] ?? ''; + $queryString = preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; + header('location: ./activate.php' . $queryString); exit(); } From b180fcb2d70b98f7ee82083a0bb5155a013b8d99 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 24 Aug 2026 07:33:15 -0400 Subject: [PATCH 05/10] security(redirects): rebuild the reflected query string from parsed args Follow-up to the #179 review: the character allowlist reflected malformed percent-escapes verbatim ("%ZZ" and a trailing "%" match the class -- Copilot, three sites) and its 512-byte cap dropped a long urlencoded xoops_redirect entirely, so a login lost its destination (Greptile P1, reproduced with a 1072-byte publisher search URL). The filter now parses the query string with parse_str() and re-emits it with http_build_query(PHP_QUERY_RFC3986), capped at 2000 bytes for header-size sanity: every reflected byte is RFC 3986-safe or a valid escape by construction, hostile input becomes inert encoding instead of costing the visitor their query, and the redirect target parses the rebuilt string identically to the original, since parse_str() mirrors PHP's own request parsing (round-trip equivalence probed by execution). --- docs/changelog.270.txt | 2 +- htdocs/modules/pm/preloads/core.php | 21 ++++++++++++++++----- htdocs/modules/profile/preloads/core.php | 21 ++++++++++++++++----- htdocs/modules/profile/register.php | 19 ++++++++++++++----- 4 files changed, 47 insertions(+), 16 deletions(-) diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index 721776213..4b7607f7c 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -40,7 +40,7 @@ Security: - extract the tplsets path-containment contract into the shared PathGuard class (resolveDir/resolveFile: NUL refusal with a ValueError backstop, false-realpath refusal, boundary-aware root containment so a sibling like themes2 never satisfies a prefix check, is_dir/is_file by mode, case-insensitive extension allowlist, canonical result), refactor all four endpoints onto the one implementation, and pin the contract with a truth-table PHPUnit suite that runs against a real fixture tree - traversal, NUL masks, a directory named like a stylesheet, symlink escape refused and an internal symlink canonicalized - and harden the four tpls_generate_surcharge write loops through one checked writer: fopen() verified (an unchecked false handle is a PHP 8 TypeError at fwrite()) and fwrite() compared against false so an empty template source still counts as written (mamba) in #178 - close image.php's remote-image branch. With ONLY_LOCAL_IMAGES flipped to false the raw request URL reached getimagesize() and file_get_contents(), honoring http://, phar:// and data:// — an SSRF and phar-deserialization surface — and the branch had never worked for anyone anyway: it truncated every rooted URL to a single character (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant), so no real-world behavior is lost. The constant stays and the branch now refuses the request, so a fork that flips it fails closed instead of fatal (mamba) - require a valid session token to log out. profile's user.php tore the session down on a bare GET, so any third-party page could end a visitor's session at will (forced-logout CSRF). A tokenless request now renders a POST confirmation carrying the token instead of acting immediately, which keeps every existing user.php?op=logout link working — including the ones baked into cached theme and block templates — at the cost of one extra click (mamba) -- stop reflecting the raw QUERY_STRING into redirect Location headers at all eight sites (three pm preload events, four profile preload events, and profile's register.php activation redirect). PHP already blocks CRLF in headers, so this was never header splitting, but an attacker-shaped query string reflected verbatim invites cache-poisoning and phishing parameter injection; the string is now appended only when it fits a conservative character allowlist (all core xoops_redirect producers urlencode, so legitimate flows pass), and dropped otherwise (mamba) +- stop reflecting the raw QUERY_STRING into redirect Location headers at all eight sites (three pm preload events, four profile preload events, and profile's register.php activation redirect). PHP already blocks CRLF in headers, so this was never header splitting, but an attacker-shaped query string reflected verbatim invites cache-poisoning and phishing parameter injection; the string is now parsed with parse_str() — which mirrors how the redirect target itself reads the parameters — and re-emitted with http_build_query(), so every reflected byte is RFC 3986-safe or a valid escape by construction, hostile input becomes inert encoding instead of costing the visitor their query, and a long urlencoded xoops_redirect survives (the 2000-byte cap is header-size sanity only) (mamba) - render an empty IN list as a constant predicate. Criteria::render() emitted the literal IN (), which MySQL rejects as a syntax error; an empty set is now 1=0 for IN and 1=1 for NOT IN. Returning an empty fragment would be wrong, since CriteriaCompo drops empty fragments and the condition would silently match every row. This also fixes search.php, which built the fragment "mid IN ()" for any user whose module-read permission list was empty (mamba) in #154 - preserve the caller's PHP type in an IN array: an int renders as an SQL integer, anything else as a quoted string literal. Previously a numeric-looking string was cast, so a text-column search for '001' rendered IN (1), which MySQL evaluates numerically and matches against '1', '01' and ' 1'; a list of '001' and '1' collapsed to IN (1,1). Integer columns are unaffected (mamba) in #154 - let the legacy-IN diagnostic be switched on from debug.php. Criteria read XOOPS_DB_LEGACY_LOG only, and that constant lives in mainfile.php, which an upgrade leaves alone - so setting 'legacy_log' in xoops_data/data/debug.php did nothing on an upgraded site, and the warning that tells a developer to migrate a legacy IN list was silent on exactly the installations most likely to contain one. The configuration is read alongside the constant, and the caller backtrace and the E_USER_DEPRECATED notice inside that block now ask xoops_isDebugEnabled() rather than reading XOOPS_DEBUG, which was false on the same sites for the same reason (mamba) diff --git a/htdocs/modules/pm/preloads/core.php b/htdocs/modules/pm/preloads/core.php index fda88a4b3..ebc586485 100644 --- a/htdocs/modules/pm/preloads/core.php +++ b/htdocs/modules/pm/preloads/core.php @@ -31,18 +31,29 @@ class PmCorePreload extends XoopsPreloadItem { /** * The current request's query string, '?' included, ready to append to a - * redirect target — or '' when it is absent or contains anything outside a - * conservative allowlist. The raw QUERY_STRING is attacker-controlled, and - * reflecting it unchecked into a Location header invites cache-poisoning - * and phishing parameter injection (CRLF itself is already blocked by PHP). + * redirect target — or '' when there is nothing usable. The raw + * QUERY_STRING is attacker-controlled, and reflecting it unchecked into a + * Location header invites cache-poisoning and phishing parameter injection + * (CRLF itself is already blocked by PHP). Rather than gate on a character + * allowlist — which reflected malformed percent-escapes verbatim and cost + * a long urlencoded xoops_redirect its whole query — the string is parsed + * with parse_str(), which mirrors how the redirect target itself will read + * it, and re-emitted with http_build_query(), so every reflected byte is + * RFC 3986-safe or a valid escape by construction. The length cap is + * header-size sanity only. * * @return string */ private static function filteredQueryString() { $queryString = $_SERVER['QUERY_STRING'] ?? ''; + if ('' === $queryString || strlen($queryString) > 2000) { + return ''; + } + parse_str($queryString, $params); + $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); - return preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; + return ('' === $rebuilt) ? '' : ('?' . $rebuilt); } /** diff --git a/htdocs/modules/profile/preloads/core.php b/htdocs/modules/profile/preloads/core.php index df5ad9f1f..e79401dab 100644 --- a/htdocs/modules/profile/preloads/core.php +++ b/htdocs/modules/profile/preloads/core.php @@ -33,18 +33,29 @@ class ProfileCorePreload extends XoopsPreloadItem { /** * The current request's query string, '?' included, ready to append to a - * redirect target — or '' when it is absent or contains anything outside a - * conservative allowlist. The raw QUERY_STRING is attacker-controlled, and - * reflecting it unchecked into a Location header invites cache-poisoning - * and phishing parameter injection (CRLF itself is already blocked by PHP). + * redirect target — or '' when there is nothing usable. The raw + * QUERY_STRING is attacker-controlled, and reflecting it unchecked into a + * Location header invites cache-poisoning and phishing parameter injection + * (CRLF itself is already blocked by PHP). Rather than gate on a character + * allowlist — which reflected malformed percent-escapes verbatim and cost + * a long urlencoded xoops_redirect its whole query — the string is parsed + * with parse_str(), which mirrors how the redirect target itself will read + * it, and re-emitted with http_build_query(), so every reflected byte is + * RFC 3986-safe or a valid escape by construction. The length cap is + * header-size sanity only. * * @return string */ private static function filteredQueryString() { $queryString = $_SERVER['QUERY_STRING'] ?? ''; + if ('' === $queryString || strlen($queryString) > 2000) { + return ''; + } + parse_str($queryString, $params); + $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); - return preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; + return ('' === $rebuilt) ? '' : ('?' . $rebuilt); } /** diff --git a/htdocs/modules/profile/register.php b/htdocs/modules/profile/register.php index f52d8f117..e0d512899 100644 --- a/htdocs/modules/profile/register.php +++ b/htdocs/modules/profile/register.php @@ -29,12 +29,21 @@ $regOp = Request::getString('op', '', 'GET'); if ($regOp !== '' && in_array($regOp, ['actv', 'activate'])) { - // Append the query string only when it fits a conservative allowlist: the - // raw QUERY_STRING is attacker-controlled, and reflecting it unchecked into - // a Location header invites cache-poisoning and phishing parameter - // injection (CRLF itself is already blocked by PHP). + // Rebuild the query string before reflecting it: the raw QUERY_STRING is + // attacker-controlled, and appended unchecked to a Location header it + // invites cache-poisoning and phishing parameter injection (CRLF itself is + // already blocked by PHP). parse_str() mirrors how activate.php will read + // the parameters, and http_build_query() re-emits them with every byte + // RFC 3986-safe or a valid escape by construction; the length cap is + // header-size sanity only. $queryString = $_SERVER['QUERY_STRING'] ?? ''; - $queryString = preg_match('/^[A-Za-z0-9_\[\]=&%;.\-]{1,512}$/', $queryString) ? ('?' . $queryString) : ''; + if ('' === $queryString || strlen($queryString) > 2000) { + $queryString = ''; + } else { + parse_str($queryString, $queryParams); + $rebuilt = http_build_query($queryParams, '', '&', PHP_QUERY_RFC3986); + $queryString = ('' === $rebuilt) ? '' : ('?' . $rebuilt); + } header('location: ./activate.php' . $queryString); exit(); } From 452434122dbf2232e0664d037e70f84ee0aa4e03 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Mon, 24 Aug 2026 07:45:55 -0400 Subject: [PATCH 06/10] security(redirects): share one query-string rebuild and pin it with a test Review follow-up on #179: the parse_str()/http_build_query() rebuild was duplicated verbatim in the pm and profile preloads and inline in register.php. It now lives once as xoops_rebuildQueryString() in include/file_safety.php -- the side-effect-free helper home already hosting the redirect-safety functions, guarded with function_exists() like its neighbors -- and all three call sites delegate to it. A new RebuildQueryStringTest pins the contract: malformed escapes re-emitted as valid ones, hostile input reduced to inert encoding, every emitted byte RFC 3986-safe or a valid escape, unusable input dropped to '', and the >512-byte urlencoded xoops_redirect surviving with round-trip parse equivalence at the target. --- docs/changelog.270.txt | 2 +- htdocs/include/file_safety.php | 31 +++++++ htdocs/modules/pm/preloads/core.php | 24 ++---- htdocs/modules/profile/preloads/core.php | 24 ++---- htdocs/modules/profile/register.php | 23 ++---- .../htdocs/include/RebuildQueryStringTest.php | 82 +++++++++++++++++++ 6 files changed, 133 insertions(+), 53 deletions(-) create mode 100644 tests/unit/htdocs/include/RebuildQueryStringTest.php diff --git a/docs/changelog.270.txt b/docs/changelog.270.txt index 4b7607f7c..4f8bc4401 100644 --- a/docs/changelog.270.txt +++ b/docs/changelog.270.txt @@ -40,7 +40,7 @@ Security: - extract the tplsets path-containment contract into the shared PathGuard class (resolveDir/resolveFile: NUL refusal with a ValueError backstop, false-realpath refusal, boundary-aware root containment so a sibling like themes2 never satisfies a prefix check, is_dir/is_file by mode, case-insensitive extension allowlist, canonical result), refactor all four endpoints onto the one implementation, and pin the contract with a truth-table PHPUnit suite that runs against a real fixture tree - traversal, NUL masks, a directory named like a stylesheet, symlink escape refused and an internal symlink canonicalized - and harden the four tpls_generate_surcharge write loops through one checked writer: fopen() verified (an unchecked false handle is a PHP 8 TypeError at fwrite()) and fwrite() compared against false so an empty template source still counts as written (mamba) in #178 - close image.php's remote-image branch. With ONLY_LOCAL_IMAGES flipped to false the raw request URL reached getimagesize() and file_get_contents(), honoring http://, phar:// and data:// — an SSRF and phar-deserialization surface — and the branch had never worked for anyone anyway: it truncated every rooted URL to a single character (substr($imageUrl, 0, 1) where substr($imageUrl, 1) was meant), so no real-world behavior is lost. The constant stays and the branch now refuses the request, so a fork that flips it fails closed instead of fatal (mamba) - require a valid session token to log out. profile's user.php tore the session down on a bare GET, so any third-party page could end a visitor's session at will (forced-logout CSRF). A tokenless request now renders a POST confirmation carrying the token instead of acting immediately, which keeps every existing user.php?op=logout link working — including the ones baked into cached theme and block templates — at the cost of one extra click (mamba) -- stop reflecting the raw QUERY_STRING into redirect Location headers at all eight sites (three pm preload events, four profile preload events, and profile's register.php activation redirect). PHP already blocks CRLF in headers, so this was never header splitting, but an attacker-shaped query string reflected verbatim invites cache-poisoning and phishing parameter injection; the string is now parsed with parse_str() — which mirrors how the redirect target itself reads the parameters — and re-emitted with http_build_query(), so every reflected byte is RFC 3986-safe or a valid escape by construction, hostile input becomes inert encoding instead of costing the visitor their query, and a long urlencoded xoops_redirect survives (the 2000-byte cap is header-size sanity only) (mamba) +- stop reflecting the raw QUERY_STRING into redirect Location headers at all eight sites (three pm preload events, four profile preload events, and profile's register.php activation redirect). PHP already blocks CRLF in headers, so this was never header splitting, but an attacker-shaped query string reflected verbatim invites cache-poisoning and phishing parameter injection; the string is now parsed with parse_str() — which mirrors how the redirect target itself reads the parameters — and re-emitted with http_build_query(), so every reflected byte is RFC 3986-safe or a valid escape by construction, hostile input becomes inert encoding instead of costing the visitor their query, and a long urlencoded xoops_redirect survives (the 2000-byte cap is header-size sanity only); the rebuild lives in include/file_safety.php as xoops_rebuildQueryString() — one implementation shared by all three call sites — and is pinned by a unit test (mamba) - render an empty IN list as a constant predicate. Criteria::render() emitted the literal IN (), which MySQL rejects as a syntax error; an empty set is now 1=0 for IN and 1=1 for NOT IN. Returning an empty fragment would be wrong, since CriteriaCompo drops empty fragments and the condition would silently match every row. This also fixes search.php, which built the fragment "mid IN ()" for any user whose module-read permission list was empty (mamba) in #154 - preserve the caller's PHP type in an IN array: an int renders as an SQL integer, anything else as a quoted string literal. Previously a numeric-looking string was cast, so a text-column search for '001' rendered IN (1), which MySQL evaluates numerically and matches against '1', '01' and ' 1'; a list of '001' and '1' collapsed to IN (1,1). Integer columns are unaffected (mamba) in #154 - let the legacy-IN diagnostic be switched on from debug.php. Criteria read XOOPS_DB_LEGACY_LOG only, and that constant lives in mainfile.php, which an upgrade leaves alone - so setting 'legacy_log' in xoops_data/data/debug.php did nothing on an upgraded site, and the warning that tells a developer to migrate a legacy IN list was silent on exactly the installations most likely to contain one. The configuration is read alongside the constant, and the caller backtrace and the E_USER_DEPRECATED notice inside that block now ask xoops_isDebugEnabled() rather than reading XOOPS_DEBUG, which was false on the same sites for the same reason (mamba) diff --git a/htdocs/include/file_safety.php b/htdocs/include/file_safety.php index 4cc9db50c..da88ca6bc 100644 --- a/htdocs/include/file_safety.php +++ b/htdocs/include/file_safety.php @@ -10,6 +10,7 @@ * - xoops_chmod_quietly() — scoped-suppressed chmod() with single warning * - xoops_remove_file_quietly() — scoped-suppressed unlink() with single warning * - xoops_isLocalUrl() — strict same-origin check (scheme/host/port) for redirects + * - xoops_rebuildQueryString() — parse-and-re-emit a query string for safe reflection * * They originally lived in include/cp_functions.php, but that file * unconditionally `define()`s XOOPS_CPFUNC_LOADED, which include/ @@ -309,3 +310,33 @@ function xoops_isLocalUrl($url) return $sameHost && $sameScheme && $samePort; } } + +if (!function_exists('xoops_rebuildQueryString')) { + /** + * Rebuild a query string so it is safe to reflect into a redirect + * Location header, '?' included — or return '' when there is nothing + * usable. The raw QUERY_STRING is attacker-controlled: appended verbatim + * it invites cache-poisoning and phishing parameter injection (CRLF + * itself is already blocked by PHP's header()). Instead of gating on a + * character allowlist — which reflects malformed percent-escapes + * verbatim and costs a long urlencoded xoops_redirect its whole query — + * the string is parsed with parse_str(), which mirrors how the redirect + * target itself will read it, and re-emitted with http_build_query(), so + * every reflected byte is RFC 3986-safe or a valid escape by + * construction. The length cap is header-size sanity only. + * + * @param string $queryString raw query string (e.g. $_SERVER['QUERY_STRING']) + * @return string '?' plus the rebuilt query string, or '' when empty, oversized or unparseable + */ + function xoops_rebuildQueryString($queryString) + { + $queryString = (string) $queryString; + if ('' === $queryString || strlen($queryString) > 2000) { + return ''; + } + parse_str($queryString, $params); + $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); + + return ('' === $rebuilt) ? '' : ('?' . $rebuilt); + } +} diff --git a/htdocs/modules/pm/preloads/core.php b/htdocs/modules/pm/preloads/core.php index ebc586485..a75c49e94 100644 --- a/htdocs/modules/pm/preloads/core.php +++ b/htdocs/modules/pm/preloads/core.php @@ -30,30 +30,18 @@ class PmCorePreload extends XoopsPreloadItem { /** - * The current request's query string, '?' included, ready to append to a - * redirect target — or '' when there is nothing usable. The raw - * QUERY_STRING is attacker-controlled, and reflecting it unchecked into a - * Location header invites cache-poisoning and phishing parameter injection - * (CRLF itself is already blocked by PHP). Rather than gate on a character - * allowlist — which reflected malformed percent-escapes verbatim and cost - * a long urlencoded xoops_redirect its whole query — the string is parsed - * with parse_str(), which mirrors how the redirect target itself will read - * it, and re-emitted with http_build_query(), so every reflected byte is - * RFC 3986-safe or a valid escape by construction. The length cap is - * header-size sanity only. + * The current request's query string, rebuilt for safe reflection into a + * redirect Location target, '?' included — or '' when there is nothing + * usable. One shared implementation: see xoops_rebuildQueryString() in + * include/file_safety.php for the threat model. * * @return string */ private static function filteredQueryString() { - $queryString = $_SERVER['QUERY_STRING'] ?? ''; - if ('' === $queryString || strlen($queryString) > 2000) { - return ''; - } - parse_str($queryString, $params); - $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); + require_once XOOPS_ROOT_PATH . '/include/file_safety.php'; - return ('' === $rebuilt) ? '' : ('?' . $rebuilt); + return xoops_rebuildQueryString($_SERVER['QUERY_STRING'] ?? ''); } /** diff --git a/htdocs/modules/profile/preloads/core.php b/htdocs/modules/profile/preloads/core.php index e79401dab..9a00a8369 100644 --- a/htdocs/modules/profile/preloads/core.php +++ b/htdocs/modules/profile/preloads/core.php @@ -32,30 +32,18 @@ class ProfileCorePreload extends XoopsPreloadItem { /** - * The current request's query string, '?' included, ready to append to a - * redirect target — or '' when there is nothing usable. The raw - * QUERY_STRING is attacker-controlled, and reflecting it unchecked into a - * Location header invites cache-poisoning and phishing parameter injection - * (CRLF itself is already blocked by PHP). Rather than gate on a character - * allowlist — which reflected malformed percent-escapes verbatim and cost - * a long urlencoded xoops_redirect its whole query — the string is parsed - * with parse_str(), which mirrors how the redirect target itself will read - * it, and re-emitted with http_build_query(), so every reflected byte is - * RFC 3986-safe or a valid escape by construction. The length cap is - * header-size sanity only. + * The current request's query string, rebuilt for safe reflection into a + * redirect Location target, '?' included — or '' when there is nothing + * usable. One shared implementation: see xoops_rebuildQueryString() in + * include/file_safety.php for the threat model. * * @return string */ private static function filteredQueryString() { - $queryString = $_SERVER['QUERY_STRING'] ?? ''; - if ('' === $queryString || strlen($queryString) > 2000) { - return ''; - } - parse_str($queryString, $params); - $rebuilt = http_build_query($params, '', '&', PHP_QUERY_RFC3986); + require_once XOOPS_ROOT_PATH . '/include/file_safety.php'; - return ('' === $rebuilt) ? '' : ('?' . $rebuilt); + return xoops_rebuildQueryString($_SERVER['QUERY_STRING'] ?? ''); } /** diff --git a/htdocs/modules/profile/register.php b/htdocs/modules/profile/register.php index e0d512899..3e18c12f9 100644 --- a/htdocs/modules/profile/register.php +++ b/htdocs/modules/profile/register.php @@ -29,22 +29,13 @@ $regOp = Request::getString('op', '', 'GET'); if ($regOp !== '' && in_array($regOp, ['actv', 'activate'])) { - // Rebuild the query string before reflecting it: the raw QUERY_STRING is - // attacker-controlled, and appended unchecked to a Location header it - // invites cache-poisoning and phishing parameter injection (CRLF itself is - // already blocked by PHP). parse_str() mirrors how activate.php will read - // the parameters, and http_build_query() re-emits them with every byte - // RFC 3986-safe or a valid escape by construction; the length cap is - // header-size sanity only. - $queryString = $_SERVER['QUERY_STRING'] ?? ''; - if ('' === $queryString || strlen($queryString) > 2000) { - $queryString = ''; - } else { - parse_str($queryString, $queryParams); - $rebuilt = http_build_query($queryParams, '', '&', PHP_QUERY_RFC3986); - $queryString = ('' === $rebuilt) ? '' : ('?' . $rebuilt); - } - header('location: ./activate.php' . $queryString); + // Rebuild the query string before reflecting it into the Location header: + // reflected verbatim, the attacker-controlled QUERY_STRING invites + // cache-poisoning and phishing parameter injection. One shared + // implementation: see xoops_rebuildQueryString() in include/file_safety.php + // for the threat model. + require_once XOOPS_ROOT_PATH . '/include/file_safety.php'; + header('location: ./activate.php' . xoops_rebuildQueryString($_SERVER['QUERY_STRING'] ?? '')); exit(); } diff --git a/tests/unit/htdocs/include/RebuildQueryStringTest.php b/tests/unit/htdocs/include/RebuildQueryStringTest.php new file mode 100644 index 000000000..205f981f7 --- /dev/null +++ b/tests/unit/htdocs/include/RebuildQueryStringTest.php @@ -0,0 +1,82 @@ + */ + public static function rebuiltStrings(): array + { + return [ + 'plain pair' => ['uid=5', '?uid=5'], + 'multiple pairs' => ['send=1&to_userid=5', '?send=1&to_userid=5'], + 'token passthrough' => ['op=logout&XOOPS_TOKEN_REQUEST=abc123', '?op=logout&XOOPS_TOKEN_REQUEST=abc123'], + 'malformed escape' => ['a=%ZZ', '?a=%25ZZ'], + 'trailing percent' => ['a=%', '?a=%25'], + 'encoded crlf' => ['evil=%0d%0aSet-Cookie:x=1', '?evil=%0D%0ASet-Cookie%3Ax%3D1'], + 'markup' => ['a=