From 02d107eb1c1a239d5b2bdc381d08640f4d9df37b Mon Sep 17 00:00:00 2001 From: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Date: Mon, 5 Oct 2026 21:12:36 +0300 Subject: [PATCH 1/3] fix(rp1): identify the Docker image operand after options RP1 took the first word after `docker run|create|pull` as the image and searched it for a tag. Options were reported as the image (`docker run --rm alpine:3.20` gave an unpinned image named `--rm`), and a registry port or an option value was read as a tag (`docker pull localhost:5000/team/tool`, `docker run --publish=8080:80 evil/image` gave no finding). Read the command as shell words and skip options using docker/cli's option table for each subcommand, so the first operand is the image. Check for a tag only in the image's last path component. When the image cannot be identified (unknown option, missing value, end of command, unterminated quote, or the 1024-character read bound), the command is still reported. test_privileged_payload_in_referenced_reference_file_stays_install_unsafe reached DO_NOT_INSTALL (56) only through the RP1 false positive on `docker run --privileged --pid=host vendor/collector:1.4`; with the pinned image no longer reported, PE5 and TM4 alone score 48. The fixture now uses an unpinned image so RP1 fires for a real reason. Signed-off-by: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- .../nodes/analyzers/mcp_rug_pull.py | 118 +++++++++++++++++- tests/nodes/test_security_end_to_end.py | 2 +- tests/test_mcp_rug_pull.py | 99 +++++++++++++++ 3 files changed, 214 insertions(+), 5 deletions(-) diff --git a/src/skillspector/nodes/analyzers/mcp_rug_pull.py b/src/skillspector/nodes/analyzers/mcp_rug_pull.py index 31b8f2486..4193cfd0c 100644 --- a/src/skillspector/nodes/analyzers/mcp_rug_pull.py +++ b/src/skillspector/nodes/analyzers/mcp_rug_pull.py @@ -136,12 +136,63 @@ def analyzer_exhausted(self) -> bool: re.IGNORECASE, ) _RP1_DOCKER_CMD = re.compile( - r"docker\s+(?:pull|run|create)\s+\S+", + r"docker\s+(pull|run|create)\s+(\S+)", re.IGNORECASE, ) _VERSION_PIN_RE = re.compile(r"@[\d.]+\b|==[\d.]+|:[\d.]+|@sha256:") +# Options that take a separate value and boolean options, from docker/cli +# (cli/command/container/opts.go, run.go and create.go; cli/command/image/pull.go), +# including hidden and deprecated ones. `docker create` accepts the `docker run` +# options except -d, --detach-keys and --sig-proxy; sharing one table only changes +# how commands that docker itself rejects are read. +_DOCKER_RUN_VALUE_OPTIONS = frozenset( + """ + -a -c -e -h -l -m -p -u -v -w + --add-host --annotation --attach --blkio-weight --blkio-weight-device --cap-add + --cap-drop --cgroup-parent --cgroupns --cidfile --cpu-count --cpu-percent --cpu-period + --cpu-quota --cpu-rt-period --cpu-rt-runtime --cpu-shares --cpus --cpuset-cpus + --cpuset-mems --detach-keys --device --device-cgroup-rule --device-read-bps + --device-read-iops --device-write-bps --device-write-iops --dns --dns-opt --dns-option + --dns-search --domainname --entrypoint --env --env-file --expose --gpus --group-add + --health-cmd --health-interval --health-retries --health-start-interval + --health-start-period --health-timeout --hostname --io-maxbandwidth --io-maxiops --ip + --ip6 --ipc --isolation --kernel-memory --label --label-file --link --link-local-ip + --log-driver --log-opt --mac-address --memory --memory-reservation --memory-swap + --memory-swappiness --mount --name --net --net-alias --network --network-alias + --oom-score-adj --pid --pids-limit --platform --publish --pull --restart --runtime + --security-opt --shm-size --stop-signal --stop-timeout --storage-opt --sysctl --tmpfs + --ulimit --umask --user --userns --uts --volume --volume-driver --volumes-from --workdir + """.split() +) +_DOCKER_RUN_FLAG_OPTIONS = frozenset( + """ + -P -d -i -q -t + --detach --disable-content-trust --help --init --interactive --no-healthcheck + --oom-kill-disable --privileged --publish-all --quiet --read-only --rm --sig-proxy --tty + --use-api-socket + """.split() +) +_DOCKER_OPTIONS: dict[str, tuple[frozenset[str], frozenset[str]]] = { + "run": (_DOCKER_RUN_VALUE_OPTIONS, _DOCKER_RUN_FLAG_OPTIONS), + "create": (_DOCKER_RUN_VALUE_OPTIONS, _DOCKER_RUN_FLAG_OPTIONS), + "pull": ( + frozenset({"--platform"}), + frozenset({"-a", "-q", "--all-tags", "--disable-content-trust", "--help", "--quiet"}), + ), +} +# One shell word: unquoted text, $(...), backslash escapes (including a line +# continuation) and complete quoted strings. The alternatives start with distinct +# characters, so matching is linear; an unterminated quote ends the word. +_SHELL_WORD_RE = re.compile( + r"""(?:[^\s"'\\|&;()`$]+|\$(?:\([^()\n]*\))?|\\(?:\r?\n|.)|"(?:[^"\\\n]|\\.)*"|'[^'\n]*')+""" +) +_SHELL_WORD_GAP_RE = re.compile(r"(?:[ \t]+|\\\r?\n)+") +_SHELL_QUOTING_RE = re.compile(r"""\\(.)|["']""", re.DOTALL) +# Bound on the text read after `docker ` to find the image operand. +_DOCKER_OPERAND_MAX_CHARS = 1024 + # RP2: Manifest-permission pre-staging _PERMISSION_EXPANSION_PATTERNS = [ (r'"permissions?"\s*:\s*\[[^\]]*\]', 0.60), @@ -207,6 +258,58 @@ def _get_parameters_map( # --------------------------------------------------------------------------- +def _docker_image_operand(subcommand: str, text: str, truncated: bool) -> tuple[str | None, int]: + """Return the image operand of a docker command and where reading stopped. + + *text* starts at the first argument after ``docker run|create|pull``. Options + and their values are skipped using docker's option table. The image is None + when it cannot be identified (an unknown option, a missing value, the end of + the command, or a word cut off by the read bound), so the caller still + reports the command. + """ + value_options, flag_options = _DOCKER_OPTIONS[subcommand.lower()] + pos = 0 + expect_value = False + end_of_options = False + while True: + gap = _SHELL_WORD_GAP_RE.match(text, pos) + word_match = _SHELL_WORD_RE.match(text, gap.end() if gap else pos) + if word_match is None or (truncated and word_match.end() == len(text)): + return None, pos + pos = word_match.end() + word = _SHELL_QUOTING_RE.sub(r"\1", word_match.group(0)) + if expect_value: + expect_value = False + elif end_of_options or not word.startswith("-") or word == "-": + return word, pos + elif word == "--": + end_of_options = True + elif word.startswith("--"): + name, has_value, _ = word.partition("=") + if name in value_options and not has_value: + expect_value = True + elif name not in flag_options and not has_value: + return None, pos + else: + # Short options combine (-it). The first one that takes a value uses + # the rest of the word (-p8080:80) or, if nothing is left, the next word. + for index, letter in enumerate(word[1:], start=1): + if "-" + letter in value_options: + expect_value = index == len(word) - 1 + break + if "-" + letter not in flag_options: + return None, pos + + +def _docker_image_has_pin(image: str) -> bool: + """Return whether *image* has a tag or digest. + + Only the last path component is checked, because a registry port + (``localhost:5000/team/tool``) is not a tag. + """ + return _VERSION_PIN_RE.search(image.rsplit("/", 1)[-1]) is not None + + def _operand_has_version_pin(line_remainder: str) -> bool: """Return whether a version pin is attached to the matched package operand. @@ -333,14 +436,21 @@ def _check_rp1( # docker without tag or digest for m in _RP1_DOCKER_CMD.finditer(content): budget.check_runtime(file_path) - full_match = m.group(0) - if _VERSION_PIN_RE.search(full_match): + operand_start = m.start(2) + operand_text = content[operand_start : operand_start + _DOCKER_OPERAND_MAX_CHARS] + truncated = operand_start + _DOCKER_OPERAND_MAX_CHARS < len(content) + image, operand_end = _docker_image_operand(m.group(1), operand_text, truncated) + if image is not None and _docker_image_has_pin(image): continue + # Report through the image, or through the last word read when the image + # could not be identified. + span_end = operand_start + operand_end if operand_end else m.end(1) + full_match = content[m.start() : span_end] line_num = _find_line(content, m.start()) budget.emit( Finding( rule_id="RP1", - message=f"Docker image referenced without tag or digest: '{full_match[:80]}'.", + message=f"Docker image referenced without tag or digest: '{full_match[:200]}'.", severity="MEDIUM", confidence=0.75, file=file_path, diff --git a/tests/nodes/test_security_end_to_end.py b/tests/nodes/test_security_end_to_end.py index 46fbd34ff..2da1b20eb 100644 --- a/tests/nodes/test_security_end_to_end.py +++ b/tests/nodes/test_security_end_to_end.py @@ -1971,7 +1971,7 @@ async def test_privileged_payload_in_referenced_reference_file_stays_install_uns " securityContext:\n" " privileged: true\n" "\n" - " docker run --privileged --pid=host vendor/collector:1.4\n" + " docker run --privileged --pid=host vendor/collector\n" ), }, ) diff --git a/tests/test_mcp_rug_pull.py b/tests/test_mcp_rug_pull.py index 244936326..fee1a37c1 100644 --- a/tests/test_mcp_rug_pull.py +++ b/tests/test_mcp_rug_pull.py @@ -18,6 +18,7 @@ from __future__ import annotations import json +import time from skillspector.nodes.analyzers.mcp_rug_pull import node from skillspector.nodes.build_context import build_context @@ -146,6 +147,104 @@ def test_rp1_docker_unpinned(): assert len(rp1) >= 1 +_DIGEST = "sha256:" + "0" * 64 + + +def _rp1_matches(content: str) -> list[str]: + result = node(_state(file_cache={"setup.sh": content})) + return [f.matched_text for f in result["findings"] if f.rule_id == "RP1"] + + +def test_rp1_docker_pinned_image_after_options_no_finding(): + """Options before a pinned image are not read as the image.""" + for content in ( + "docker run --rm alpine:3.20 cat /etc/alpine-release\n", + f"docker run -d img@{_DIGEST}\n", + f"docker run --rm -e A my-image@{_DIGEST}\n", + 'docker run -it --rm -v "$(pwd)":/app -w /app node:20 npm test\n', + "docker run -dp 8080:80 --name web nginx:1.27\n", + "docker run -p8080:80 --network=host nginx:1.27\n", + "docker run --gpus all --user 1000:1000 nvcr.io/nvidia/pytorch:24.01-py3\n", + 'docker run --rm --entrypoint "" -- alpine:3.20\n', + 'docker run --rm \\\n -v "$PWD:/work" \\\n ghcr.io/org/tool:1.4.2 lint\n', + "`docker create --name probe alpine:3.20`\n", + "docker pull -q --platform linux/amd64 alpine:3.20\n", + "docker pull -a localhost:5000/team/tool:1.0\n", + ): + assert _rp1_matches(content) == [], content + + +def test_rp1_docker_unpinned_image_after_options_names_image(): + """The finding names the image, and option values are not taken as its tag.""" + for content, expected in ( + ("docker run --rm alpine\n", "docker run --rm alpine"), + ( + "docker run --user 1000:1000 evil/image\n", + "docker run --user 1000:1000 evil/image", + ), + ( + "docker run --rm -e MODE=fast -p 8080:80 evil/image\n", + "docker run --rm -e MODE=fast -p 8080:80 evil/image", + ), + ("docker run --publish=8080:80 evil/image\n", "docker run --publish=8080:80 evil/image"), + ("docker run -p8080:80 evil/image\n", "docker run -p8080:80 evil/image"), + ("docker run -e TAG=1.2 evil/image:latest\n", "docker run -e TAG=1.2 evil/image:latest"), + ('docker run "--env=x:1" evil/image\n', 'docker run "--env=x:1" evil/image'), + ('docker run -e "A x:1" evil/image\n', 'docker run -e "A x:1" evil/image'), + ("docker run -e A\\ x:1 evil/image\n", "docker run -e A\\ x:1 evil/image"), + ("docker run --rm \\\n evil/image\n", "docker run --rm \\\n evil/image"), + ("docker pull localhost:5000/team/tool\n", "docker pull localhost:5000/team/tool"), + ("docker pull -a evil/image\n", "docker pull -a evil/image"), + ( + "docker run --rm --entrypoint /openshell-sandbox " + '"${SANDBOX_IMAGE:-ghcr.io/nvidia/openshell/sandbox:latest}" --version\n', + "docker run --rm --entrypoint /openshell-sandbox " + '"${SANDBOX_IMAGE:-ghcr.io/nvidia/openshell/sandbox:latest}"', + ), + ("docker run --rm alpine:3.20; docker run evil/image\n", "docker run evil/image"), + ): + assert _rp1_matches(content) == [expected], content + + +def test_rp1_docker_unresolved_image_is_still_reported(): + """When the image cannot be identified, the command stays reported.""" + for content, expected in ( + ("docker run --rm\n", "docker run --rm"), + ("docker run --rm | tee log\n", "docker run --rm"), + ("docker run -e\n", "docker run -e"), + ("docker run --bogus x:1 evil/image\n", "docker run --bogus"), + ("docker run -Z x:1 evil/image\n", "docker run -Z"), + ("docker run -dZ x:1 evil/image\n", "docker run -dZ"), + ("docker run -e A=$((1+2)) x:1 evil/image\n", "docker run -e A=$"), + ('docker run -e "A x:1 evil/image\n', "docker run -e"), + ("docker run --rm\nalpine:3.20\n", "docker run --rm"), + ): + assert _rp1_matches(content) == [expected], content + + # The read bound cuts this image after "localhost:5000"; that prefix is not + # taken as a tagged image. + padding = "-e A " * 202 + content = f"docker run {padding}localhost:5000/evil\n" + assert _rp1_matches(content) == [f"docker run {padding.rstrip()}"[:200]] + + +def test_rp1_docker_operand_scan_is_linear(): + """Adversarial lines finish quickly; the operand scan is bounded.""" + for content in ( + "docker run " + "-e A " * 10_000, + "docker run " * 4_545, + 'docker run -e "' + "a" * 50_000, + "docker run --rm $(" + "a" * 50_000, + "docker run -e " + "\\a" * 25_000, + "docker run " + "\\\n" * 25_000 + "img", + 'docker pull -q "' * 3_125, + ): + started = time.perf_counter() + _rp1_matches(content) + elapsed = time.perf_counter() - started + assert elapsed < 1.0, f"{elapsed:.2f}s for {content[:30]!r}" + + def test_rp1_docker_credentials_are_redacted_in_reports(): result = node( _state( From b25acf8112c651e3245ea598e20e41e96f634ef9 Mon Sep 17 00:00:00 2001 From: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Date: Wed, 7 Oct 2026 09:17:09 -0400 Subject: [PATCH 2/3] fix(rp1): keep RP1 for images from expansions and skip redirections The operand scan accepted two kinds of words that do not prove a pinned image: - An image built from an expansion. In "${IMAGE:-alpine:3.20}" the fallback's :3.20 was read as the tag although IMAGE may hold an unpinned image, and :1 in "${IMAGE:1}" is a substring offset. An image word with an expansion now counts as pinned only when every expansion is double-quoted and the literal text after the last one carries the tag or digest: it contains a / ("${REGISTRY}/tool:1.4"), or it starts with : or @ ("${IMAGE}:1.4"), which with no / after it can only begin a tag or digest. Unquoted expansions, brace expansion, "$@" and "${arr[@]}" are never resolved, because they can change which word is the image, and neither are command substitutions, whose closing ) can sit in a case pattern or a comment. - A redirection. In `docker run --rm 2>log:1 alpine`, 2>log:1 was taken as the image and :1 as its tag. Redirection operators now end a word (alpine>log:1 is alpine), and an operator is skipped with its target wherever it appears. A missing or unreadable target (2> at the end of the line, <(env)) leaves the image unresolved, so the command is still reported. A placeholder such as :1.0 is read the same way and is reported. Signed-off-by: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- .../nodes/analyzers/mcp_rug_pull.py | 132 ++++++++++++++++-- tests/test_mcp_rug_pull.py | 86 ++++++++++++ 2 files changed, 209 insertions(+), 9 deletions(-) diff --git a/src/skillspector/nodes/analyzers/mcp_rug_pull.py b/src/skillspector/nodes/analyzers/mcp_rug_pull.py index 4193cfd0c..aaffa2b6f 100644 --- a/src/skillspector/nodes/analyzers/mcp_rug_pull.py +++ b/src/skillspector/nodes/analyzers/mcp_rug_pull.py @@ -184,12 +184,21 @@ def analyzer_exhausted(self) -> bool: } # One shell word: unquoted text, $(...), backslash escapes (including a line # continuation) and complete quoted strings. The alternatives start with distinct -# characters, so matching is linear; an unterminated quote ends the word. +# characters, so matching is linear; an unterminated quote ends the word, and so +# does a redirection operator (alpine>log). _SHELL_WORD_RE = re.compile( - r"""(?:[^\s"'\\|&;()`$]+|\$(?:\([^()\n]*\))?|\\(?:\r?\n|.)|"(?:[^"\\\n]|\\.)*"|'[^'\n]*')+""" + r"""(?:[^\s"'\\|&;()<>`$]+|\$(?:\([^()\n]*\))?|\\(?:\r?\n|.)|"(?:[^"\\\n]|\\.)*"|'[^'\n]*')+""" ) _SHELL_WORD_GAP_RE = re.compile(r"(?:[ \t]+|\\\r?\n)+") _SHELL_QUOTING_RE = re.compile(r"""\\(.)|["']""", re.DOTALL) +# A redirection operator with its optional file descriptor (2>, &>>, 2>&, <<<, {fd}>). +# The shell removes it and its target word wherever it appears in the command. +_SHELL_REDIRECTION_RE = re.compile( + r"(?:[0-9]+|\{[A-Za-z_][A-Za-z0-9_]*\})?(?:&>>?|<<<|<<-?|<>|<&|>&|>>|>\||[<>])" +) +# A parameter name or a special parameter after `$` ($HOME, $1, $?). `$@` is not +# one of them here: it expands to several words even in double quotes. +_SHELL_PARAMETER_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*|[0-9*#?$!-]") # Bound on the text read after `docker ` to find the image operand. _DOCKER_OPERAND_MAX_CHARS = 1024 @@ -258,14 +267,112 @@ def _get_parameters_map( # --------------------------------------------------------------------------- +def _next_shell_word(text: str, pos: int, truncated: bool) -> re.Match[str] | None: + """Return the shell word after *pos*. + + None at the end of the command or when the word may be cut off by the read + bound. + """ + gap = _SHELL_WORD_GAP_RE.match(text, pos) + word_match = _SHELL_WORD_RE.match(text, gap.end() if gap else pos) + if word_match is None or (truncated and word_match.end() == len(text)): + return None + return word_match + + +def _expansion_end(word: str, start: int) -> int | None: + """Return where the parameter expansion at ``word[start]`` ends. + + None when the expansion cannot be read safely: + + - a command substitution or arithmetic (a backquote, ``$(...)``, ``$[...]``), + whose closing character can sit in a ``case`` pattern or a comment; + - ``$@`` or a ``${...}`` with ``@`` or ``[``, since ``"$@"`` and + ``"${arr[@]}"`` expand to several words even in double quotes; + - a ``${...}`` that is unclosed or holds quoting, escapes or a nested + substitution, any of which can hide its closing brace. + """ + if word[start] == "`" or word[start + 1 : start + 2] in ("(", "[", "@"): + return None + if word.startswith("${", start): + depth = 0 + for index in range(start + 1, len(word)): + char = word[index] + if char in "\\'\"`()[]@": + return None + if char == "{": + depth += 1 + elif char == "}": + depth -= 1 + if depth == 0: + return index + 1 + return None + # A `$` that starts no expansion ("$" at the end of a string) is literal. + parameter = _SHELL_PARAMETER_RE.match(word, start + 1) + return parameter.end() if parameter else start + 1 + + +def _docker_image_known_text(word: str) -> str | None: + """Return the part of an image word whose value is known, without quoting. + + An expansion (``$IMAGE``, ``${IMAGE:-alpine:3.20}``) has a value the scan + does not know. The tag or digest is still known when every expansion is + double-quoted and the literal text after the last one holds it: that text + contains a ``/``, so the last path component is literal + (``"${REGISTRY}/tool:1.4"``), or it starts with ``:`` or ``@`` + (``"${IMAGE}:1.4"``), which with no ``/`` after it can only begin a tag or + digest. That literal text is returned. Otherwise None is returned and the + command is reported. An unquoted expansion or brace expansion + (``{alpine,alpine:3.20}``) is never resolved, because it can change which + word is the image, and neither is a command substitution (see + ``_expansion_end``). + """ + known_from = 0 + in_double_quotes = False + brace_depth = 0 + index = 0 + while index < len(word): + char = word[index] + if char == "\\": + index += 2 + continue + if char == "'" and not in_double_quotes: + close = word.find("'", index + 1) + if close < 0: + return None + index = close + 1 + continue + if char in "$`": + end = _expansion_end(word, index) if in_double_quotes else None + if end is None: + return None + index = known_from = end + continue + if char == '"': + in_double_quotes = not in_double_quotes + elif not in_double_quotes and char == "{": + brace_depth += 1 + elif not in_double_quotes and char == "}" and brace_depth: + brace_depth -= 1 + elif not in_double_quotes and char == "," and brace_depth: + return None + index += 1 + known = _SHELL_QUOTING_RE.sub(r"\1", word[known_from:]) + if known_from and "/" not in known and not known.startswith((":", "@")): + return None + return known + + def _docker_image_operand(subcommand: str, text: str, truncated: bool) -> tuple[str | None, int]: """Return the image operand of a docker command and where reading stopped. *text* starts at the first argument after ``docker run|create|pull``. Options - and their values are skipped using docker's option table. The image is None - when it cannot be identified (an unknown option, a missing value, the end of - the command, or a word cut off by the read bound), so the caller still - reports the command. + and their values are skipped using docker's option table, and redirections + (``2>log``, ``<< tuple[ end_of_options = False while True: gap = _SHELL_WORD_GAP_RE.match(text, pos) - word_match = _SHELL_WORD_RE.match(text, gap.end() if gap else pos) - if word_match is None or (truncated and word_match.end() == len(text)): + redirection = _SHELL_REDIRECTION_RE.match(text, gap.end() if gap else pos) + if redirection is not None: + target = _next_shell_word(text, redirection.end(), truncated) + if target is None: + return None, redirection.end() + pos = target.end() + continue + word_match = _next_shell_word(text, pos, truncated) + if word_match is None: return None, pos pos = word_match.end() word = _SHELL_QUOTING_RE.sub(r"\1", word_match.group(0)) if expect_value: expect_value = False elif end_of_options or not word.startswith("-") or word == "-": - return word, pos + return _docker_image_known_text(word_match.group(0)), pos elif word == "--": end_of_options = True elif word.startswith("--"): diff --git a/tests/test_mcp_rug_pull.py b/tests/test_mcp_rug_pull.py index fee1a37c1..0c853f3fe 100644 --- a/tests/test_mcp_rug_pull.py +++ b/tests/test_mcp_rug_pull.py @@ -228,6 +228,88 @@ def test_rp1_docker_unresolved_image_is_still_reported(): assert _rp1_matches(content) == [f"docker run {padding.rstrip()}"[:200]] +def test_rp1_docker_image_pinned_after_an_expansion_no_finding(): + """A double-quoted expansion cannot change a tag or digest written after it.""" + for content in ( + 'docker run --rm "${REGISTRY}/tool:1.4" lint\n', + 'docker run "${REGISTRY:-ghcr.io}/org/tool:1.4.2"\n', + f'docker pull "$REGISTRY"/team/tool@{_DIGEST}\n', + 'docker run "${REGISTRY}/${NAME}:1.4"\n', + 'docker run --rm "$IMAGE:3.20"\n', + f'docker pull "${{IMAGE}}@{_DIGEST}"\n', + 'docker run -e "MODE=$MODE" --name "$(whoami)-job" alpine:3.20\n', + ): + assert _rp1_matches(content) == [], content + + +def test_rp1_docker_image_with_unknown_expansion_is_reported(): + """An image whose tag depends on an expansion's value stays reported.""" + for content, expected in ( + ('docker run --rm "${IMAGE:-alpine:3.20}"\n', 'docker run --rm "${IMAGE:-alpine:3.20}"'), + ('docker run --rm "${IMAGE:1}"\n', 'docker run --rm "${IMAGE:1}"'), + ('docker run "${IMAGE:-x/alpine:3.20}"\n', 'docker run "${IMAGE:-x/alpine:3.20}"'), + ('docker run "${X:-\\}/alpine:3.20}"\n', 'docker run "${X:-\\}/alpine:3.20}"'), + ('docker run "${IMAGE}:${TAG}"\n', 'docker run "${IMAGE}:${TAG}"'), + ("docker run $REGISTRY/tool:1.4\n", "docker run $REGISTRY/tool:1.4"), + ('docker run "$@/tool:1.4"\n', 'docker run "$@/tool:1.4"'), + ('docker run "${arr[@]}/tool:1.4"\n', 'docker run "${arr[@]}/tool:1.4"'), + ( + 'docker run "${IMAGE:-$(echo })/tool:1.4}"\n', + 'docker run "${IMAGE:-$(echo })/tool:1.4}"', + ), + ('docker run "$(echo registry)/tool:1.4"\n', 'docker run "$(echo registry)/tool:1.4"'), + ( + 'docker run "$(case x in x) echo alpine;; y/z:1) ;; esac)"\n', + 'docker run "$(case x in x) echo alpine;; y/z:1) ;; esac)"', + ), + ('docker run "$(echo alpine):3.20"\n', 'docker run "$(echo alpine):3.20"'), + ('docker run "`echo alpine`:3.20"\n', 'docker run "`echo alpine`:3.20"'), + ("docker run {alpine,alpine:3.20}\n", "docker run {alpine,alpine:3.20}"), + ): + assert _rp1_matches(content) == [expected], content + + +def test_rp1_docker_redirections_before_pinned_image_no_finding(): + """Redirections are not read as the image or as an option value.""" + for content in ( + "docker pull -q 2>/dev/null alpine:3.20\n", + "docker run --rm >out.log 2>&1 alpine:3.20 true\n", + "docker run --rm 2> err.log alpine:3.20\n", + "docker run -i >run.log alpine:3.20\n", + "docker run -e 2>err.log MODE=fast alpine:3.20\n", + "docker run --rm alpine:3.20>out.log\n", + ): + assert _rp1_matches(content) == [], content + + +def test_rp1_docker_redirection_target_is_not_taken_as_image(): + """A redirection's file name cannot pin the image that follows it.""" + for content, expected in ( + ("docker run --rm 2>log:1 alpine\n", "docker run --rm 2>log:1 alpine"), + ("docker run >out:1 alpine\n", "docker run >out:1 alpine"), + ("docker run >> out:1 alpine\n", "docker run >> out:1 alpine"), + ("docker run out:1 alpine\n", "docker run &>out:1 alpine"), + ("docker run 2>&1 >|out:1 alpine\n", "docker run 2>&1 >|out:1 alpine"), + ("docker run <<x:1 A evil/image\n", "docker run -e 2>x:1 A evil/image"), + ("docker run --rm alpine>log:1\n", "docker run --rm alpine"), + ): + assert _rp1_matches(content) == [expected], content + + +def test_rp1_docker_unclear_redirection_is_still_reported(): + """When a redirection cannot be read, the command stays reported.""" + for content, expected in ( + ("docker run --rm 2>\n", "docker run --rm 2>"), + ("docker run --rm 2>|\n", "docker run --rm 2>|"), + ("docker run --env-file <(env) alpine:3.20\n", "docker run --env-file <"), + ): + assert _rp1_matches(content) == [expected], content + + def test_rp1_docker_operand_scan_is_linear(): """Adversarial lines finish quickly; the operand scan is bounded.""" for content in ( @@ -238,6 +320,10 @@ def test_rp1_docker_operand_scan_is_linear(): "docker run -e " + "\\a" * 25_000, "docker run " + "\\\n" * 25_000 + "img", 'docker pull -q "' * 3_125, + "docker run " + "2>x " * 10_000, + "docker run " + "1" * 50_000 + ">x", + 'docker run "' + "${a" * 10_000 + '"', + 'docker run "' + "$(" * 10_000 + '"', ): started = time.perf_counter() _rp1_matches(content) From 1a86b0fe8920dd4f5e4a12aeb177c74c22f2d173 Mon Sep 17 00:00:00 2001 From: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Date: Sat, 10 Oct 2026 09:37:48 -0400 Subject: [PATCH 3/3] fix(rp1): keep RP1 for option values that may split and for 2&>log The operand scan still accepted two kinds of words that do not prove which word is the image: - An option value that the shell can turn into several arguments or none. With DOCKER_ENV='MODE=dev ubuntu', `docker run --rm -e $DOCKER_ENV alpine:3.20` runs ubuntu, and alpine:3.20 becomes the container command. Every option, option value and redirection target read before the image now has to stay one argument: no unquoted expansion ($ENV, -e$ENV, --env=$ENV, $(pwd)), brace expansion ({A,B}=1, X{1..2}), or glob, and no "$@" or "${arr[@]}". A command substitution in double quotes counts only when it is plain words, because a quote inside it ("$(echo " x ")") ends the double-quoted string early here, and a case pattern can hide its closing ). Otherwise the image is unresolved and the command is still reported. Redirection targets get the same check because the scan can end them early too (2>${LOG:-a b}). - A number before &> or &>>. Those operators take no file descriptor, so in `docker run --rm 2&>log alpine:3.20` the 2 is an argument and the image. The descriptor prefix now applies only to the other operators; `&>log alpine:3.20` still reads alpine:3.20 as the image. Signed-off-by: Ilai Goldschmidt <117302862+ilaigold@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 --- .../nodes/analyzers/mcp_rug_pull.py | 81 ++++++++++++++++--- tests/test_mcp_rug_pull.py | 67 +++++++++++++++ 2 files changed, 138 insertions(+), 10 deletions(-) diff --git a/src/skillspector/nodes/analyzers/mcp_rug_pull.py b/src/skillspector/nodes/analyzers/mcp_rug_pull.py index aaffa2b6f..d7a5d96a3 100644 --- a/src/skillspector/nodes/analyzers/mcp_rug_pull.py +++ b/src/skillspector/nodes/analyzers/mcp_rug_pull.py @@ -191,11 +191,15 @@ def analyzer_exhausted(self) -> bool: ) _SHELL_WORD_GAP_RE = re.compile(r"(?:[ \t]+|\\\r?\n)+") _SHELL_QUOTING_RE = re.compile(r"""\\(.)|["']""", re.DOTALL) -# A redirection operator with its optional file descriptor (2>, &>>, 2>&, <<<, {fd}>). -# The shell removes it and its target word wherever it appears in the command. +# A redirection operator with its optional file descriptor (2>, >>, 2>&, <<<, {fd}>). +# `&>` and `&>>` take none: in `2&>log` the shell passes `2` as an argument. The +# shell removes a redirection and its target word wherever they appear in the command. _SHELL_REDIRECTION_RE = re.compile( - r"(?:[0-9]+|\{[A-Za-z_][A-Za-z0-9_]*\})?(?:&>>?|<<<|<<-?|<>|<&|>&|>>|>\||[<>])" + r"&>>?|(?:[0-9]+|\{[A-Za-z_][A-Za-z0-9_]*\})?(?:<<<|<<-?|<>|<&|>&|>>|>\||[<>])" ) +# A command substitution of plain words. Unless it runs `case`, its closing `)` or +# backquote is the one matched here; quotes, escapes, nesting or a comment could move it. +_SHELL_PLAIN_SUBSTITUTION_RE = re.compile(r"\$\(([\w \t.,:=+%/-]*)\)|`([\w \t.,:=+%/-]*)`") # A parameter name or a special parameter after `$` ($HOME, $1, $?). `$@` is not # one of them here: it expands to several words even in double quotes. _SHELL_PARAMETER_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*|[0-9*#?$!-]") @@ -363,6 +367,56 @@ def _docker_image_known_text(word: str) -> str | None: return known +def _shell_word_is_one_argument(word: str) -> bool: + """Return whether the shell word *word* is certain to stay one argument. + + Outside quotes, an expansion (``$X``, ``$(pwd)``), brace expansion + (``{a,b}``, ``{1..3}``) or a pattern (``*``, ``?``, ``[``) can produce + several arguments or none, and so can ``"$@"`` or ``"${arr[@]}"`` in double + quotes (see ``_expansion_end``). A command substitution in double quotes + counts only when it is plain words: a quote inside it ends the double-quoted + string early here, so the shell's word may be longer than *word*. + """ + in_double_quotes = False + brace_depth = 0 + index = 0 + while index < len(word): + char = word[index] + if char == "\\": + index += 2 + continue + if char == "'" and not in_double_quotes: + close = word.find("'", index + 1) + if close < 0: + return False + index = close + 1 + continue + if char in "$`": + if not in_double_quotes: + return False + plain = _SHELL_PLAIN_SUBSTITUTION_RE.match(word, index) + if plain is not None and "case" not in plain[plain.lastindex].split(): + index = plain.end() + continue + end = _expansion_end(word, index) + if end is None: + return False + index = end + continue + if char == '"': + in_double_quotes = not in_double_quotes + elif not in_double_quotes and char in "*?[": + return False + elif not in_double_quotes and char == "{": + brace_depth += 1 + elif not in_double_quotes and char == "}" and brace_depth: + brace_depth -= 1 + elif not in_double_quotes and brace_depth and (char == "," or word.startswith("..", index)): + return False + index += 1 + return True + + def _docker_image_operand(subcommand: str, text: str, truncated: bool) -> tuple[str | None, int]: """Return the image operand of a docker command and where reading stopped. @@ -370,9 +424,10 @@ def _docker_image_operand(subcommand: str, text: str, truncated: bool) -> tuple[ and their values are skipped using docker's option table, and redirections (``2>log``, ``<< tuple[ redirection = _SHELL_REDIRECTION_RE.match(text, gap.end() if gap else pos) if redirection is not None: target = _next_shell_word(text, redirection.end(), truncated) - if target is None: - return None, redirection.end() + # A target that may not be one argument can also end later than the + # word read here (2>${LOG:-a b}). + if target is None or not _shell_word_is_one_argument(target.group(0)): + return None, target.end() if target else redirection.end() pos = target.end() continue word_match = _next_shell_word(text, pos, truncated) @@ -392,10 +449,14 @@ def _docker_image_operand(subcommand: str, text: str, truncated: bool) -> tuple[ return None, pos pos = word_match.end() word = _SHELL_QUOTING_RE.sub(r"\1", word_match.group(0)) + if not expect_value and (end_of_options or not word.startswith("-") or word == "-"): + return _docker_image_known_text(word_match.group(0)), pos + # An option or value that splits into several arguments, or none, moves the + # image to another word (-e $ENV with ENV='A=1 ubuntu'). + if not _shell_word_is_one_argument(word_match.group(0)): + return None, pos if expect_value: expect_value = False - elif end_of_options or not word.startswith("-") or word == "-": - return _docker_image_known_text(word_match.group(0)), pos elif word == "--": end_of_options = True elif word.startswith("--"): diff --git a/tests/test_mcp_rug_pull.py b/tests/test_mcp_rug_pull.py index 0c853f3fe..f1c49d25c 100644 --- a/tests/test_mcp_rug_pull.py +++ b/tests/test_mcp_rug_pull.py @@ -278,8 +278,11 @@ def test_rp1_docker_redirections_before_pinned_image_no_finding(): "docker run -i >run.log alpine:3.20\n", + "docker run --rm &>run.log alpine:3.20\n", + "docker run -e 2&>run.log alpine:3.20\n", "docker run -e 2>err.log MODE=fast alpine:3.20\n", "docker run --rm alpine:3.20>out.log\n", + 'docker run --rm 2>"$LOG" >"$(pwd)/out.log" alpine:3.20\n', ): assert _rp1_matches(content) == [], content @@ -306,6 +309,63 @@ def test_rp1_docker_unclear_redirection_is_still_reported(): ("docker run --rm 2>\n", "docker run --rm 2>"), ("docker run --rm 2>|\n", "docker run --rm 2>|"), ("docker run --env-file <(env) alpine:3.20\n", "docker run --env-file <"), + # With LOG=err.log the shell redirects to err.log and runs ubuntu. + ("docker run --rm 2>${LOG:-err x:1} ubuntu\n", "docker run --rm 2>${LOG:-err"), + ('docker run --rm 2>"$(echo " x:1 ")" ubuntu\n', 'docker run --rm 2>"$(echo "'), + ): + assert _rp1_matches(content) == [expected], content + + +def test_rp1_docker_number_before_ampersand_redirection_is_an_argument(): + """`&>` takes no file descriptor, so in `2&>log` the `2` is the image.""" + for content, expected in ( + ("docker run --rm 2&>log alpine:3.20\n", "docker run --rm 2"), + ("docker run --rm 2&>>log alpine:3.20\n", "docker run --rm 2"), + ("docker run --rm {fd}&>log alpine:3.20\n", "docker run --rm {fd}"), + ): + assert _rp1_matches(content) == [expected], content + + +def test_rp1_docker_option_value_that_is_one_argument_no_finding(): + """Quoted expansions and substitutions stay one argument before the image.""" + for content in ( + 'docker run --rm -e "$DOCKER_ENV" alpine:3.20\n', + 'docker run --rm -e"$DOCKER_ENV" --env="${MODE:-dev}" alpine:3.20\n', + 'docker run --rm -e "$*" alpine:3.20\n', + "docker run --rm -e 'MODE=$X' -e MODE=\\$X alpine:3.20\n", + "docker run --rm -e \"A={x,y}\" -e 'GLOB=*.py' -e A={} alpine:3.20\n", + "docker run --rm -v ~/.cache:/root/.cache alpine:3.20\n", + 'docker run --rm -v "$(git rev-parse --show-toplevel)":/src alpine:3.20\n', + 'docker run --rm --user "$(id -u):$(id -g)" alpine:3.20\n', + 'docker run --rm -v "`pwd`":/app alpine:3.20\n', + ): + assert _rp1_matches(content) == [], content + + +def test_rp1_docker_option_value_that_can_split_is_reported(): + """An option value that can become several arguments, or none, can move the image.""" + for content, expected in ( + # With DOCKER_ENV='MODE=dev ubuntu' the image is ubuntu. + ("docker run --rm -e $DOCKER_ENV alpine:3.20\n", "docker run --rm -e $DOCKER_ENV"), + ("docker run --rm -e$DOCKER_ENV alpine:3.20\n", "docker run --rm -e$DOCKER_ENV"), + ("docker run --rm --env=$DOCKER_ENV alpine:3.20\n", "docker run --rm --env=$DOCKER_ENV"), + ("docker run --rm -e ${DOCKER_ENV} alpine:3.20\n", "docker run --rm -e ${DOCKER_ENV}"), + ("docker run --rm=$RM alpine:3.20\n", "docker run --rm=$RM"), + ('docker run --rm -e "${ARGS[@]}" alpine:3.20\n', 'docker run --rm -e "${ARGS[@]}"'), + ('docker run --rm --env="${ARGS[@]}" alpine:3.20\n', 'docker run --rm --env="${ARGS[@]}"'), + ('docker run --rm -e "$@" alpine:3.20\n', 'docker run --rm -e "$@"'), + ("docker run --rm -e $* alpine:3.20\n", "docker run --rm -e $*"), + ("docker run --rm -v $(pwd):/app node:20\n", "docker run --rm -v $(pwd):/app"), + ("docker run --rm -e {A,B}=1 alpine:3.20\n", "docker run --rm -e {A,B}=1"), + ("docker run --rm -e X{1..2}=a alpine:3.20\n", "docker run --rm -e X{1..2}=a"), + ("docker run --rm -v * alpine:3.20\n", "docker run --rm -v *"), + # A quote inside the substitution ends the double-quoted string early here. + ('docker run --rm -e "$(echo " x:1 ")" ubuntu\n', 'docker run --rm -e "$(echo "'), + ('docker run --rm -e "`echo " x:1 "`" ubuntu\n', 'docker run --rm -e "`echo "'), + ( + 'docker run --rm --name "$(case a in a) echo " x:1 ";; esac)" ubuntu\n', + 'docker run --rm --name "$(case a in a) echo "', + ), ): assert _rp1_matches(content) == [expected], content @@ -322,8 +382,15 @@ def test_rp1_docker_operand_scan_is_linear(): 'docker pull -q "' * 3_125, "docker run " + "2>x " * 10_000, "docker run " + "1" * 50_000 + ">x", + "docker run " + "1" * 50_000 + "&>x", + "docker run " + "2&>x " * 10_000, 'docker run "' + "${a" * 10_000 + '"', 'docker run "' + "$(" * 10_000 + '"', + 'docker run -e "' + "$(a" * 10_000 + '"', + 'docker run -e "$(' + "a " * 25_000 + ')" x', + 'docker run -e "x" ' * 5_000, + "docker run -e " + "{a," * 10_000, + "docker run -e " + "{a.." * 10_000, ): started = time.perf_counter() _rp1_matches(content)