From d9c7d4d5877abd56dc1432b44593ab6d29a2f788 Mon Sep 17 00:00:00 2001 From: duriantaco Date: Wed, 21 Oct 2026 09:55:58 +0800 Subject: [PATCH 1/2] fix(security): trace checkout credentials and artifact exposure --- dictionary.md | 2 +- skylos/rules/config/cicd/ai_agents.py | 20 ++ skylos/rules/config/cicd/github_actions.py | 357 +++++++++++++++++++-- test/test_github_actions_security.py | 18 +- test/test_repository_ci_scope.py | 4 + test/test_workflow_credential_proofs.py | 215 +++++++++++++ 6 files changed, 590 insertions(+), 26 deletions(-) create mode 100644 test/test_workflow_credential_proofs.py diff --git a/dictionary.md b/dictionary.md index 75e51fb45..300980aa4 100644 --- a/dictionary.md +++ b/dictionary.md @@ -427,7 +427,7 @@ that directory. | D290 | HIGH | Dangerous trigger (`pull_request_target`, `workflow_run`) | GitHub Actions | | D291 | MEDIUM-HIGH | Missing or excessive permissions | GitHub Actions | | D292 | HIGH (LOW for GitHub-owned) | Unpinned action or reusable workflow. GitHub-owned `actions/*` and `github/*` at a tag or branch are LOW (GitHub runs the workflow and owns them; zizmor's default policy allows ref-pins for them); every other owner, dynamic `${{ }}` refs and refs with no `@` are HIGH | GitHub Actions | -| D293 | MEDIUM | Checkout persists credentials | GitHub Actions | +| D293 | MEDIUM | Checkout persists credentials where they can reach someone new: a later `actions/upload-artifact` of the checkout (`.git` included: before v4.4, or with `include-hidden-files: true`, or a SHA pin), a pull request's code checked out in a job accounts without write access can start, or an explicitly write-scoped token beside a later third-party action. GitHub-owned and local actions already hold the token; an undeclared token scope is D291's finding | GitHub Actions | | D294 | HIGH | Template injection from untrusted context | GitHub Actions | | D295 | HIGH | Self-hosted runner exposure | GitHub Actions | | D296 | MEDIUM | Unpinned container image | GitHub Actions | diff --git a/skylos/rules/config/cicd/ai_agents.py b/skylos/rules/config/cicd/ai_agents.py index 40b977e1b..23cef196f 100644 --- a/skylos/rules/config/cicd/ai_agents.py +++ b/skylos/rules/config/cicd/ai_agents.py @@ -1283,6 +1283,26 @@ def step_runs(self, job_id: str, index: int, event: str, untrusted: bool) -> boo return True +def outsider_job_events( + data: dict[str, Any], path: Path, root: Path | None +) -> dict[str, list[str]]: + """For each job, the events on which an account without write access can + make it run: the workflow's untrusted triggers that the job's ``if``, + ``needs`` and recognized permission checks do not shut out. An + environment name does not prove required approval. Jobs no such event + reaches are omitted.""" + events = _untrusted_events(data, path, root, {}) + if not events: + return {} + reach = _Reach(data) + reachable = {} + for job_id in reach.jobs: + job_events = [event for event in events if reach.job_runs(job_id, event, True)] + if job_events: + reachable[job_id] = job_events + return reachable + + # ------------------------------------------------------------ privilege diff --git a/skylos/rules/config/cicd/github_actions.py b/skylos/rules/config/cicd/github_actions.py index 2a495336c..b7c126313 100644 --- a/skylos/rules/config/cicd/github_actions.py +++ b/skylos/rules/config/cicd/github_actions.py @@ -5,6 +5,7 @@ import re import shlex from collections.abc import Iterator +from fnmatch import fnmatchcase from pathlib import Path from typing import Any @@ -1016,6 +1017,17 @@ def _unpinned_uses_severity(value: str) -> str: return "HIGH" +def _is_third_party_action(value: str) -> bool: + """A remote action whose code neither the repository nor GitHub owns.""" + value = value.strip() + if not value or value.startswith(("./", "../")): + return False + if value.lower().startswith("docker://"): + return True + owner = value.partition("/")[0].lower() + return owner not in _GITHUB_OWNED_ACTION_OWNERS + + def _is_pinned_uses(value: str) -> bool: value = value.strip() if not value or value.startswith("./") or value.startswith("../"): @@ -1194,6 +1206,11 @@ def _scan_triggers( ) +def _line_for_yaml_path(lines: list[str], path: tuple[Any, ...]) -> int | None: + source = _source_for_lines(lines) + return source.line_for_path(path) if source is not None else None + + def _scan_permissions( data: dict[str, Any], path: Path, @@ -1349,27 +1366,326 @@ def _scan_uses( ), ) - if "SKY-D293" not in ignore and uses_value.lower().startswith( - "actions/checkout@" - ): + + +_UPLOAD_ARTIFACT_VERSION_RE = re.compile(r"^v(\d+)(?:\.(\d+)(?:\.\d+)?)?$") +_WORKSPACE_PREFIX_RE = re.compile( + r"^(?:\$\{\{\s*github\.workspace\s*\}\}|\$\{?GITHUB_WORKSPACE\}?)(?:/|$)" +) + + +def _literal_action_input(value: Any) -> Any: + """Normalize only a complete, bounded scalar Actions literal expression.""" + if not isinstance(value, str) or len(value) > 4096: + return value + match = re.fullmatch( + r"\$\{\{\s*(true|false|null|'(?:[^']|'')*')\s*\}\}", + value.strip(), re.IGNORECASE, + ) + if match is None: + return value + literal = match.group(1) + if literal.lower() == "true": + return True + if literal.lower() == "false": + return False + if literal.lower() == "null": + return None + return literal[1:-1].replace("''", "'") + + +def _action_input_reference(value: Any) -> str | None: + """Return an input's reference spelling, never an embedded credential value.""" + if not isinstance(value, str) or len(value) > 512: + return None + match = re.fullmatch( + r"\$\{\{\s*((?:secrets|env|inputs|vars|steps|needs)" + r"(?:\.[A-Za-z_][A-Za-z0-9_-]*){1,5})\s*\}\}", + value.strip(), re.IGNORECASE, + ) + if match is not None: + return match.group(1) + match = re.fullmatch( + r"\$\{\{\s*(secrets|env|inputs|vars)\[\s*'([A-Za-z_][A-Za-z0-9_-]*)'\s*\]" + r"\s*\}\}", value.strip(), re.IGNORECASE, + ) + if match is not None: + return match.group(1) + "." + match.group(2) + return None + + +def _workspace_relative(path: str) -> str: + """``path`` relative to the workspace, without ``./`` or a trailing slash.""" + path = _WORKSPACE_PREFIX_RE.sub("", path.strip()) + parts = [part for part in path.split("/") if part not in {"", "."}] + return "/".join(parts) + + +def _upload_skips_hidden_files(uses_value: str, with_config: dict[str, Any]) -> bool: + """actions/upload-artifact leaves out hidden files such as ``.git`` from + v4.4.0 unless ``include-hidden-files`` is set. A floating ``@v4`` resolves + to the newest v4 release; a SHA pin's version is unknown.""" + hidden = _literal_action_input(with_config.get("include-hidden-files")) + hidden_state = _actions_bool(hidden) + if hidden_state is True: + return False + if "include-hidden-files" in with_config and hidden_state is None: + # An input or expression may enable hidden files at runtime. + return False + ref = uses_value.rsplit("@", 1)[1].strip() if "@" in uses_value else "" + match = _UPLOAD_ARTIFACT_VERSION_RE.match(ref) + if match is None: + return False + major = int(match.group(1)) + minor = int(match.group(2)) if match.group(2) is not None else None + return major > 4 or (major == 4 and (minor is None or minor >= 4)) + + +def _artifact_pattern_covers_file( + pattern: str, target: str, *, directory_only: bool = False +) -> bool | None: + """Match the supported Actions glob syntax, including implicit descendants. + + Unknown syntax cannot prove that a negative pattern excludes credentials. + Match path segments separately so ``*`` cannot consume a slash. + """ + unsupported = ("$", "{", "}", "(", ")", "\\") + if len(pattern) > 4096 or any(char in pattern for char in unsupported): + return None + pattern_parts = pattern.split("/") if pattern else [] + target_parts = target.split("/") + if len(pattern_parts) > 128 or len(target_parts) > 128: + return None + positions = {0} + for part in pattern_parts: + next_positions = set() + for position in positions: + if part == "**": + next_positions.update(range(position, len(target_parts) + 1)) + elif position < len(target_parts) and fnmatchcase( + target_parts[position], part + ): + next_positions.add(position + 1) + positions = next_positions + if not positions: + return False + # A trailing separator matches directories, never the credential file itself. + if directory_only: + return any(position < len(target_parts) for position in positions) + # Actions glob includes descendants of matching directories by default. + return bool(positions) + + +def _upload_includes_checkout(step: dict[str, Any], checkout_dir: str) -> str | None: + """The uploaded path that holds the checkout's ``.git`` directory, if any.""" + uses_value = step.get("uses") + if not isinstance(uses_value, str) or not _uses_matches( + uses_value, "actions/upload-artifact" + ): + return None + with_config = step.get("with") if isinstance(step.get("with"), dict) else {} + upload = with_config.get("path") + if not isinstance(upload, str) or _upload_skips_hidden_files(uses_value, with_config): + return None + target = ".git/config" + if checkout_dir: + target = checkout_dir + "/" + target + included = None + for entry in upload.splitlines(): + entry = entry.strip() + if not entry or entry.startswith("#"): + continue + excluded = False + pattern = entry + while pattern.startswith("!"): + excluded = not excluded + pattern = pattern[1:].strip() + if not pattern: + continue + if excluded and pattern.startswith(("/", "~")): + # An absolute exclusion cannot be equated with a workspace-relative + # path. Only a recognized workspace prefix establishes that relation. + matched = None + else: + matched = _artifact_pattern_covers_file( + _workspace_relative(pattern), target, + directory_only=pattern.endswith("/"), + ) + if excluded: + if matched is True: + included = None + elif matched is not False: + included = entry + return included + + +def _checkout_credential(with_config: dict[str, Any]) -> tuple[str, bool]: + """The persisted credential and whether GITHUB_TOKEN scopes constrain it.""" + if _literal_action_input(with_config.get("ssh-key")): + # Checkout v4 configures HTTP token auth as well as its SSH command. + token_config = dict(with_config) + del token_config["ssh-key"] + token_name, _ = _checkout_credential(token_config) + ssh_name = "the configured checkout SSH key" + reference = _action_input_reference(with_config.get("ssh-key")) + if reference is not None: + ssh_name += " from " + reference + return f"{token_name} and {ssh_name}", False + token = with_config.get("token") + if token is None: + return "GITHUB_TOKEN", True + if isinstance(token, str) and re.fullmatch( + r"\s*\$\{\{\s*(?:github\s*(?:\.\s*token|\[\s*'token'\s*\])" + r"|secrets\s*(?:\.\s*GITHUB_TOKEN|\[\s*'GITHUB_TOKEN'\s*\]))\s*\}\}\s*", + token, re.I, + ): + return "GITHUB_TOKEN", True + token_name = "the configured checkout token" + reference = _action_input_reference(token) + if reference is not None: + token_name += " from " + reference + return token_name, False + + +def _token_write_scopes(job: dict[str, Any], workflow_permissions: Any) -> list[str]: + """GITHUB_TOKEN scopes the job is explicitly granted write on. ``id-token`` + mints OIDC tokens and adds nothing to the persisted repository token.""" + permissions = job.get("permissions", workflow_permissions) + if isinstance(permissions, str): + return ["write-all"] if permissions == "write-all" else [] + if not isinstance(permissions, dict): + return [] + return [ + str(scope) + for scope, level in permissions.items() + if str(level).lower() == "write" and str(scope) != "id-token" + ] + + +def _persisted_credential_exposure( + steps: list[Any], + index: int, + *, + write_scopes: list[str], + outsider_events: list[str], +) -> str | None: + """Why the credential a checkout persists can reach someone + the job did not already hand it to, or None without such evidence. + + zizmor's ``artipacked`` names the clearest path, an artifact that uploads + the checkout. The others: the checkout fetches a pull request's code in a + job that accounts without write access can start, so their code runs + beside the token, or a write-scoped token sits next to a later third-party + action. A step the repository or GitHub owns already holds the token, so + it is not a new reader; a token whose scope is undeclared is reported by + SKY-D291 instead. + """ + checkout = steps[index] + with_config = checkout.get("with") if isinstance(checkout.get("with"), dict) else {} + credential, github_token = _checkout_credential(with_config) + checkout_path = with_config.get("path") + checkout_dir = _workspace_relative(checkout_path) if isinstance(checkout_path, str) else "" + later = [step for step in steps[index + 1 :] if isinstance(step, dict)] + for step in later: + uploaded = _upload_includes_checkout(step, checkout_dir) + if uploaded is not None: + return f"a later step uploads {uploaded} as an artifact, .git/config included" + target = " ".join(str(with_config.get(key, "")) for key in ("ref", "repository")) + if outsider_events and _PR_CODE_REF_RE.search(target): + return ( + "it checks out pull request code in a job that accounts without " + f"write access can start ({', '.join(outsider_events)})" + ) + if write_scopes or not github_token: + for step in later: + uses_value = step.get("uses") + if isinstance(uses_value, str) and _is_third_party_action(uses_value): + if github_token: + return ( + f"the token has {', '.join(write_scopes)} write access and the " + f"later third-party action {uses_value.strip()} can read it" + ) + return ( + f"the later third-party action {uses_value.strip()} can read " + f"{credential}, whose access is not limited by GITHUB_TOKEN permissions" + ) + return None + + +def _scan_persisted_checkout_credentials( + data: dict[str, Any], + path: Path, + lines: list[str], + findings: list[dict[str, Any]], + ignore: set[str], + *, + is_workflow: bool, + root: Path | None, +) -> None: + rule_id = "SKY-D293" + if rule_id in ignore: + return + if is_workflow: + from skylos.rules.config.cicd.ai_agents import outsider_job_events + + outsiders = outsider_job_events(data, path, root) + workflow_permissions = data.get("permissions") + jobs = [ + (job_id, job.get("steps"), _token_write_scopes(job, workflow_permissions)) + for job_id, job in _jobs(data) + ] + else: + # A composite action runs in its caller's job: only an upload inside + # the action itself is evidence here. + runs = data.get("runs") if isinstance(data.get("runs"), dict) else {} + outsiders = {} + jobs = [(None, runs.get("steps"), [])] + for job_id, steps, write_scopes in jobs: + if not isinstance(steps, list): + continue + for index, step in enumerate(steps): + if not isinstance(step, dict): + continue + uses_value = step.get("uses") + if not isinstance(uses_value, str) or not _uses_matches( + uses_value, "actions/checkout" + ): + continue with_config = step.get("with") if isinstance(step.get("with"), dict) else {} - if not _is_false(with_config.get("persist-credentials")): - _add_finding( - findings, - lines, - _finding( - rule_id="SKY-D293", - name="github-actions-checkout-credentials", - message=( - "actions/checkout leaves credentials persisted by default. " - "Set persist-credentials: false unless later git pushes need it." - ), - file=path, - line=_line_for_contains(lines, uses_value), - severity="MEDIUM", - value=uses_value, + if _is_false(_literal_action_input(with_config.get("persist-credentials"))): + continue + exposure = _persisted_credential_exposure( + steps, + index, + write_scopes=write_scopes, + outsider_events=outsiders.get(str(job_id), []), + ) + if exposure is None: + continue + credential, _ = _checkout_credential(with_config) + step_path = ( + ("jobs", job_id, "steps", index, "uses") + if is_workflow + else ("runs", "steps", index, "uses") + ) + _add_finding( + findings, + lines, + _finding( + rule_id=rule_id, + name="github-actions-checkout-credentials", + message=( + f"actions/checkout persists {credential} for later git commands, and " + f"{exposure}. Set persist-credentials: false unless a later " + "git push needs it." ), - ) + file=path, + line=_line_for_yaml_path(lines, step_path) + or _line_for_contains(lines, uses_value), + severity="MEDIUM", + value=uses_value, + ), + ) def _scan_run_blocks( @@ -2476,6 +2792,9 @@ def scan_github_actions_file( ) _scan_uses(data, file_path, lines, findings, ignore, is_workflow=is_workflow) + _scan_persisted_checkout_credentials( + data, file_path, lines, findings, ignore, is_workflow=is_workflow, root=root_path + ) _scan_run_blocks(data, file_path, lines, findings, ignore, is_workflow=is_workflow) _scan_container_images( data, file_path, lines, findings, ignore, is_workflow=is_workflow diff --git a/test/test_github_actions_security.py b/test/test_github_actions_security.py index b33fae002..f51abf314 100644 --- a/test/test_github_actions_security.py +++ b/test/test_github_actions_security.py @@ -54,6 +54,9 @@ def _write_risky_workflow(path): runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + - uses: someorg/lint-action@v1 - run: echo "${{ github.event.pull_request.title }}" """.lstrip(), encoding="utf-8", @@ -95,24 +98,24 @@ def test_github_actions_scanner_deduplicates_same_line_findings(tmp_path): one: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: someorg/action@v1 two: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: someorg/action@v1 """.lstrip(), encoding="utf-8", ) findings = scan_github_actions(tmp_path) - checkout_credentials = [ + unpinned = [ finding for finding in findings - if finding["rule_id"] == "SKY-D293" - and finding.get("value") == "actions/checkout@v4" + if finding["rule_id"] == "SKY-D292" + and finding.get("value") == "someorg/action@v1" ] - assert len(checkout_credentials) == 1 + assert len(unpinned) == 1 def test_github_actions_run_block_env_exfil_flags(tmp_path): @@ -849,6 +852,9 @@ def test_analyzer_reports_github_actions_dangers_without_source_files(tmp_path): runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + - uses: someorg/build-action@v1 """.lstrip(), encoding="utf-8", ) diff --git a/test/test_repository_ci_scope.py b/test/test_repository_ci_scope.py index 5d11e35e8..a415e847c 100644 --- a/test/test_repository_ci_scope.py +++ b/test/test_repository_ci_scope.py @@ -26,6 +26,10 @@ " steps:\n" " - uses: actions/checkout@v4\n" " - run: echo hello\n" + " - uses: actions/upload-artifact@0123456789012345678901234567890123456789\n" + " with:\n" + " path: .\n" + " if-no-files-found: error\n" ) # Any security finding fails the gate: the point is that repository CI findings # reach it, whatever their severity (an unpinned GitHub-owned action is LOW). diff --git a/test/test_workflow_credential_proofs.py b/test/test_workflow_credential_proofs.py new file mode 100644 index 000000000..31fb69618 --- /dev/null +++ b/test/test_workflow_credential_proofs.py @@ -0,0 +1,215 @@ +"""Persisted credentials require their actual token and artifact value flow.""" + +import pytest +import yaml + +from skylos.core.safe_cache_io import write_text_no_symlink +from skylos.rules.config.cicd.github_actions import scan_github_actions_file + + +def _findings(tmp_path, checkout=None, later=None, permissions=None): + folder = tmp_path / ".github" / "workflows" + folder.mkdir(parents=True, exist_ok=True) + path = folder / "ci.yml" + first = {"uses": "actions/checkout@v4"} + if checkout: + first["with"] = checkout + steps = [first] + if later: + steps.append(later) + workflow = { + "name": "CI", + "on": "push", + "permissions": permissions or {}, + "jobs": {"build": {"runs-on": "ubuntu-latest", "steps": steps}}, + } + assert write_text_no_symlink(path, yaml.safe_dump(workflow), encoding="utf-8") + return [ + finding + for finding in scan_github_actions_file(path, root=tmp_path) + if finding["rule_id"] == "SKY-D293" + ] + + +_THIRD_PARTY = {"uses": "vendor/action@0123456789012345678901234567890123456789"} + + +@pytest.mark.parametrize("credential", ["token", "ssh-key"]) +@pytest.mark.parametrize("permissions", [{}, {"contents": "read"}]) +def test_custom_checkout_credential_is_not_limited_by_github_permissions( + tmp_path, credential, permissions +): + findings = _findings( + tmp_path, + {credential: "${{ secrets.REPO_CREDENTIAL }}"}, + _THIRD_PARTY, + permissions, + ) + assert len(findings) == 1 + expected = "SSH key" if credential == "ssh-key" else "checkout token" + assert expected in findings[0]["message"] + assert "not limited by GITHUB_TOKEN permissions" in findings[0]["message"] + + +@pytest.mark.parametrize("credential", ["token", "ssh-key"]) +def test_custom_credential_without_an_exposure_path_is_not_reported( + tmp_path, credential +): + checkout = {credential: "${{ secrets.REPO_CREDENTIAL }}"} + assert _findings(tmp_path, checkout) == [] + assert _findings(tmp_path, checkout, {"uses": "./local-action"}) == [] + checkout["persist-credentials"] = False + assert _findings(tmp_path, checkout, _THIRD_PARTY) == [] + + +@pytest.mark.parametrize("persist", [False, "${{ false }}", "${{ 'false' }}"]) +def test_nonpersisting_custom_token_does_not_expose_credentials(tmp_path, persist): + checkout = {"token": "${{ secrets.REPO_PAT }}", "persist-credentials": persist} + assert _findings(tmp_path, checkout, _THIRD_PARTY, {"contents": "read"}) == [] + + +@pytest.mark.parametrize("persist", [True, "${{ true }}", "${{ inputs.persist }}"]) +def test_unknown_or_enabled_persistence_does_not_hide_a_custom_token(tmp_path, persist): + checkout = {"token": "${{ secrets.REPO_PAT }}", "persist-credentials": persist} + assert len(_findings(tmp_path, checkout, _THIRD_PARTY, {"contents": "read"})) == 1 + + +@pytest.mark.parametrize("ssh_key", ["", "${{ '' }}", "${{ null }}"]) +def test_empty_ssh_input_does_not_invent_a_custom_credential(tmp_path, ssh_key): + assert ( + _findings(tmp_path, {"ssh-key": ssh_key}, _THIRD_PARTY, {"contents": "read"}) + == [] + ) + + +@pytest.mark.parametrize("ssh_key", ["${{ secrets.SSH_KEY }}", "${{ inputs.ssh_key }}"]) +def test_unknown_ssh_key_remains_independent_of_github_token_scope(tmp_path, ssh_key): + assert ( + len( + _findings( + tmp_path, {"ssh-key": ssh_key}, _THIRD_PARTY, {"contents": "read"} + ) + ) + == 1 + ) + + +@pytest.mark.parametrize( + ("credential", "reference", "spelling"), + [ + ("token", "${{ secrets.REPO_PAT }}", "secrets.REPO_PAT"), + ("token", "${{ secrets['REPO_PAT'] }}", "secrets.REPO_PAT"), + ("token", "${{ env.TOKEN }}", "env.TOKEN"), + ( + "token", + "${{ steps.issue_token.outputs.token }}", + "steps.issue_token.outputs.token", + ), + ("ssh-key", "${{ secrets.SSH_KEY }}", "secrets.SSH_KEY"), + ], +) +def test_checkout_credential_evidence_names_only_the_reference( + tmp_path, credential, reference, spelling +): + findings = _findings(tmp_path, {credential: reference}, _THIRD_PARTY) + assert spelling in findings[0]["message"] + + +def test_literal_credential_values_are_never_copied_into_evidence(tmp_path): + literal = "private credential bytes" + findings = _findings(tmp_path, {"token": literal}, _THIRD_PARTY) + assert len(findings) == 1 + assert literal not in findings[0]["message"] + + +@pytest.mark.parametrize( + "token", + [ + None, + "${{ github.token }}", + "${{ secrets.GITHUB_TOKEN }}", + "${{ github['token'] }}", + "${{ secrets['GITHUB_TOKEN'] }}", + ], +) +def test_known_github_token_keeps_its_actual_scope(tmp_path, token): + checkout = {"token": token} if token is not None else {} + assert _findings(tmp_path, checkout, _THIRD_PARTY, {"contents": "read"}) == [] + assert len(_findings(tmp_path, checkout, _THIRD_PARTY, {"contents": "write"})) == 1 + + +def _upload(path=".", hidden=None, ref="v4"): + config = {"path": path} + if hidden is not None: + config["include-hidden-files"] = hidden + return {"uses": f"actions/upload-artifact@{ref}", "with": config} + + +@pytest.mark.parametrize( + ("hidden", "reported"), + [ + (None, False), + (False, False), + (True, True), + ("${{ true }}", True), + ("${{ false }}", False), + ("${{ inputs.hidden }}", True), + ("${{ matrix.hidden }}", True), + ("${{ inputs.hidden && true || false }}", True), + ], +) +def test_hidden_file_upload_states_are_conservative(tmp_path, hidden, reported): + assert bool(_findings(tmp_path, later=_upload(hidden=hidden))) is reported + + +def test_hidden_file_proof_still_needs_an_upload_of_the_checkout(tmp_path): + assert _findings(tmp_path, later=_upload("dist/", "${{ true }}")) == [] + assert _findings(tmp_path, later=_upload("dist/", "${{ inputs.hidden }}")) == [] + assert _findings(tmp_path, later={"run": "echo done"}) == [] + + +@pytest.mark.parametrize( + ("path", "checkout", "reported"), + [ + ("src/**", {"path": "src"}, True), + ("src/**/*", {"path": "src"}, True), + ("src/*", {"path": "src"}, True), + ("src/dist/**", {"path": "src"}, False), + (".\n!.git/**", {}, False), + (".\n!.git/config", {}, False), + (".\n!.gitignore", {}, True), + (".\n!.git/config.backup", {}, True), + (".\n!.git/config/", {}, True), + (".\n!.git/", {}, False), + ("src/**\n!src/.git/**", {"path": "src"}, False), + ("src/**\n!src2/.git/**", {"path": "src"}, True), + ("src/**\n!/src/.git/config", {"path": "src"}, True), + ("src/**\n! /src/.git/config", {"path": "src"}, True), + ("src/**\n!${{ github.workspace }}/src/.git/config", {"path": "src"}, False), + ("!.git/**\n.", {}, True), + (".\n!${{ inputs.excluded }}", {}, True), + (".\n!.git/**\n.git/config", {}, True), + (".\n!.git/**\n!!.git/config", {}, True), + (".\n!!!.git/config", {}, False), + ], +) +def test_artifact_globs_and_exclusions_follow_the_credential_file( + tmp_path, path, checkout, reported +): + assert bool(_findings(tmp_path, checkout, _upload(path, True))) is reported + + +def test_ssh_configuration_does_not_remove_the_persisted_http_token(tmp_path): + findings = _findings( + tmp_path, {"ssh-key": "${{ secrets.SSH_KEY }}"}, _upload(hidden=True) + ) + assert len(findings) == 1 + assert "GITHUB_TOKEN" in findings[0]["message"] + + +def test_unknown_action_ref_cannot_prove_hidden_files_are_excluded(tmp_path): + assert len(_findings(tmp_path, later=_upload(ref="v4.4-custom"))) == 1 + + +def test_legacy_upload_does_not_honor_a_newer_hidden_files_input(tmp_path): + assert len(_findings(tmp_path, later=_upload(hidden=False, ref="v4.3.6"))) == 1 From eb94c3599be16c0e5befba0ecde422cbb28e0297 Mon Sep 17 00:00:00 2001 From: duriantaco Date: Wed, 21 Oct 2026 09:56:03 +0800 Subject: [PATCH 2/2] fix(security): require workflow authorization evidence --- dictionary.md | 2 +- skylos/rules/catalog.py | 2 +- skylos/rules/config/cicd/ai_agents.py | 42 +++----- skylos/rules/config/cicd/github_actions.py | 23 +++- test/test_github_actions_ai_agents.py | 3 +- test/test_github_actions_security.py | 1 - test/test_workflow_authorization_proofs.py | 116 +++++++++++++++++++++ 7 files changed, 156 insertions(+), 33 deletions(-) create mode 100644 test/test_workflow_authorization_proofs.py diff --git a/dictionary.md b/dictionary.md index 300980aa4..8f71559fb 100644 --- a/dictionary.md +++ b/dictionary.md @@ -447,7 +447,7 @@ that directory. | D310 | HIGH | OIDC token exposed to local build script | GitHub Actions | | D311 | MEDIUM | Lax artifact upload | GitHub Actions | | D312 | MEDIUM | JavaScript install scripts in CI | GitHub Actions | -| D313 | MEDIUM | Privileged job missing timeout | GitHub Actions | +| D313 | LOW | Privileged job missing a bounded timeout while reachable accounts without write access can run pull request code. The default is 360 minutes on both GitHub-hosted and self-hosted runners; trusted jobs and jobs that do not run PR code are not reported | GitHub Actions | | D350 | MEDIUM-CRITICAL | AI agent on an untrusted trigger with no actor gate | GitHub Actions | | D351 | MEDIUM-CRITICAL | AI agent action's write-access check widened (`*`) | GitHub Actions | | D352 | MEDIUM-HIGH | AI agent can run any shell command on an untrusted trigger | GitHub Actions | diff --git a/skylos/rules/catalog.py b/skylos/rules/catalog.py index a023c5999..c9c3112d3 100644 --- a/skylos/rules/catalog.py +++ b/skylos/rules/catalog.py @@ -965,7 +965,7 @@ def to_dict(self) -> dict: "SKY-D313", "GitHub Actions privileged job missing timeout", "security", - "MEDIUM", + "LOW", finding_type="review", ), RuleCatalogEntry( diff --git a/skylos/rules/config/cicd/ai_agents.py b/skylos/rules/config/cicd/ai_agents.py index 23cef196f..c1503112b 100644 --- a/skylos/rules/config/cicd/ai_agents.py +++ b/skylos/rules/config/cicd/ai_agents.py @@ -1186,13 +1186,6 @@ def _job_routes_by_event(job: dict[str, Any] | None) -> bool: ) -def _approval_environment(job: dict[str, Any]) -> bool: - """An environment chosen per run, the documented way to require approval - for fork pull requests. A fixed name usually only scopes secrets.""" - environment = job.get("environment") - if isinstance(environment, dict): - environment = environment.get("name") - return isinstance(environment, str) and "${{" in environment def _literal_environment(job: dict[str, Any]) -> str: @@ -1205,9 +1198,7 @@ def _literal_environment(job: dict[str, Any]) -> str: def _job_authorizes(job: dict[str, Any]) -> bool: - """An approval environment or a permission check.""" - if _approval_environment(job): - return True + """A recognized permission check; an environment name proves no approval.""" steps = job.get("steps") return isinstance(steps, list) and any( isinstance(step, dict) and _step_checks_permission(step) for step in steps @@ -1242,23 +1233,20 @@ def job_runs( return True inner = stack + (job_id,) result = True - if untrusted and _approval_environment(job): - result = False - else: - condition = parse_condition(job.get("if")) - if not has_status_override(condition): - for dep in _needs(job): - if self._blocked(dep, event, untrusted, inner): - result = False - break - if result: - ctx = EvalContext( - event, - untrusted, - lambda dep: self._blocked(dep, event, untrusted, inner), - lambda dep: _job_routes_by_event(self.jobs.get(dep)), - ) - result = evaluate(condition, ctx)[0] + condition = parse_condition(job.get("if")) + if not has_status_override(condition): + for dep in _needs(job): + if self._blocked(dep, event, untrusted, inner): + result = False + break + if result: + ctx = EvalContext( + event, + untrusted, + lambda dep: self._blocked(dep, event, untrusted, inner), + lambda dep: _job_routes_by_event(self.jobs.get(dep)), + ) + result = evaluate(condition, ctx)[0] self._memo[key] = result return result diff --git a/skylos/rules/config/cicd/github_actions.py b/skylos/rules/config/cicd/github_actions.py index b7c126313..ed138d364 100644 --- a/skylos/rules/config/cicd/github_actions.py +++ b/skylos/rules/config/cicd/github_actions.py @@ -2712,16 +2712,26 @@ def _scan_privileged_job_timeouts( lines: list[str], findings: list[dict[str, Any]], ignore: set[str], + *, + root: Path | None = None, ) -> None: + """A privileged job lets outsider-controlled PR code consume its timeout. + + The default job timeout is 360 minutes on hosted and self-hosted runners. + Only a reachable outsider PR-code path justifies this security advisory. + """ rule_id = "SKY-D313" if rule_id in ignore: return + from skylos.rules.config.cicd.ai_agents import outsider_job_events + trigger = _on_value(data) workflow_permissions = data.get("permissions") workflow_privileged = _is_release_like_workflow( trigger ) or _workflow_has_dangerous_trigger(trigger) + outsiders = outsider_job_events(data, path, root) for job_id, job in _jobs(data): if _is_reusable_job(job) or "timeout-minutes" in job: continue @@ -2734,6 +2744,13 @@ def _scan_privileged_job_timeouts( ) if not privileged: continue + if outsiders.get(str(job_id)) and _job_reads_pr_code(job): + reason = ( + "it runs pull request code that accounts without write access " + f"can start ({', '.join(outsiders[str(job_id)])})" + ) + else: + continue _add_finding( findings, lines, @@ -2741,7 +2758,7 @@ def _scan_privileged_job_timeouts( rule_id=rule_id, name="github-actions-missing-timeout", message=( - f"Privileged or release-like job {job_id} has no timeout-minutes. " + f"Privileged job {job_id} has no timeout-minutes and {reason}. " "Set a bounded timeout to limit hung or compromised workflow runs." ), file=path, @@ -2783,7 +2800,9 @@ def scan_github_actions_file( _scan_cache_poisoning(data, file_path, lines, findings, ignore) _scan_broad_secret_env(data, file_path, lines, findings, ignore) _scan_oidc_build_script_exposure(data, file_path, lines, findings, ignore) - _scan_privileged_job_timeouts(data, file_path, lines, findings, ignore) + _scan_privileged_job_timeouts( + data, file_path, lines, findings, ignore, root=root_path + ) # Imported here: ai_agents builds on this module's helpers. from skylos.rules.config.cicd.ai_agents import scan_ai_agent_workflows diff --git a/test/test_github_actions_ai_agents.py b/test/test_github_actions_ai_agents.py index 013bfc985..56c0705b3 100644 --- a/test/test_github_actions_ai_agents.py +++ b/test/test_github_actions_ai_agents.py @@ -1317,7 +1317,8 @@ def test_fixed_environment_is_noted_but_not_treated_as_approval(tmp_path): "ENV", "${{ github.event.issue.author_association == 'OWNER' && 'trusted' || 'needs-approval' }}" ), ) - assert "SKY-D350" not in _by_rule(_agent_findings(tmp_path)) + # Naming a different environment for outsiders does not establish reviewers. + assert "SKY-D350" in _by_rule(_agent_findings(tmp_path)) def test_codex_sandbox_with_network_access_is_tools(tmp_path): diff --git a/test/test_github_actions_security.py b/test/test_github_actions_security.py index f51abf314..b6fcfed3c 100644 --- a/test/test_github_actions_security.py +++ b/test/test_github_actions_security.py @@ -457,7 +457,6 @@ def test_github_actions_scanner_detects_issue_derived_hardening_gaps(tmp_path): "SKY-D310", "SKY-D311", "SKY-D312", - "SKY-D313", }.issubset(_rule_ids(findings)) diff --git a/test/test_workflow_authorization_proofs.py b/test/test_workflow_authorization_proofs.py new file mode 100644 index 000000000..b402b39ac --- /dev/null +++ b/test/test_workflow_authorization_proofs.py @@ -0,0 +1,116 @@ +"""Naming an environment or permission check does not enforce authorization.""" + +import pytest +import yaml + +from skylos.core.safe_cache_io import write_text_no_symlink +from skylos.rules.config.cicd.github_actions import scan_github_actions_file + + +def _scan( + tmp_path, + *, + environment=None, + condition=None, + runner="ubuntu-latest", + trigger="pull_request_target", + pr_code=True, + timeout=None, + guard=None, +): + folder = tmp_path / ".github" / "workflows" + folder.mkdir(parents=True, exist_ok=True) + path = folder / "ci.yml" + checkout = {"uses": "actions/checkout@v4"} + if pr_code: + checkout["with"] = {"ref": "${{ github.event.pull_request.head.sha }}"} + job = { + "runs-on": runner, + "steps": [ + checkout, + { + "run": 'claude -p "Review ${{ github.event.pull_request.body }}" --dangerously-skip-permissions', + "env": {"ANTHROPIC_API_KEY": "${{ secrets.AGENT_KEY }}"}, + }, + ], + } + if environment is not None: + job["environment"] = environment + if condition is not None: + job["if"] = condition + if timeout is not None: + job["timeout-minutes"] = timeout + jobs = {"review": job} + if guard is not None: + jobs["authorize"] = guard + job["needs"] = "authorize" + workflow = { + "name": "Review", + "on": trigger, + "permissions": {"contents": "write"}, + "jobs": jobs, + } + assert write_text_no_symlink(path, yaml.safe_dump(workflow), encoding="utf-8") + return scan_github_actions_file(path, root=tmp_path) + + +def _ids(findings): + return {finding["rule_id"] for finding in findings} + + +@pytest.mark.parametrize( + "environment", + [ + "preview", + "${{ 'preview' }}", + "${{ inputs.environment }}", + "${{ matrix.environment }}", + "${{ github.actor }}", + "${{ github.event.pull_request.head.ref }}", + "${{ github.event.pull_request.head.repo.fork && 'approval' || 'preview' }}", + "${{ github.event.pull_request.author_association == 'OWNER' && 'trusted' || 'approval' }}", + {"name": "${{ 'preview' }}"}, + ], +) +def test_environment_selection_does_not_prove_approval(tmp_path, environment): + ids = _ids(_scan(tmp_path, environment=environment)) + assert {"SKY-D293", "SKY-D313", "SKY-D350"} <= ids + + +def test_actual_job_actor_guard_blocks_outsider_reachability(tmp_path): + findings = _scan( + tmp_path, condition="github.event.pull_request.author_association == 'OWNER'" + ) + assert not ({"SKY-D293", "SKY-D313", "SKY-D350"} & _ids(findings)) + + +def test_actual_dependency_guard_blocks_outsider_reachability(tmp_path): + guard = { + "if": "github.event.pull_request.author_association == 'OWNER'", + "runs-on": "ubuntu-latest", + "steps": [{"run": "echo approved"}], + } + assert not ( + {"SKY-D293", "SKY-D313", "SKY-D350"} & _ids(_scan(tmp_path, guard=guard)) + ) + # A status override intentionally bypasses the skipped dependency. + assert {"SKY-D293", "SKY-D313", "SKY-D350"} <= _ids( + _scan(tmp_path, guard=guard, condition="always()") + ) + + +def test_dependency_environment_does_not_prove_approval(tmp_path): + guard = { + "environment": "${{ 'approval' }}", + "steps": [{"run": "echo approved"}], + "runs-on": "ubuntu-latest", + } + assert {"SKY-D293", "SKY-D313", "SKY-D350"} <= _ids(_scan(tmp_path, guard=guard)) + + +@pytest.mark.parametrize("runner", ["ubuntu-latest", ["self-hosted", "linux"]]) +def test_timeout_requires_the_outsider_pr_code_path(tmp_path, runner): + assert "SKY-D313" not in _ids(_scan(tmp_path, runner=runner, trigger="push")) + assert "SKY-D313" not in _ids(_scan(tmp_path, runner=runner, pr_code=False)) + assert "SKY-D313" not in _ids(_scan(tmp_path, runner=runner, timeout=15)) + assert "SKY-D313" in _ids(_scan(tmp_path, runner=runner))