Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Analyze cmdsubs in arith-cmd redirects, for-arith expressions, and pa…
…ram expansions

Additional security gaps found and fixed:
- arith-cmd: check redirects on (( expr )) constructs
- for-arith: extract and analyze $(cmd) in init/cond/incr strings
- param expansion: analyze cmdsubs in ${x:-$(cmd)} style constructs

Adds _analyze_string_cmdsubs() helper to parse $(cmd) from raw strings
where Parable doesn't provide full AST nodes.
  • Loading branch information
ldayton committed Jan 17, 2026
commit 8c8c15b84bcd22cd30da298bb14420dcd3efeb27
84 changes: 73 additions & 11 deletions src/dippy/core/analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ def _analyze_node(node, config: Config, cwd: Path) -> Decision:

elif kind == "for-arith":
decisions = [_analyze_node(node.body, config, cwd)]
# Check init/cond/incr expressions for cmdsubs (stored as raw strings)
for expr in (node.init, node.cond, node.incr):
if expr:
decisions.extend(_analyze_string_cmdsubs(expr, config, cwd))
decisions.extend(_analyze_redirects(node, config, cwd))
return _combine(decisions)

Expand Down Expand Up @@ -168,15 +172,17 @@ def _analyze_node(node, config: Config, cwd: Path) -> Decision:
for cmdsub in _find_cmdsubs_in_arith(node.expression):
inner_decision = _analyze_node(cmdsub.command, config, cwd)
if inner_decision.action != "allow":
return Decision(
inner_decision.action,
f"arithmetic cmdsub: {inner_decision.reason}",
children=[inner_decision],
decisions.append(
Decision(
inner_decision.action,
f"arithmetic cmdsub: {inner_decision.reason}",
children=[inner_decision],
)
)
decisions.append(inner_decision)
if decisions:
return _combine(decisions)
return Decision("allow", "arithmetic")
else:
decisions.append(inner_decision)
decisions.extend(_analyze_redirects(node, config, cwd))
return _combine(decisions) if decisions else Decision("allow", "arithmetic")

elif kind == "comment":
return Decision("allow", "comment")
Expand Down Expand Up @@ -251,6 +257,15 @@ def _analyze_command(node, config: Config, cwd: Path) -> Decision:
):
inner_cmd = _get_word_value(word).strip("$()")
return Decision("ask", f"cmdsub injection risk: {inner_cmd}")
elif part_kind == "param":
# Parameter expansion - check for cmdsubs in arg (raw string)
arg = getattr(part, "arg", None)
if arg and isinstance(arg, str):
param_decisions = _analyze_string_cmdsubs(arg, config, cwd)
for pd in param_decisions:
if pd.action != "allow":
return pd
decisions.extend(param_decisions)

# 2. Check redirects
redirect_decisions = _analyze_redirects(node, config, cwd)
Expand Down Expand Up @@ -494,7 +509,7 @@ def _analyze_cond_node(node, config: Config, cwd: Path) -> list[Decision]:


def _analyze_word_parts(word, config: Config, cwd: Path) -> list[Decision]:
"""Analyze word parts for command/process substitutions."""
"""Analyze word parts for command/process substitutions, including nested ones."""
decisions = []
parts = getattr(word, "parts", [])
for part in parts:
Expand All @@ -505,7 +520,7 @@ def _analyze_word_parts(word, config: Config, cwd: Path) -> list[Decision]:
decisions.append(
Decision(
inner_decision.action,
f"conditional cmdsub: {inner_decision.reason}",
f"cmdsub: {inner_decision.reason}",
children=[inner_decision],
)
)
Expand All @@ -518,12 +533,59 @@ def _analyze_word_parts(word, config: Config, cwd: Path) -> list[Decision]:
decisions.append(
Decision(
inner_decision.action,
f"conditional procsub {direction}(...): {inner_decision.reason}",
f"procsub {direction}(...): {inner_decision.reason}",
children=[inner_decision],
)
)
else:
decisions.append(inner_decision)
elif part_kind == "param":
# Parameter expansion - check for cmdsubs in arg value (raw string)
# ${x:-$(cmd)}, ${x:=$(cmd)}, ${x:+$(cmd)}, ${x:?$(cmd)}
arg = getattr(part, "arg", None)
if arg and isinstance(arg, str):
decisions.extend(_analyze_string_cmdsubs(arg, config, cwd))
return decisions


