fix(controller): allowlist module names to close command injection (D2)#13
Merged
Merged
Conversation
DeployFragment.processNextInQueue() interpolated the module name into a sed/echo/runrole command executed as root inside the container. Module names come from the fixed ModuleRegistry catalog but round-trip through SharedPreferences, so a tampered or unknown value containing shell metacharacters (quote, ;, &&, $()) could inject commands run as root (tech-debt D2). - domain: new pure ModuleName.isAllowed(name, known) -- the name must be a known catalog key (allowlist) AND match [a-z0-9_-] with no shell metacharacters. Pure JVM, unit-tested (ModuleNameTest). - ModuleRegistry.validYamlKeys(): single allowlist source derived from MASTER_ROSTER (the catalog stays the source of truth). - DeployFragment guards the popped module before building the command and fails closed (logs + skips) on anything not allowed. The command structure is unchanged for legitimate modules, so behaviour for real installs is identical. Out of scope (documented follow-up): the lower-risk on-device `sh -c` pipes in the extract/backup paths, which interpolate app-internal absolute paths.
2d6aaa0 to
c460b5c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 (security hardening).
DeployFragment.processNextInQueue()interpolates a module name into a command executed as root inside the container:Module names come from the fixed
ModuleRegistrycatalog (the user only ticks checkboxes), so this is not exploitable today — but the queue round-trips through SharedPreferences and the value is concatenated raw, so a tampered/unknown name with shell metacharacters (',;,&&,$()) would inject commands run as root. Tech-debt item D2.Changes (allowlist, fail-closed — no behaviour change for real modules)
deploy/domain/ModuleName(new, pure JVM):isAllowed(name, known)= the name is a known catalog key (allowlist) and matches[a-z0-9_-](no uppercase, whitespace, quotes or shell metacharacters). Unit-tested (ModuleNameTest).ModuleRegistry.validYamlKeys(): the single allowlist source, derived fromMASTER_ROSTER(the catalog stays the source of truth).DeployFragment: validates the popped module before building the command and fails closed — an unrecognized/unsafe name is logged and skipped. The command string and the flow are unchanged for legitimate modules.Design notes / what we deliberately did NOT change
We chose an exact allowlist by roster (gold standard for command injection) over a looser charset-only check or a deeper refactor of the install flow /
PRootEngine(which would touch the god class and risk breaking the install). The fully-static commands (./runrole --reinstall maps, the bootstrapbash -c) are not interpolated and are left as-is. The lower-risk on-devicesh -cpipes in the extract/backup paths (which interpolate app-internal absolute paths, not container-root) are a documented follow-up.Testing
ModuleNameTest(pure JVM, runs in the CItestDebugUnitTestgate): accepts every roster module; rejects injection payloads, well-formed-but-unknown names, and bad charsets. Logic additionally verified out of band (27 cases).