Fix/273 hardening

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,9 @@ 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:
+- 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)
 - 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)

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');

htdocs/class/captcha/image.php

@@ -42,7 +42,6 @@ public function isActive()
                 'imagecolorallocate',
                 'imagefilledrectangle',
                 'imagejpeg',
-                'imagedestroy',
                 'imageftbbox',
             ];
             foreach ($required_functions as $func) {

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();
         }

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;
 }
@@ -333,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
@@ -712,9 +714,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

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');

htdocs/modules/pm/preloads/core.php

@@ -29,12 +29,39 @@
  */
 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.
+     *
+     * @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 ('' === $rebuilt) ? '' : ('?' . $rebuilt);
+    }
+
     /**
      * @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 +70,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 +79,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();
     }