Skip to content

RC-scoped 3.6.4 fixes for PHP 8, dblayertrap, and filter loading #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mambax7 merged 7 commits into from
Apr 21, 2026
Merged

RC-scoped 3.6.4 fixes for PHP 8, dblayertrap, and filter loading #28

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0e9c91d
fix(protector): RC-scoped stability and compatibility tightening for …
mambax7 Apr 20, 2026
98f081d
fix(protector): clarify 1.1 comment wording and tighten RC test helpers
mambax7 Apr 20, 2026
9443f97
test(protector): harden RC regression tests for determinism and fixt…
mambax7 Apr 20, 2026
25fe8fd
docs(protector): clarify Fix 1.2 rationale wording and Fix 1.5 test …
mambax7 Apr 20, 2026
28b3052
test(protector): clear superglobals before Protector::getInstance() …
mambax7 Apr 20, 2026
89140bf
fix(protector): skip directories and unreadable files in filter loader
mambax7 Apr 20, 2026
00f372c
test(protector): match both quote styles in allowlist regression check
mambax7 Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 42 additions & 11 deletions htdocs/xoops_lib/modules/protector/class/ProtectorFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,18 +91,49 @@ public function execute($type)
$ret = 0;

$dh = opendir($this->filters_base);
if (false === $dh) {
return $ret;
}

// Fix 9.1: resolve the filters_enabled base path once so every candidate
// filter can be verified to live inside it. The previous loader included
// every file readdir() returned, which allowed a writable filters_enabled
// directory to achieve RCE if an attacker could place or symlink a file
// whose name happened to start with the requested type prefix.
$baseReal = realpath($this->filters_base);

while (($file = readdir($dh)) !== false) {
if (strncmp($file, $type . '_', strlen($type) + 1) === 0) {
include_once $this->filters_base . '/' . $file;
$plugin_name = 'protector_' . substr($file, 0, -4);
if (function_exists($plugin_name)) {
// old way
$ret |= call_user_func($plugin_name);
} elseif (class_exists($plugin_name)) {
// newer way
$plugin_obj = new $plugin_name(); //old code is -> $plugin_obj =& new $plugin_name() ; //hack by Trabis
$ret |= $plugin_obj->execute();
}
if (strncmp($file, $type . '_', strlen($type) + 1) !== 0) {
continue;
}
// Require .php suffix — blocks .phtml, .inc, .phar and other executable
// extensions that may be interpreted as PHP by misconfigured servers.
// Case-insensitive: Windows NTFS and macOS HFS+ resolve case-variant
// filenames as the same file, and pre-existing custom filters may use
// .PHP / .Php. strcasecmp lets those continue to load on case-sensitive
// filesystems too.
if (0 !== strcasecmp(substr($file, -4), '.php')) {
continue;
}
// Resolve the real path and ensure it stays inside filters_enabled.
// Blocks symlinks pointing outside the directory and any crafted name
// whose canonical path escapes the base.
$realPath = realpath($this->filters_base . '/' . $file);
if (false === $realPath
|| false === $baseReal
|| !str_starts_with($realPath, $baseReal . DIRECTORY_SEPARATOR)) {
continue;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realpath() + prefix containment prevents traversal, but the loader still include_onces without first asserting the target is a regular readable file. If filters_enabled/ contains a directory (or unreadable file) matching the {$type}_*.php pattern, this will emit warnings during request handling (and may leak filesystem paths when display_errors is on). Consider adding an is_file() / is_readable() check on $realPath before including, similar to the pattern used for custom PHP blocks.

Suggested change
}
}
// Only include regular readable files. This avoids runtime warnings
// when a matching entry is a directory or otherwise unreadable.
if (!is_file($realPath) || !is_readable($realPath)) {
trigger_error('Unable to load Protector filter: ' . basename($realPath), E_USER_WARNING);
continue;
}

Copilot uses AI. Check for mistakes.

include_once $realPath;
$plugin_name = 'protector_' . substr($file, 0, -4);
if (function_exists($plugin_name)) {
// old way
$ret |= call_user_func($plugin_name);
} elseif (class_exists($plugin_name)) {
// newer way
$plugin_obj = new $plugin_name(); //old code is -> $plugin_obj =& new $plugin_name() ; //hack by Trabis
$ret |= $plugin_obj->execute();
}
}
closedir($dh);
Expand Down
11 changes: 10 additions & 1 deletion htdocs/xoops_lib/modules/protector/class/ProtectorMysqlDatabase.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,16 @@ public function injectionFound($sql)
$protector->last_error_type = 'SQL Injection';
$protector->message .= $sql;
$protector->output_log($protector->last_error_type);
die('SQL Injection found');

// Fix 1.4: reply with HTTP 403 and a generic body. The previous
// terminator returned HTTP 200 with a distinctive plain-text body,
// which fingerprinted the dblayertrap response and allowed attackers
// to probe the module's sensitivity character-by-character. A generic
// 'Forbidden' response matches any other blocked-request path.
if (!headers_sent()) {
http_response_code(403);
}
die('Forbidden');
}

