fix: do not let a stale cache directory suppress signature verification - #228
Merged
Merged
Conversation
`ct.sh` treated the existence of the cache directory as proof that a
verified install had happened, and interpolated unvalidated version
strings into filesystem paths and into `$GITHUB_PATH` / `$GITHUB_ENV`.
Three related problems followed from that.
1. `mkdir -p "${cache_dir}"` ran before the download and verification,
so a run that failed verification left an empty directory behind.
Every later run on the same runner then took the cache-hit path,
skipped `cosign` entirely, and executed whatever was at
`${cache_dir}/ct`.
2. Because `${version}` was interpolated straight into `cache_dir`, a
value containing `../` resolved outside `$RUNNER_TOOL_CACHE`
altogether. Pointing it at an existing attacker-controlled directory
made the cache-hit path fire on the first run: `cosign` was never
invoked, the script exited 0, and that directory was prepended to
`$GITHUB_PATH` for every subsequent step.
3. A newline in a version string added extra lines to the
`$GITHUB_ENV` file, which the runner parses one `KEY=VALUE` per
line, allowing arbitrary environment variables (`LD_PRELOAD`, say)
to be set for later steps.
Fixes:
- Validate all three version inputs against a strict version pattern,
rejecting path separators and newlines at the point of entry.
- Gate the cache-hit path on `[[ ! -x "${cache_dir}/ct" ]]` rather than
on the directory existing, so a partially populated cache no longer
counts as a verified install.
- Download, verify and extract into a `mktemp -d` staging directory and
publish to `${cache_dir}` only once all three succeed, with an EXIT
trap removing the staging directory on every path. A failed run now
leaves nothing behind that a later run could reuse.
- Add `--fail` to the download so an HTTP error is reported as a
download failure instead of being saved as the "tarball" and
resurfacing as a misleading signature-verification error.
Residual, and accepted: an executable `ct` pre-seeded at the validated
cache path still short-circuits verification. That requires write access
to `$RUNNER_TOOL_CACHE`, which means code already running on the runner,
and it is the same trust model the `actions/setup-*` tool cache uses.
Verified with a harness using fake curl/cosign/uv:
- traversal version -> rejected, exit 1 (was: exit 0, cosign never
invoked)
- newline version -> rejected, exit 1, $GITHUB_ENV untouched
- empty cached dir -> cosign now invoked and install completes
(was: exit 127, cosign never invoked)
- failed verification -> exit 1, zero entries left under the tool cache,
$GITHUB_PATH untouched, no staging dir leaked
- clean install -> exit 0, correct $GITHUB_PATH, `ct version` runs
- unknown version -> "Unable to download", not "Unable to validate"
shellcheck, `bash -n` and `zsh -n` all clean.
Signed-off-by: Carlos Panato <ctadeu@gmail.com>
The existing jobs in test-action.yml only exercise successful installs. That covers a broken install loudly, but leaves every failure path unguarded -- and those are the ones that regress silently. A change that stopped invoking cosign, or that let a leftover cache directory count as a verified install, would keep all three jobs green. Add tests/ct_test.sh, which stubs curl, cosign and uv so the control flow can be exercised without network access, and a workflow to run it. Real cosign and real downloads stay covered by the end-to-end jobs in test-action.yml, so stubbing them here does not leave a gap. Covered: - hostile version strings (path traversal, absolute path, command substitution, semicolon, embedded newline) are rejected before reaching a filesystem path or the runner files - a v-prefixed version and a prerelease version are still accepted - a leftover cache directory does not suppress verification - a failed verification leaves nothing under the tool cache and does not write to $GITHUB_PATH - the staging directory is removed on failure - a download error is reported as a download error, not as a signature-verification error - a successful install publishes the binary, $GITHUB_PATH and CT_CONFIG_DIR - a missing $RUNNER_TOOL_CACHE is an error The suite is deliberately checked against both versions of the script: 13/13 pass on this branch, and 8 of the 13 fail on the unhardened script for the expected reasons, so the tests demonstrably exercise the fix rather than merely passing alongside it. The job lives in a new workflow file rather than as another job in test-action.yml to avoid colliding with the other in-flight changes to that file. Signed-off-by: Carlos Panato <ctadeu@gmail.com>
setup() exported TMPDIR into the sandbox directory that teardown() then deleted, so the next test case called `mktemp -d` with TMPDIR pointing at a path that no longer existed. This passed locally and failed on the runner because of a platform difference: GNU mktemp honours TMPDIR for its default template and fails when the directory is missing, while the BSD mktemp on macOS ignores TMPDIR and always uses /tmp. Only the first test case ran in CI. Scope TMPDIR to the ct.sh invocation in run_ct() instead, so the staging directory is still observable without the harness's own mktemp calls depending on it. Verified by emulating GNU mktemp semantics through a PATH wrapper: the previous commit reproduces the CI failure after the first test case, and this version reports 13/13 under both GNU and BSD semantics. The differential against the unhardened script is unchanged at 8 of 13 failing, so the suite still exercises the fix. Signed-off-by: Carlos Panato <ctadeu@gmail.com>
davidkarlsen
approved these changes
Sep 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ct.shtreated the existence of the cache directory as proof that averified install had happened, and interpolated unvalidated version
strings into filesystem paths and into
$GITHUB_PATH/$GITHUB_ENV.Three related problems followed from that.
mkdir -p "${cache_dir}"ran before the download and verification,so a run that failed verification left an empty directory behind.
Every later run on the same runner then took the cache-hit path,
skipped
cosignentirely, and executed whatever was at${cache_dir}/ct.Because
${version}was interpolated straight intocache_dir, avalue containing
../resolved outside$RUNNER_TOOL_CACHEaltogether. Pointing it at an existing attacker-controlled directory
made the cache-hit path fire on the first run:
cosignwas neverinvoked, the script exited 0, and that directory was prepended to
$GITHUB_PATHfor every subsequent step.A newline in a version string added extra lines to the
$GITHUB_ENVfile, which the runner parses oneKEY=VALUEperline, allowing arbitrary environment variables (
LD_PRELOAD, say)to be set for later steps.
Fixes:
Validate all three version inputs against a strict version pattern,
rejecting path separators and newlines at the point of entry.
Gate the cache-hit path on
[[ ! -x "${cache_dir}/ct" ]]rather thanon the directory existing, so a partially populated cache no longer
counts as a verified install.
Download, verify and extract into a
mktemp -dstaging directory andpublish to
${cache_dir}only once all three succeed, with an EXITtrap removing the staging directory on every path. A failed run now
leaves nothing behind that a later run could reuse.
Add
--failto the download so an HTTP error is reported as adownload failure instead of being saved as the "tarball" and
resurfacing as a misleading signature-verification error.
Residual, and accepted: an executable
ctpre-seeded at the validatedcache path still short-circuits verification. That requires write access
to
$RUNNER_TOOL_CACHE, which means code already running on the runner,and it is the same trust model the
actions/setup-*tool cache uses.Verified with a harness using fake curl/cosign/uv:
invoked)
(was: exit 127, cosign never invoked)
$GITHUB_PATH untouched, no staging dir leaked
ct versionrunsshellcheck,
bash -nandzsh -nall clean.