ADR-013 Phase 2: verify every plugins.* import via a meta_path guard
A re-review showed the previous "single import choke point" claim was wrong: `plugins` is a normal importable package, so core request handlers that do `from plugins.<name>.models import ...` never passed through the loader and ran unverified - an attacker who dropped a file into plugins/<name>/ got arbitrary in-process code execution on an ordinary HTTP request (and a planted .pyc ran from cache). Gating load_plugin_class covered only plugin.py, one path of many. Fix: importguard.py installs a sys.meta_path finder (under enforcement) that intercepts EVERY plugins.<name>.* import, verifies the plugin's signed provenance once, then verifies each module file against it and execs the exact bytes it hashed - read once, compiled, exec'd, never a .pyc, never a re-opened file. This closes the submodule bypass and the planted-bytecode read, and the read-once exec closes the verify-vs-exec TOCTOU on the import path. The import system, not one method, is the real choke point. - init_app installs the guard when PLUGIN_REQUIRE_SIGNED, clears it otherwise. - load_plugin_class now verifies plugin.py from a single read and execs that buffer (finding #3 on that file); its submodule imports flow through the guard. - docs: stamp-bundled must cover every plugin dir present (a disabled plugin's module can be imported by core); recommend a read-only plugins/ owned by the deploy user as defense in depth (closes the residual migrate-time race an attacker with concurrent write could otherwise attempt). Earlier review's fixes #3 (migrate code paths) and #4 (shelf content binding) were confirmed sound and are unchanged. 7 import-guard tests (submodule verify, tamper, unsigned refused, planted .pyc ignored, real import through the guard, install/uninstall). 1061 pass, naming green.
This commit is contained in:
157
tests/test_plugin_importguard.py
Normal file
157
tests/test_plugin_importguard.py
Normal file
@@ -0,0 +1,157 @@
|
||||
"""Import-guard tests (ADR-013 Phase 2 re-review fix).
|
||||
|
||||
The critical re-review finding: `plugins` is a normal package, so core code's
|
||||
`from plugins.<name>.models import ...` bypassed the loader entirely and ran
|
||||
unverified. These cover the sys.meta_path guard that verifies EVERY plugins.*
|
||||
import from the exact bytes it execs.
|
||||
"""
|
||||
|
||||
import importlib
|
||||
import json
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
||||
from shopdb.plugins import signing
|
||||
from shopdb.plugins.importguard import (PluginImportGuard,
|
||||
PluginVerificationError,
|
||||
install, uninstall, get_installed)
|
||||
from shopdb.plugins.verification import PluginVerifier
|
||||
|
||||
|
||||
def _write_pkg_plugin(plugins_dir, name, marker='source'):
|
||||
"""A plugin with a models submodule that records which bytes executed."""
|
||||
pdir = plugins_dir / name
|
||||
(pdir / 'models').mkdir(parents=True)
|
||||
(pdir / 'manifest.json').write_text(json.dumps({
|
||||
'name': name, 'version': '1.0.0', 'description': 'd',
|
||||
'core_version': '>=0.1.0,<1.0.0', 'api_prefix': f'/api/{name}',
|
||||
}))
|
||||
(pdir / '__init__.py').write_text('')
|
||||
(pdir / 'plugin.py').write_text('# plugin\n')
|
||||
(pdir / 'models' / '__init__.py').write_text(f'MARKER = {marker!r}\n')
|
||||
return pdir
|
||||
|
||||
|
||||
def _stamp(pdir, private_key, name):
|
||||
provenance = signing.build_provenance(pdir, name, '1.0.0', 'pub')
|
||||
provenance_bytes = signing.serialize_provenance(provenance)
|
||||
(pdir / signing.PROVENANCE_NAME).write_bytes(provenance_bytes)
|
||||
(pdir / signing.PROVENANCE_SIG).write_bytes(
|
||||
signing.sign(private_key, provenance_bytes))
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def keypair(tmp_path):
|
||||
private_pem, public_pem = signing.generate_keypair()
|
||||
pub_path = tmp_path / 'k.pub'
|
||||
pub_path.write_bytes(public_pem)
|
||||
return signing.load_private_key(private_pem), str(pub_path)
|
||||
|
||||
|
||||
def _verifier(plugins_dir, pub_path):
|
||||
return PluginVerifier(plugins_dir, require_signed=True,
|
||||
trusted_key_paths=[pub_path])
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def guarded(tmp_path, keypair):
|
||||
"""Install a guard, yield (guard, plugins_dir, private_key, name), clean up
|
||||
sys.modules + sys.meta_path afterward so tests do not leak."""
|
||||
private_key, pub_path = keypair
|
||||
plugins_dir = tmp_path / 'plugins'
|
||||
plugins_dir.mkdir()
|
||||
name = 'guardtestpkg'
|
||||
guard = install(plugins_dir, _verifier(plugins_dir, pub_path))
|
||||
try:
|
||||
yield guard, plugins_dir, private_key, name
|
||||
finally:
|
||||
uninstall()
|
||||
for mod in list(sys.modules):
|
||||
if mod == f'plugins.{name}' or mod.startswith(f'plugins.{name}.'):
|
||||
del sys.modules[mod]
|
||||
|
||||
|
||||
# --- direct find_spec / verified_source -------------------------------------
|
||||
|
||||
def test_verified_source_signed_ok_tampered_raises(guarded):
|
||||
guard, plugins_dir, priv, name = guarded
|
||||
pdir = _write_pkg_plugin(plugins_dir, name)
|
||||
_stamp(pdir, priv, name)
|
||||
|
||||
assert guard.verified_source(name, 'plugin.py') == b'# plugin\n'
|
||||
|
||||
(pdir / 'plugin.py').write_text('# tampered\n')
|
||||
with pytest.raises(PluginVerificationError):
|
||||
guard.verified_source(name, 'plugin.py')
|
||||
|
||||
|
||||
def test_find_spec_refuses_unsigned_plugin(guarded):
|
||||
guard, plugins_dir, priv, name = guarded
|
||||
_write_pkg_plugin(plugins_dir, name) # no provenance stamped
|
||||
with pytest.raises(PluginVerificationError):
|
||||
guard.find_spec(f'plugins.{name}.models')
|
||||
|
||||
|
||||
def test_find_spec_refuses_tampered_submodule(guarded):
|
||||
guard, plugins_dir, priv, name = guarded
|
||||
pdir = _write_pkg_plugin(plugins_dir, name)
|
||||
_stamp(pdir, priv, name)
|
||||
# tamper the submodule after signing
|
||||
(pdir / 'models' / '__init__.py').write_text('MARKER = "evil"\n')
|
||||
with pytest.raises(PluginVerificationError):
|
||||
guard.find_spec(f'plugins.{name}.models')
|
||||
|
||||
|
||||
# --- real import through the installed guard --------------------------------
|
||||
|
||||
def test_core_style_submodule_import_is_verified(guarded):
|
||||
"""`from plugins.<name>.models import MARKER` runs the SIGNED source."""
|
||||
guard, plugins_dir, priv, name = guarded
|
||||
pdir = _write_pkg_plugin(plugins_dir, name, marker='source')
|
||||
_stamp(pdir, priv, name)
|
||||
|
||||
module = importlib.import_module(f'plugins.{name}.models')
|
||||
assert module.MARKER == 'source'
|
||||
|
||||
|
||||
def test_planted_pyc_is_never_consulted(guarded):
|
||||
"""A planted __pycache__/*.pyc must not run: the guard loads verified source
|
||||
only, so a garbage bytecode cache is simply ignored (finding #1/#2)."""
|
||||
guard, plugins_dir, priv, name = guarded
|
||||
pdir = _write_pkg_plugin(plugins_dir, name, marker='source')
|
||||
_stamp(pdir, priv, name)
|
||||
|
||||
cache = pdir / 'models' / '__pycache__'
|
||||
cache.mkdir()
|
||||
# garbage bytecode: if the loader ever honored it, import would crash
|
||||
for pyc in (f'__init__.cpython-{sys.version_info.major}'
|
||||
f'{sys.version_info.minor}.pyc',):
|
||||
(cache / pyc).write_bytes(b'\x00\x00\x00\x00garbage-not-valid-bytecode')
|
||||
|
||||
module = importlib.import_module(f'plugins.{name}.models')
|
||||
assert module.MARKER == 'source' # source ran, pyc ignored
|
||||
|
||||
|
||||
def test_tampered_submodule_import_raises(guarded):
|
||||
guard, plugins_dir, priv, name = guarded
|
||||
pdir = _write_pkg_plugin(plugins_dir, name)
|
||||
_stamp(pdir, priv, name)
|
||||
(pdir / 'models' / '__init__.py').write_text('MARKER = "evil"\n')
|
||||
with pytest.raises(ImportError):
|
||||
importlib.import_module(f'plugins.{name}.models')
|
||||
|
||||
|
||||
def test_install_uninstall_idempotent(tmp_path, keypair):
|
||||
_, pub_path = keypair
|
||||
plugins_dir = tmp_path / 'plugins'
|
||||
plugins_dir.mkdir()
|
||||
verifier = _verifier(plugins_dir, pub_path)
|
||||
try:
|
||||
install(plugins_dir, verifier)
|
||||
install(plugins_dir, verifier) # replaces, does not stack
|
||||
assert sum(isinstance(f, PluginImportGuard) for f in sys.meta_path) == 1
|
||||
assert get_installed() is not None
|
||||
finally:
|
||||
uninstall()
|
||||
assert get_installed() is None
|
||||
Reference in New Issue
Block a user