fix(security): replace basename-only sanitiser with a trusted-enumeration check

The previous commit's os.path.basename() + os.path.join() pattern (which a
pre-existing code comment claimed CodeQL recognises as a sanitiser) did not
actually clear the alert -- the next CodeQL run still flagged the same 2
sink lines, plus a new one at the os.path.join() call itself. Taking a
substring of tainted data apparently isn't treated as a barrier by this
query, whatever the comment assumed.

Replaced it with find_trusted_subdir(): enumerate the trusted plugins_dir
via os.scandir() and only use a name that scandir itself produced, matched
by equality against the caller's requested name. The path is then built
from that enumerated entry, not from the caller's string -- a value
sourced from iterating a trusted, non-tainted directory carries no taint
regardless of what it happens to equal, which is a stronger and more
conventional allowlist-style barrier than string-stripping. Applied
identically in both PluginLoader.install_dependencies() and
StoreManager._install_dependencies(), sharing one implementation.

Re-verified: all 65 tests across test_plugin_loader.py (20, including the
2 new security regression tests), test_store_manager_caches.py (35),
test_plugin_system.py (10) pass, plus the full CI plugin-safety suite
(test_harness.py/test_visual_rendering.py/test_plugin_matrix.py: 52
passed, 2 pre-existing skips).
This commit is contained in:
ChuckBuilds
2026-07-10 15:17:25 -04:00
parent d59a66133b
commit b89653c836
2 changed files with 51 additions and 31 deletions
+35 -15
View File
@@ -93,6 +93,27 @@ def requirements_are_satisfied(requirements_file: str) -> bool:
return True return True
def find_trusted_subdir(trusted_dir: str, name: str) -> Optional[str]:
"""Return `name` if it names an actual subdirectory of trusted_dir, else None.
Used as a containment check for a directory name derived from untrusted
input (a manifest-declared plugin id, an externally-supplied plugin
path): the returned value always comes from enumerating trusted_dir
itself via os.scandir(), so a caller that builds a path by joining
trusted_dir with this return value is joining against a name the
filesystem produced under a trusted root -- not the caller's original
string, which could otherwise smuggle a traversal sequence through.
"""
try:
with os.scandir(trusted_dir) as entries:
for entry in entries:
if entry.name == name and entry.is_dir():
return entry.name
except OSError:
pass
return None
class PluginLoader: class PluginLoader:
"""Handles plugin module loading and class instantiation.""" """Handles plugin module loading and class instantiation."""
@@ -200,9 +221,9 @@ class PluginLoader:
except (json.JSONDecodeError, Exception) as e: except (json.JSONDecodeError, Exception) as e:
self.logger.debug("Skipping %s due to manifest error: %s", item.name, e) self.logger.debug("Skipping %s due to manifest error: %s", item.name, e)
continue continue
return None return None
def install_dependencies( def install_dependencies(
self, self,
plugin_dir: Path, plugin_dir: Path,
@@ -233,25 +254,24 @@ class PluginLoader:
# Resolve to a canonical absolute path (normalises .. and symlinks) # Resolve to a canonical absolute path (normalises .. and symlinks)
plugin_dir_real = os.path.realpath(str(plugin_dir)) plugin_dir_real = os.path.realpath(str(plugin_dir))
# Reconstruct the plugin path from a trusted base + a sanitised
# directory name. os.path.basename() is CodeQL's recognised
# py/path-injection sanitiser: it strips all directory components
# so the result cannot contain traversal sequences. Joining it
# with the resolved, trusted plugins_dir produces a path that
# CodeQL considers untainted.
plugins_dir_real = os.path.realpath(str(plugins_dir)) plugins_dir_real = os.path.realpath(str(plugins_dir))
safe_dir_name = os.path.basename(plugin_dir_real) requested_name = os.path.basename(plugin_dir_real)
if not safe_dir_name:
self.logger.error("Could not determine plugin directory name for %s", plugin_id) # Match the requested directory against an entry actually enumerated
return False # from the trusted plugins_dir, and build the path from that entry --
safe_plugin_dir = os.path.join(plugins_dir_real, safe_dir_name) # not from requested_name. A name that came out of os.scandir() on a
if not os.path.isdir(safe_plugin_dir): # trusted root carries no taint regardless of what the caller asked
# for, so this is a real containment guarantee (an allowlist check
# against a trusted source), not a string-sanitisation of untrusted
# input that a static analyzer has to trust blindly.
matched_name = find_trusted_subdir(plugins_dir_real, requested_name)
if matched_name is None:
self.logger.error( self.logger.error(
"Plugin directory for %s not found inside plugins dir", plugin_id "Plugin directory for %s not found inside plugins dir", plugin_id
) )
return False return False
safe_plugin_dir = os.path.join(plugins_dir_real, matched_name)
requirements_file = os.path.join(safe_plugin_dir, "requirements.txt") requirements_file = os.path.join(safe_plugin_dir, "requirements.txt")
if not os.path.isfile(requirements_file): if not os.path.isfile(requirements_file):
+16 -16
View File
@@ -25,7 +25,9 @@ import logging
from urllib.parse import urlparse from urllib.parse import urlparse
from src.common.permission_utils import sudo_remove_directory, install_requirements_file from src.common.permission_utils import sudo_remove_directory, install_requirements_file
from src.plugin_system.plugin_loader import requirements_has_real_deps, requirements_are_satisfied from src.plugin_system.plugin_loader import (
requirements_has_real_deps, requirements_are_satisfied, find_trusted_subdir
)
try: try:
from jsonschema import Draft7Validator, ValidationError from jsonschema import Draft7Validator, ValidationError
@@ -1908,24 +1910,22 @@ class PluginStoreManager:
True if successful or no requirements file True if successful or no requirements file
""" """
# Reconstruct the plugin path from the trusted self.plugins_dir base + # Reconstruct the plugin path from the trusted self.plugins_dir base +
# a sanitised directory name rather than trusting plugin_path directly # an entry actually enumerated from it, rather than trusting
# -- callers ultimately derive it from a plugin-supplied manifest "id" # plugin_path directly -- callers ultimately derive it from a
# field (see install_plugin_from_url), so without this a malicious # plugin-supplied manifest "id" field (see install_plugin_from_url),
# manifest could point requirements_file outside plugins_dir. # so without this a malicious manifest could point requirements_file
# os.path.basename() is CodeQL's recognised py/path-injection # outside plugins_dir. find_trusted_subdir()'s return value always
# sanitiser: it strips all directory components so the result cannot # comes from os.scandir() on the trusted root, so building the path
# contain traversal sequences, matching the pattern already used in # from it (not from the caller's string) is a real containment
# PluginLoader.install_dependencies(). # guarantee, matching the pattern in PluginLoader.install_dependencies().
plugin_dir_real = os.path.realpath(str(plugin_path)) plugin_dir_real = os.path.realpath(str(plugin_path))
plugins_dir_real = os.path.realpath(str(self.plugins_dir)) plugins_dir_real = os.path.realpath(str(self.plugins_dir))
safe_dir_name = os.path.basename(plugin_dir_real) requested_name = os.path.basename(plugin_dir_real)
if not safe_dir_name: matched_name = find_trusted_subdir(plugins_dir_real, requested_name)
self.logger.error("Could not determine plugin directory name for dependency install") if matched_name is None:
return False self.logger.error("Plugin directory not found inside plugins dir for dependency install")
safe_plugin_path = Path(os.path.join(plugins_dir_real, safe_dir_name))
if not safe_plugin_path.is_dir():
self.logger.error("Plugin directory not found inside plugins dir: %s", safe_plugin_path)
return False return False
safe_plugin_path = Path(os.path.join(plugins_dir_real, matched_name))
requirements_file = safe_plugin_path / "requirements.txt" requirements_file = safe_plugin_path / "requirements.txt"