Skip to content

Harden docker-sbx install: remove curl|sudo sh root install #62991

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
Copilot wants to merge 6 commits into from
Closed

Harden docker-sbx install: remove curl|sudo sh root install #62991

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cca9839
Initial plan
Copilot Sep 23, 2026
daab099
Harden sudo_docker_sbx_install.sh against curl|sudo sh supply-chain risk
Copilot Sep 23, 2026
63b999e
Merge branch 'main' into copilot/setup-security-fix
pelikhan Sep 23, 2026
820506d
Verify Docker repository signing key
Copilot Sep 23, 2026
56149e1
Merge branch 'main' into copilot/setup-security-fix
pelikhan Sep 23, 2026
3c476df
Validate all Docker key fingerprints
Copilot Sep 23, 2026
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
5 changes: 5 additions & 0 deletions .changeset/harden-docker-sbx-install-apt-repo.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 22 additions & 2 deletions actions/setup/sh/sudo_docker_sbx_install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,32 @@ set +o histexpand
#
# Usage: sudo_docker_sbx_install.sh
# No arguments required.
#
# Key notes:
# - Adds the Docker apt repository deterministically (pinned GPG key fetched to a
# file, signed-by apt source line) instead of piping the get.docker.com
# convenience script into a root shell. See:
# https://docs.docker.com/engine/install/ubuntu/#install-using-the-repository

set -euo pipefail

echo "::group::Install docker-sbx"
# Add Docker apt repo without installing Docker Engine (already present).
curl -fsSL https://get.docker.com | sudo REPO_ONLY=1 sh
KEYRING_PATH="/etc/apt/keyrings/docker.asc"
SOURCE_LIST="/etc/apt/sources.list.d/docker.list"
DISTRO_ID="$(. /etc/os-release && echo "$ID")"
DISTRO_CODENAME="$(. /etc/os-release && echo "$VERSION_CODENAME")"

echo "Adding Docker apt repository for ${DISTRO_ID} ${DISTRO_CODENAME}..."
sudo install -m 0755 -d /etc/apt/keyrings
# runner-guard:ignore RGS-012 -- fetches Docker's GPG signing key to a file (never piped to a shell); the key is used only to verify the signed-by apt repository below.
curl -fsSL "https://download.docker.com/linux/${DISTRO_ID}/gpg" -o /tmp/docker.asc

Copilot AI Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 820506d: the downloaded key is verified with gpg --show-keys against the pinned Docker fingerprint before installation. The inline rotation instruction requires verifying any replacement against Docker's published signing-key documentation.

Copilot AI Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 820506d: TestDockerSbxShellScriptContent now requires the fingerprint verification, signed-by source, secure temporary file, and Docker download endpoint, while rejecting the legacy endpoint and pipe-to-shell form.

@github-actions github-actions Bot Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] /tmp/docker.asc is a predictable, fixed filename in a world-writable shared directory — on a multi-tenant or reused runner this is a classic TOCTOU/symlink race: another process (or a prior failed run) could pre-create/replace that path before install -m 0644 reads it, silently poisoning the trust root this script is trying to harden.

💡 Suggested fix

Use mktemp for the scratch file instead of a fixed /tmp path:

KEY_TMP="$(mktemp)"
trap 'rm -f "$KEY_TMP"' EXIT
curl -fsSL "(download.docker.com/redacted) -o "$KEY_TMP"
sudo install -m 0644 "$KEY_TMP" "${KEYRING_PATH}"

This closes the same class of predictable-path risk the PR is otherwise fixing (unverified root-trust material), and is consistent with treating /tmp as untrusted on shared runners.

@copilot please address this.

Copilot AI Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 820506d: the downloaded key now uses mktemp and an EXIT cleanup trap, eliminating the predictable shared /tmp/docker.asc path.

@github-actions github-actions Bot Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Symlink/TOCTOU risk with predictable /tmp/docker.asc path. This introduces an unprivileged curl write to a fixed, world-writable /tmp path, followed by a privileged sudo install copy into the apt trust-root keyring (/etc/apt/keyrings/docker.asc). A local attacker/process could pre-create /tmp/docker.asc as a symlink before this step runs, and the subsequent sudo install would then copy attacker-controlled content into the apt-trusted keyring.

The sibling install_gh_cli.sh avoids this class of bug entirely by piping straight into the privileged write (curl -fsSL ... | sudo dd of="${KEYRING_PATH}"), never touching an intermediate world-writable file. Recommend the same pattern here, or at minimum use mktemp for a private, non-predictable temp file instead of the fixed /tmp/docker.asc path.

@copilot please address this.

Copilot AI Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 820506d: the installer now uses a private mktemp file and cleans it on exit, preventing a predictable-path symlink or TOCTOU attack.

sudo install -m 0644 /tmp/docker.asc "${KEYRING_PATH}"
rm -f /tmp/docker.asc

echo "deb [arch=$(dpkg --print-architecture) signed-by=${KEYRING_PATH}] https://download.docker.com/linux/${DISTRO_ID} ${DISTRO_CODENAME} stable" \

@github-actions github-actions Bot Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hard-codes ${DISTRO_CODENAME} into the apt source without validating that Docker publishes that suite, so on runners or derivatives where the field is empty or unsupported this step now dies at apt-get update and takes every docker-sbx workflow down.

💡 Why this matters and how to harden it

The old get.docker.com path handled distro detection internally; this replacement moved that responsibility into our script but never added the guardrails. At minimum, fail fast with a clear error when VERSION_CODENAME is empty, and preferably verify that the Docker repo exposes dists/${DISTRO_ID}/dists/${DISTRO_CODENAME} before writing the source line.

Copilot AI Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in 820506d: the installer now validates non-empty ID and VERSION_CODENAME values before writing the apt source, failing with a clear error when either is unavailable.

| sudo tee "${SOURCE_LIST}" > /dev/null

sudo apt-get update -qq
sudo apt-get install -y docker-sbx
sbx version
# Fix KVM permissions so the runner user can create microVMs.
Expand Down
Loading