fix: prevent shell injection in AndroidExtEnv.execute_adb_with_cmd (CWE-78)

[sebastiondev]

Summary

Fixes a shell command injection vulnerability (CWE-78) in AndroidExtEnv.execute_adb_with_cmd. This method is the single chokepoint through which all 20+ adb invocations in the Android environment flow, and it currently runs an attacker-influenceable command string through the host shell.

Vulnerability

• File: metagpt/environment/android/android_ext_env.py
• Function: AndroidExtEnv.execute_adb_with_cmd
• CWE: CWE-78 — OS Command Injection
• Severity: High (RCE on the operator host running the Android environment)

The vulnerable line:

res = subprocess.run(adb_cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)

adb_cmd is built via f-strings from agent-/LLM-supplied data throughout the file. For example, the user_input path:

EnvAction.USER_INPUT.input_txt
  -> AndroidExtEnv._execute_env_action
  -> AndroidExtEnv.user_input(input_txt=...)
  -> f"adb -s {self.device_id} shell input text {input_txt}"
  -> execute_adb_with_cmd(...)
  -> subprocess.run(..., shell=True)

The only sanitization on input_txt strips spaces and quotes — shell metacharacters such as ;, &, |, `, $(), and newlines pass through untouched. An adversary that can influence the agent's task text (a realistic threat in multi-agent / tool-use deployments and any flow where task descriptions, page content, or tool outputs are fed back into the agent) can therefore execute arbitrary commands on the host.

Fix

Tokenize the command with shlex.split and disable the shell:

import shlex
...
res = subprocess.run(
    shlex.split(adb_cmd), shell=False, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True
)

Rationale:

• Patching at the single sink removes the vulnerability for every caller without having to audit and sanitize each of the ~20 f-string command builders individually.
• shlex.split preserves the existing quoting semantics callers already rely on (e.g. adb shell "am start ..."), so behavior for legitimate adb commands is unchanged.
• shell=False ensures shell metacharacters in any embedded user/agent text remain literal arguments to adb rather than being interpreted by /bin/sh.

The diff is intentionally minimal — one import and one call site:

metagpt/environment/android/android_ext_env.py |  5 +-
tests/security/test_cwe78_android_ext_env.py   | 85 ++++++++++++++++++++++++++
2 files changed, 89 insertions(+), 1 deletion(-)

Tests

Added tests/security/test_cwe78_android_ext_env.py with three regression checks (all passing locally):

1. test_execute_adb_with_cmd_does_not_invoke_a_shell — AST-asserts subprocess.run is invoked with shell=False.
2. test_execute_adb_with_cmd_passes_tokenized_argv — AST-asserts the first positional argument is shlex.split(adb_cmd) (not a raw string).
3. test_agent_user_input_reaches_adb_execution_sink — Documents and pins the data-flow path EnvAction.USER_INPUT → user_input → execute_adb_with_cmd, so future refactors can't silently re-introduce a shell-string sink without tripping the test.

3 passed in 3.56s

We also constructed a runtime PoC against the pre-fix code using a payload like hello; touch /tmp/pwned routed through user_input; the file was created on the unpatched build and is no longer created after the patch.

Security analysis

• Exploitability: The reachable path requires only that an attacker influence agent-consumed text (task description, retrieved content, or any field that lands in EnvAction.USER_INPUT.input_txt or another adb command builder) on a deployment that actually runs the Android environment with adb available. The existing input scrubbing only removes whitespace and quotes, which does not block command separators or substitutions.
• Mitigations after fix: Shell metacharacters are no longer interpreted; the worst case for a malicious input_txt is now a malformed adb shell input text argument list that adb rejects.
• Scope: The fix is at the single sink, so all other adb command builders (install, pull, push, getevent, screencap, etc.) inherit the protection without code changes elsewhere.

Adversarial review

Before submitting we tried to disprove this. We checked whether the upstream user_input sanitizer or any wrapper neutralized shell metacharacters — it only strips spaces and quotes, leaving ;, |, &, `, $(), and newlines intact. We considered whether the Android environment is gated behind operator-only configuration that would make agent input non-attacker-controlled — it isn't; in normal multi-agent/tool-use flows the LLM-produced text reaches this sink. We considered whether shell=True with a fixed adb prefix limits damage — it doesn't, because the shell evaluates the whole string and command separators escape the adb prefix entirely. The preconditions to exploit (LLM-influenced text + Android env enabled on a host with adb) do not already grant the attacker shell access on the operator's machine, so this fix provides genuine defense rather than restating an existing capability.

cc @lewiswigmore

[lewiswigmore]

Closing this as inactive — no maintainer response after 14 days.

The security finding and fix remain valid. If this is still relevant, I'm happy to reopen, rebase, or re-submit against a different branch. Just drop a comment.

[lewiswigmore]

Closing this as inactive — no maintainer response after 14 days.

The security finding and fix remain valid. If this is still relevant, I'm happy to reopen, rebase, or re-submit against a different branch. Just drop a comment.