/**
Expand Down
60 changes: 56 additions & 4 deletions htdocs/xoops_lib/modules/protector/class/protector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,10 @@ protected function _initial_recursive($val, $key)
}

// register as doubtful requests against SQL Injections
if (preg_match('?[\s\'"`/]?', $val)) {
// Fix 1.1: '?' is not a valid PCRE delimiter. The previous pattern returned
// false from preg_match() on every call, disabling _doubtful_requests
// population and neutralising check_sql_union() and check_sql_isolatedcommentin().
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new comment claims that '?' is not a valid PCRE delimiter and that the old pattern made preg_match() return false. In PHP regex patterns, '?' is a valid delimiter character; the change to '#' may still be desirable for readability, but the rationale here is inaccurate/misleading. Please adjust the comment to reflect the real reason for changing delimiters (clarity/avoid delimiter/quantifier confusion) or document the actual failure mode observed.

Suggested change
// Fix 1.1: '?' is not a valid PCRE delimiter. The previous pattern returned
// false from preg_match() on every call, disabling _doubtful_requests
// population and neutralising check_sql_union() and check_sql_isolatedcommentin().
// Use '#' as the PCRE delimiter for clarity so the pattern is easier to
// read and does not visually confuse the delimiter with regex
// metacharacters such as '?' used elsewhere in patterns.

Copilot uses AI. Check for mistakes.
if (preg_match('#[\s\'"`/]#', $val)) {
$this->_doubtful_requests[(string)$key] = $val;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}
Expand Down Expand Up @@ -242,7 +245,11 @@ public function purgeSession()
public function purgeCookies()
{
if (!headers_sent()) {
$domain = defined(XOOPS_COOKIE_DOMAIN) ? XOOPS_COOKIE_DOMAIN : '';
// Fix 1.2: defined() requires the constant name as a string literal.
// Without quotes PHP 8 throws a Fatal Error when XOOPS_COOKIE_DOMAIN is absent;
// when it is defined, PHP evaluates it first and passes its value to defined(),
// which is then always false.
Comment thread
mambax7 marked this conversation as resolved.
Outdated
$domain = defined('XOOPS_COOKIE_DOMAIN') ? XOOPS_COOKIE_DOMAIN : '';
$past = time() - 3600;
foreach ($_COOKIE as $key => $value) {
setcookie($key, '', $past, '', $domain);
Expand Down Expand Up @@ -681,7 +688,44 @@ public function dblayertrap_init($force_override = false)
$this->_dblayertrap_check_recursive($_POST);
$this->_dblayertrap_check_recursive($_COOKIE);
if (empty($this->_conf['dblayertrap_wo_server'])) {
$this->_dblayertrap_check_recursive($_SERVER);
// Fix 1.5: scan only a positive allowlist of server-intrinsic $_SERVER keys.
// Any denylist over this surface is incomplete. Request-derived keys are
// excluded — attacker-controlled values in them would otherwise poison
// _dblayertrap_doubtfuls and trigger false-positive cascades on queries
// that happen to share a substring with the attacker's input.
//
// Excluded classes:
// - HTTP_* (all request headers expand to this namespace)
// - CONTENT_TYPE, CONTENT_LENGTH (request headers outside HTTP_*)
// - PHP_AUTH_USER, PHP_AUTH_PW, PHP_AUTH_DIGEST, AUTH_TYPE (auth headers)
// - QUERY_STRING, REQUEST_URI, PATH_INFO, PATH_TRANSLATED, ORIG_PATH_INFO (URL parts)
// - REDIRECT_* (mod_rewrite/redirect context)
// - PHP_SELF, SCRIPT_NAME (request-derived on URL-rewriting deployments;
// Protector itself has documented XSS handling for these elsewhere)
// - SERVER_NAME (reflects the Host header when UseCanonicalName is Off,
// which is common on Apache and default on many reverse-proxied setups)
// - REQUEST_METHOD (client-supplied token from the request line; Apache
// and many other servers pass arbitrary method names like SELECT or
// UNION straight through to PHP)
// - SERVER_PROTOCOL (client-supplied token from the request line; a
// lenient server can surface values like SELECT/1.0, and its realistic
// contents HTTP/1.0 | HTTP/1.1 | HTTP/2.0 offer no SQLi signal anyway)
//
// The existing dblayertrap_wo_server config flag remains as the escape hatch
// for admins who want to skip the $_SERVER scan entirely.
static $serverScanAllowlist = [
'SERVER_ADDR' => true,
'SERVER_PORT' => true,
'SERVER_SOFTWARE' => true,
'GATEWAY_INTERFACE' => true,
'DOCUMENT_ROOT' => true,
'REQUEST_SCHEME' => true,
'REMOTE_ADDR' => true,
'REMOTE_PORT' => true,
'SCRIPT_FILENAME' => true,
];
$serverSafe = array_intersect_key($_SERVER, $serverScanAllowlist);
$this->_dblayertrap_check_recursive($serverSafe);
}

if (!empty($this->_dblayertrap_doubtfuls) || $force_override) {
Expand Down Expand Up @@ -1284,7 +1328,15 @@ public function check_dos_attack($uid = 0, $can_ban = false)
}

// Check its Agent
if ('' != trim($this->_conf['dos_crsafe']) && isset($_SERVER['HTTP_USER_AGENT']) && preg_match($this->_conf['dos_crsafe'], $_SERVER['HTTP_USER_AGENT'])) {
// Fix 1.3 (runtime guard): an admin-supplied regex that is syntactically invalid
// would emit a PHP warning on every request. Probe validity against an empty
// subject first; silently ignore the setting if invalid.
// NOTE: this check covers SYNTAX only. A syntactically-valid but catastrophically
// backtracking pattern (ReDoS) still runs against the real User-Agent. Full
// admin-side validation and PCRE backtrack-limit handling defer to 3.7.0.
$crsafe_pattern = trim((string)($this->_conf['dos_crsafe'] ?? ''));
$crsafe_valid = '' !== $crsafe_pattern && false !== @preg_match($crsafe_pattern, '');
if ($crsafe_valid && isset($_SERVER['HTTP_USER_AGENT']) && preg_match($crsafe_pattern, $_SERVER['HTTP_USER_AGENT'])) {
// welcomed crawler
$this->_done_dos = true;

Expand Down
Loading
Loading