def _analyze_string_cmdsubs(s: str, config: Config, cwd: Path) -> list[Decision]:
"""Extract and analyze command substitutions from a raw string."""
decisions = []
i = 0
while i < len(s):
# Look for $( pattern
if s[i : i + 2] == "$(":
# Find matching closing paren, accounting for nesting
depth = 1
start = i + 2
j = start
while j < len(s) and depth > 0:
if s[j : j + 2] == "$(":
depth += 1
j += 2
elif s[j] == ")":
depth -= 1
j += 1
else:
j += 1
if depth == 0:
inner_cmd = s[start : j - 1]
inner_decision = analyze(inner_cmd, config, cwd)
if inner_decision.action != "allow":
decisions.append(
Decision(
inner_decision.action,
f"cmdsub: {inner_decision.reason}",
children=[inner_decision],
)
)
else:
decisions.append(inner_decision)
i = j
else:
i += 1
else:
i += 1
return decisions


Expand Down
74 changes: 74 additions & 0 deletions tests/test_analyzer_bugs.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,3 +245,77 @@ def test_compound_redirect_cmdsub(self, cmd, expected, config, cwd):
def test_redirect_target_cmdsub(self, cmd, expected, config, cwd):
"""Cmdsubs in redirect targets should be analyzed."""
assert analyze(cmd, config, cwd).action == expected


class TestArithCmdRedirect:
"""Tests for arith-cmd redirect checking."""

@pytest.fixture
def config(self):
return Config()

@pytest.fixture
def cwd(self):
return Path.cwd()

@pytest.mark.parametrize(
"cmd,expected",
[
("(( 1 )) > $(rm foo)", "ask"),
("(( x++ )) > /tmp/out", "ask"),
],
)
def test_arith_cmd_redirect(self, cmd, expected, config, cwd):
"""Arith-cmd should check its redirects."""
assert analyze(cmd, config, cwd).action == expected


class TestForArithCmdsub:
"""Tests for cmdsubs in for-arith init/cond/incr expressions."""

@pytest.fixture
def config(self):
return Config()

@pytest.fixture
def cwd(self):
return Path.cwd()

@pytest.mark.parametrize(
"cmd,expected",
[
("for (( i=$(rm foo); i<10; i++ )); do echo $i; done", "ask"),
("for (( i=0; i<$(rm foo); i++ )); do echo $i; done", "ask"),
("for (( i=0; i<10; i+=$(rm foo) )); do echo $i; done", "ask"),
],
)
def test_for_arith_cmdsub(self, cmd, expected, config, cwd):
"""Cmdsubs in for-arith init/cond/incr should be analyzed."""
assert analyze(cmd, config, cwd).action == expected


class TestParamExpansionCmdsub:
"""Tests for cmdsubs nested inside parameter expansions."""

@pytest.fixture
def config(self):
return Config()

@pytest.fixture
def cwd(self):
return Path.cwd()

@pytest.mark.parametrize(
"cmd,expected",
[
("echo ${x:-$(rm foo)}", "ask"),
("echo ${x:=$(rm foo)}", "ask"),
("echo ${x:+$(rm foo)}", "ask"),
("echo ${x:?$(rm foo)}", "ask"),
("[[ -f ${x:-$(rm foo)} ]]", "ask"),
("for i in ${x:-$(rm foo)}; do echo $i; done", "ask"),
],
)
def test_param_expansion_cmdsub(self, cmd, expected, config, cwd):
"""Cmdsubs nested in parameter expansions should be analyzed."""
assert analyze(cmd, config, cwd).action == expected