Skip to content

fix(cron): refuse bash brace expansion at storage time #7608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aniruddhaadak80 wants to merge 1 commit into from
Closed

fix(cron): refuse bash brace expansion at storage time #7608

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 27 additions & 13 deletions src/kiro_crew/mcp_cron.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,9 @@
# backtick pairs. We deliberately reject a lone backtick too — a stray one means
# unmatched-quoting confusion, not a benign literal.
_CRON_CMD_SUBST_RE = re.compile(
r"\$\(|" # $( ... ) and $(( ... )) (`\(` covers both since $((… starts with $()
r"\$'|" # $'...' ANSI-C quoting: `$'\x2e\x73\x73\x68'` decodes to ".ssh"
r"`", # backtick — matches EITHER end of a pair, and unmatched too
r"\$\(|" # $( ... ) and $(( ... )) (`\(` covers both since $((… starts with $()
r"\$'|" # $'...' ANSI-C quoting: `$'\x2e\x73\x73\x68'` decodes to ".ssh"
r"`", # backtick — matches EITHER end of a pair, and unmatched too
)
# A ``${...}`` that is NOT a plain ``${NAME}`` reference. Every other brace form
# COMPOSES a string at expansion time, which is the same hazard as command
Expand All @@ -150,6 +150,12 @@
# rather than enumerating the operators means a form nobody listed is refused by
# default instead of admitted.
_CRON_BRACE_EXPANSION_RE = re.compile(r"\$\{(?![A-Za-z_][A-Za-z0-9_]*\})")
# Bash brace expansion in a cron `command` (`{a,b}` / `{1..3}`): the shell
# expands it before the vet gate's static string checks run, so a credential
# path can be assembled from fragments that never appear literally. Whitespace-
# free braces only — `find -exec {} ;` and `awk '{print $1}'` contain no comma
# or `..` and stay allowed (verified: bash does not expand `{a b,c}`).
_CRON_BASH_BRACE_RE = re.compile(r"\{[^}\s]*,[^}\s]*\}|\{[0-9]+\.\.[0-9]+\}")
# Any `$NAME` / `${NAME}` variable reference. Used AFTER local assignment
# resolution to catch the last composition class: an UNRESOLVED reference. sh
# expands an unset variable to the empty string, so `cat ~/.ss${UNSET}h/id_rsa`
Expand Down Expand Up @@ -178,9 +184,7 @@
# start-of-command / after a separator / after `do`/`then` and word-bounded, so
# `git log --format=for` (keyword as an argument) and a quoted `'while ...'` are
# NOT matched. `case` is included because its patterns compose the same way.
_CRON_SHELL_KEYWORD_RE = re.compile(
r"(?:^|[;&|]|\bdo\b|\bthen\b)\s*\b(?:for|while|until|case)\b"
)
_CRON_SHELL_KEYWORD_RE = re.compile(r"(?:^|[;&|]|\bdo\b|\bthen\b)\s*\b(?:for|while|until|case)\b")
# Pathname expansion (globbing) is a FOURTH way to compose a sensitive path that
# never appears literally: ``cat ~/.s?h/id_rsa`` reads ``~/.ssh/id_rsa`` (verified
# against real sh, all three metacharacters). Blanket-refusing ``*``/``?``/``[``
Expand Down Expand Up @@ -211,10 +215,10 @@
# ``a=b`` as an assignment is harmless here: this map is only ever used to make
# the credential-path scan see MORE, never to permit something.
_CRON_LOCAL_ASSIGN_RE = re.compile(
r"(?:^|[;&|\s])\s*" # start-of-command, a separator, or whitespace
r"(?:^|[;&|\s])\s*" # start-of-command, a separator, or whitespace
r"([A-Za-z_][A-Za-z0-9_]*)" # variable name
r"=" # literal =
r"([^\s;&|]*)", # value up to next separator
r"=" # literal =
r"([^\s;&|]*)", # value up to next separator
)
# A backslash escaping any character. sh drops the backslash and keeps the
# character during word expansion, so the scan must do the same to see the string
Expand Down Expand Up @@ -329,7 +333,7 @@ def _glob_could_reach_credentials(command: str) -> bool:
# stripped above). Both fnmatch directions so a glob in EITHER the
# command or the sensitive name is caught.
for start in range(len(cand_segments) - depth + 1):
win_segs = cand_segments[start:start + depth]
win_segs = cand_segments[start : start + depth]
# sh does NOT let a leading `*`/`?`/`[` match a leading dot — a
# hidden file is excluded from globbing unless the pattern spells
# the dot literally. Every sensitive name here is a dotfile
Expand Down Expand Up @@ -618,6 +622,18 @@ def _vet_shell_command(command: str) -> str | None:
"strings a static check cannot see. If your job needs runtime "
"composition, ship it as a `script` job — the body is scanned in full."
)
# Bash brace expansion (`{a,b}` / `{1..3}`) assembles paths at
# expansion time that never appear literally. Whitespace-free braces
# only — `find -exec {} ;` and `awk '{print $1}'` stay allowed because
# they contain no comma or `..` (verified: bash does not expand
# `{a b,c}`).
if _CRON_BASH_BRACE_RE.search(command):
return (
"Error: cron command blocked: brace expansion "
"(`{a,b}`, `{1..3}`) is not permitted in a cron `command`. "
"If your job needs runtime composition, ship it as a `script` "
"job — the script body is scanned in full."
)
if _CRON_POSITIONAL_PARAM_RE.search(command):
return (
"Error: cron command blocked: positional and special parameters "
Expand Down Expand Up @@ -729,9 +745,7 @@ def _unquote(s: str) -> str:
# fragment). `resolved` already has the tracked `A=.s; ... $A` cases expanded,
# so this does not fire on those.
leftover = {
name
for name in _CRON_VAR_REF_RE.findall(resolved)
if name not in _CRON_VAR_REF_ALLOWED
name for name in _CRON_VAR_REF_RE.findall(resolved) if name not in _CRON_VAR_REF_ALLOWED
}
if leftover:
return (
Expand Down
Loading