Fix shell injection in command hook $ARGUMENTS substitution
_inject_arguments replaced $ARGUMENTS with json.dumps(payload) directly
into shell command strings. JSON encoding wraps values in double quotes,
but bash still expands $(...), backticks, and other metacharacters inside
double-quoted strings. A payload containing {"command": "$(whoami)"}
would result in the subshell being executed.
Add shlex.quote() around the serialized JSON when substituting into
command hooks (shell_escape=True), wrapping it in single quotes so bash
treats the entire payload as a literal string. Prompt hooks continue to
use raw substitution since they pass the result to the LLM API, not a
shell.
The existing OPENHARNESS_HOOK_PAYLOAD environment variable (already set
on line 83) remains the recommended way to access payload data from hook
scripts, since env vars are not subject to shell expansion.
This commit is contained in:
@@ -19,6 +19,7 @@ The format is based on Keep a Changelog, and this project currently tracks chang
|
||||
|
||||
### Fixed
|
||||
|
||||
- Shell-escape `$ARGUMENTS` substitution in command hooks to prevent shell injection from payload values containing metacharacters like `$(...)` or backticks.
|
||||
- Memory scanner now parses YAML frontmatter (`name`, `description`, `type`) instead of returning raw `---` as description.
|
||||
- Memory search matches against body content in addition to metadata, with metadata weighted higher for relevance.
|
||||
- Memory search tokenizer handles Han characters for multilingual queries.
|
||||
|
||||
@@ -6,6 +6,7 @@ import asyncio
|
||||
import fnmatch
|
||||
import json
|
||||
import os
|
||||
import shlex
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
@@ -70,7 +71,7 @@ class HookExecutor:
|
||||
event: HookEvent,
|
||||
payload: dict[str, Any],
|
||||
) -> HookResult:
|
||||
command = _inject_arguments(hook.command, payload)
|
||||
command = _inject_arguments(hook.command, payload, shell_escape=True)
|
||||
try:
|
||||
process = await create_shell_subprocess(
|
||||
command,
|
||||
@@ -207,8 +208,13 @@ def _matches_hook(hook: HookDefinition, payload: dict[str, Any]) -> bool:
|
||||
return fnmatch.fnmatch(subject, matcher)
|
||||
|
||||
|
||||
def _inject_arguments(template: str, payload: dict[str, Any]) -> str:
|
||||
return template.replace("$ARGUMENTS", json.dumps(payload, ensure_ascii=True))
|
||||
def _inject_arguments(
|
||||
template: str, payload: dict[str, Any], *, shell_escape: bool = False
|
||||
) -> str:
|
||||
serialized = json.dumps(payload, ensure_ascii=True)
|
||||
if shell_escape:
|
||||
serialized = shlex.quote(serialized)
|
||||
return template.replace("$ARGUMENTS", serialized)
|
||||
|
||||
|
||||
def _parse_hook_json(text: str) -> dict[str, Any]:
|
||||
|
||||
@@ -10,6 +10,7 @@ from openharness.api.client import ApiMessageCompleteEvent
|
||||
from openharness.api.usage import UsageSnapshot
|
||||
from openharness.engine.messages import ConversationMessage, TextBlock
|
||||
from openharness.hooks import HookEvent, HookExecutionContext, HookExecutor
|
||||
from openharness.hooks.executor import _inject_arguments
|
||||
from openharness.hooks.loader import HookRegistry
|
||||
from openharness.hooks.schemas import CommandHookDefinition, PromptHookDefinition
|
||||
|
||||
@@ -70,3 +71,51 @@ async def test_prompt_hook_can_block(tmp_path: Path):
|
||||
|
||||
assert result.blocked is True
|
||||
assert result.reason == "blocked by policy"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _inject_arguments shell escaping
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_inject_arguments_no_escape_by_default():
|
||||
payload = {"command": "$(whoami)"}
|
||||
result = _inject_arguments("echo $ARGUMENTS", payload)
|
||||
# Without shell_escape, the raw JSON is substituted
|
||||
assert result == 'echo {"command": "$(whoami)"}'
|
||||
|
||||
|
||||
def test_inject_arguments_shell_escape_wraps_in_single_quotes():
|
||||
payload = {"command": "$(whoami)"}
|
||||
result = _inject_arguments("echo $ARGUMENTS", payload, shell_escape=True)
|
||||
# With shell_escape, shlex.quote wraps the JSON in single quotes
|
||||
# so bash treats it as a literal string
|
||||
assert result.startswith("echo '")
|
||||
assert "$(whoami)" in result
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_command_hook_escapes_shell_metacharacters(tmp_path: Path):
|
||||
"""$ARGUMENTS in command hooks must be shell-escaped to prevent injection."""
|
||||
registry = HookRegistry()
|
||||
registry.register(
|
||||
HookEvent.PRE_TOOL_USE,
|
||||
CommandHookDefinition(command="echo $ARGUMENTS"),
|
||||
)
|
||||
executor = HookExecutor(
|
||||
registry,
|
||||
HookExecutionContext(
|
||||
cwd=tmp_path,
|
||||
api_client=FakeApiClient('{"ok": true}'),
|
||||
default_model="claude-test",
|
||||
),
|
||||
)
|
||||
|
||||
# $(echo INJECTED) would execute as a subshell if not properly escaped
|
||||
payload = {"tool_name": "test", "input": "$(echo INJECTED)"}
|
||||
result = await executor.execute(HookEvent.PRE_TOOL_USE, payload)
|
||||
|
||||
output = result.results[0].output
|
||||
# With proper escaping, the literal $(echo INJECTED) must survive.
|
||||
# Without escaping, bash expands the subshell and the $() wrapper is gone.
|
||||
assert "$(echo INJECTED)" in output
|
||||
|
||||
Reference in New Issue
Block a user