Harden cloud-hypervisor privileged runtime path and add explicit human-review warning - #52757
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
@lpcox review |
PR Triage
Automated triage by PR Triage Agent.
|
This comment has been minimized.
This comment has been minimized.
PR TriageCategory: Score breakdown
Recommended action:
|
|
Great work hardening the cloud-hypervisor privileged runtime path! 🔒 This PR addresses the critical security findings from the UK AI Operational Resilience governance review (#52748) with focused, targeted hardening: ✅ KVM access scope tightened — enhanced host checks and explicit ACL verification for the runner user The PR is well-focused on one security objective, includes tests, and the description clearly maps changes to risk mitigation. This is ready for review by security and core team maintainers.
|
PR TriageCategory: bug/security | Risk: high | Score: 80/100
Recommended action:
|
PR Triage
Security-hardening for privileged cloud-hypervisor sandbox runtime path. Draft PR touching critical-path security code — needs human security review before merge. Automated triage — see [PR Triage Report] for full context.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #52757 does not have the 'implementation' label and has only 44 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The hardening direction is good, but two of the new verification gates are fragile enough to break valid runners or leave the containment check weaker than advertised.
Blocking themes
- The
/dev/kvmACL verifier assumes a singlegetfacloutput shape and can reject valid ACLs when an effective-permissions suffix is present. - The extracted-file containment check validates the spelled path, not the canonical path, so it does not independently prove the resolved artifact still lives under the extraction root.
- I also attempted to use the requested
grumpy-codersub-agent, but the executable is not available in this environment, so its output was discarded.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 5.21 AIC · ⌖ 5.34 AIC · ⊞ 6.7K
Comment /review to run again
| echo "::error::failed to read /dev/kvm ACLs for verification." | ||
| exit 1 | ||
| fi | ||
| if ! grep -Eq "^user:${runner_uid}:rw-?$" <<<"${acl_output}"; then |
There was a problem hiding this comment.
This ACL verification is too brittle: getfacl commonly emits user:UID:rw- with a trailing \t#effective:rw- comment when a mask is present, so your anchored regex will reject a valid ACL and fail the setup on otherwise healthy runners.
💡 Why this blocks the change
setfacl -m u:${runner_uid}:rw /dev/kvm can create an entry that still prints an #effective: suffix depending on the existing ACL mask. In that case grep -Eq "^user:${runner_uid}:rw-?$" does not match, even though the runner really does have the intended access. That turns a hardening check into a false-negative gate on some hosts.
A safer verification is to tolerate the optional effective-permissions annotation, for example:
if ! grep -Eq "^user:${runner_uid}:rw-?(\t#effective:rw-?)?$" <<<"${acl_output}"; then
echo "::error::failed to verify scoped ACL entry for the runner user on /dev/kvm."
exit 1
fior, even better, query the permissions with a tool/output format that is not sensitive to cosmetic formatting differences.
| echo "::error::invalid extracted cloud-hypervisor bundle file: ${file}" | ||
| exit 1 | ||
| fi | ||
| case "${file}" in |
There was a problem hiding this comment.
This path check can be bypassed by a tarball that stores the payload under a symlinked directory: resolve_path returns realpath, but validate_extracted_file only string-prefixes against ${extract_dir}, so a file extracted through bundle/link -> /somewhere still passes as bundle/link/file even though the resolved file lives outside the extraction root.
💡 Why this matters
The new preflight blocks link entries from tar -tvzf, but the post-extraction guard should still validate the resolved canonical path rather than the original spelling. Right now the check is:
case "${file}" in
"${extract_dir}"/*) ;;If file is /tmp/.../bundle/link/cloud-hypervisor and link points elsewhere, the prefix test succeeds while realpath "$file" is outside ${extract_dir}. That leaves the safety property dependent on the tar listing parser never missing a link variant.
Please compare canonicalized paths instead, e.g. realpath "$file" must start with $(realpath "$extract_dir")/, so the containment check stays correct even if a symlink slips through or the extraction directory already contains one.
There was a problem hiding this comment.
Pull request overview
Hardens the preview Cloud Hypervisor runtime and introduces a compiler warning requiring human security review.
Changes:
- Adds KVM device and ACL validation.
- Adds archive and extracted-file safety checks.
- Adds compiler warning and targeted tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_validators.go |
Emits the security-review warning. |
pkg/workflow/compiler_validators_test.go |
Tests warning output and count. |
pkg/workflow/cloud_hypervisor_test.go |
Updates shell-content assertions. |
actions/setup/sh/cloud_hypervisor_setup_bundle.sh |
Hardens archive extraction. |
actions/setup/sh/cloud_hypervisor_kvm_access.sh |
Strengthens KVM ACL checks. |
actions/setup/sh/cloud_hypervisor_host_preflight.sh |
Requires KVM to be a character device. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| exit 1 | ||
| fi | ||
|
|
||
| acl_output="$(getfacl -cp /dev/kvm || true)" |
There was a problem hiding this comment.
net: -2 lines possible.
Generated by ✂️ Ponytail Reviewer for #52757 · auto · 21.2 AIC · ⌖ 3.22 AIC · ⊞ 7.2K
Comment /ponytail to run again
| exit 1 | ||
| fi | ||
| archive_table="$(tar -tvzf "${archive_path}")" | ||
| if awk '$1 ~ /^[lh]/ { found=1; exit 0 } END { exit (found ? 0 : 1) }' <<<"${archive_table}"; then |
There was a problem hiding this comment.
L47: shrink: awk one-liner to detect symlink/hardlink tar entries. grep -Eq '^[lh]' on the same table, 1 line.
| echo "::error::invalid extracted cloud-hypervisor bundle file: ${file}" | ||
| exit 1 | ||
| fi | ||
| case "${file}" in |
There was a problem hiding this comment.
L120-126: shrink: case/esac to check directory prefix. [[ "${file}" != "${extract_dir}"/* ]] && { echo ...; exit 1; }, no esac needed.
Test Quality Sentinel Report 🧪Summary
Test Functions AnalyzedNew Test: TestEmitGeneralToolWarningsCloudHypervisorReviewTrigger (compiler_validators_test.go)
What it tests: Assertions: 5 total
Strength: Properly tests a behavioral contract (warning emission) with appropriate setup/cleanup and multiple assertion points validating the complete warning message. Note: No edge cases tested (e.g., when runtime is NOT cloud-hypervisor). Test inflation ratio is 5.8:1 (35 added lines vs 6 production lines), but justified given the complexity of stderr capture and cleanup. Test Inflation Analysis
Violations✅ None detected. All tests include:
Recommendation✅ APPROVE — Test validates critical security requirement (human-review warning for privileged cloud-hypervisor path). Despite edge-case coverage gaps and high test inflation, the behavioral contract is clear and the design invariant is important. Passing threshold: Design tests 100% (✓), Implementation tests 0% (✓ under 30%), no violations (✓)
|
There was a problem hiding this comment.
Review Summary
This PR adds solid layered security hardening across the cloud-hypervisor privileged runtime path. The changes are well-targeted and the test coverage is appropriate.
Strengths:
- Character-device guard (
test -c /dev/kvm) prevents symlink/bind-mount substitution attacks. - UID numeric validation before
setfaclprevents injection via a crafted$USER/idoutput. - ACL post-verification with
getfacl -cpand an exact regex closes the "grant succeeded but had no effect" gap. - Archive path traversal scan and symlink/hardlink rejection before extraction are solid supply-chain guards.
tar --no-same-owner --no-same-permissionslimits privilege-escalation via crafted archive metadata.validate_extracted_filedefends against TOCTOU and out-of-tree path injection post-extraction.- Compiler warning for cloud-hypervisor runtime ensures human review before merge/rollout.
One blocking issue: extract_dir is not canonicalized before the path-containment case glob, while file paths are canonicalized via realpath. See inline comment on line 121.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.5 AIC · ⌖ 7.17 AIC · ⊞ 5.6K
| exit 1 | ||
| fi | ||
| case "${file}" in | ||
| "${extract_dir}"/*) ;; |
There was a problem hiding this comment.
The path-containment check "${extract_dir}"/* compares a raw (un-canonicalized) extract_dir against realpath-canonicalized file paths. If bundle_root contains symlinks or .. segments, the glob pattern won't match the canonicalized path, and the containment guard can be bypassed.
Suggest canonicalizing extract_dir at declaration time:
extract_dir="$(realpath -m "${bundle_root}/bundle")"@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — requesting changes on correctness and security gaps.
📋 Key Themes & Highlights
Issues Found
- Double-close pipe race in the new
TestEmitGeneralToolWarningsCloudHypervisorReviewTriggertest —wis closed by botht.Cleanupand an explicit call, inconsistent with the fix applied to the adjacent test in this same diff. - Path-traversal bypass in
validate_extracted_file— thecase "${extract_dir}"/*pattern does not canonicalize the path and can be bypassed by entries like"${extract_dir}/../outside". Userealpathto compare resolved paths. - Inverted
awkexit-code logic for symlink detection is non-obvious, has a double-negation, and would silently pass an emptyarchive_table. Agrep -Eq '^[lh]'replacement is clearer and safer. - ACL regex
rw-?acceptsrw— the optional dash is harmless today but imprecise;rw-exactly matches the expected three-character mask. exit 1insidevalidate_extracted_file— worth a comment if intentional, or convert toreturn 1for reusability.
Positive Highlights
- ✅ Defense-in-depth: pre-extraction + post-extraction validation are both present
- ✅
--no-same-owner --no-same-permissionsontarextraction is correct and important - ✅ Compiler-time human-review warning is a good governance signal for a high-blast-radius preview feature
- ✅ Numeric UID sanity check prevents ACL injection via unexpected
id -uoutput
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 50.9 AIC · ⌖ 8.91 AIC · ⊞ 7.3K
Comment /matt to run again
|
|
||
| compiler.emitGeneralToolWarnings(workflowData, "test.md") | ||
|
|
||
| require.NoError(t, w.Close()) |
There was a problem hiding this comment.
[/diagnosing-bugs] w is closed twice — once in t.Cleanup (line 236) and again here — which can race or double-close the pipe's write end, causing an unpredictable test panic.
💡 Fix
Remove the explicit w.Close() at line 242 and let t.Cleanup own the pipe lifecycle. Read the pipe output inside t.Cleanup after closing w:
var buf bytes.Buffer
t.Cleanup(func() {
_ = w.Close()
_, _ = io.Copy(&buf, r)
_ = r.Close()
os.Stderr = oldStderr
})
compiler.emitGeneralToolWarnings(workflowData, "test.md")
// assertions moved here, but buf must be populated in Cleanup aboveThis mirrors the pattern the adjacent test already uses after its own cleanup fix in this diff.
@copilot please address this.
| exit 1 | ||
| fi | ||
| case "${file}" in | ||
| "${extract_dir}"/*) ;; |
There was a problem hiding this comment.
[/diagnosing-bugs] The case pattern "${extract_dir}"/* does not reject paths like "${extract_dir}/../outside" — a resolved path can still escape the extraction root if extract_dir itself contains .. components or if the archive entry is constructed to match the prefix literally.
💡 Fix
Use realpath --relative-base or compare canonicalized paths to rule out traversal:
validate_extracted_file() {
local file="$1"
if [[ -z "${file}" || ! -f "${file}" || -L "${file}" ]]; then
echo "::error::invalid extracted cloud-hypervisor bundle file: ${file}"
exit 1
fi
local real_file real_extract_dir
real_file="$(realpath "${file}")"
real_extract_dir="$(realpath "${extract_dir}")"
if [[ "${real_file}" != "${real_extract_dir}"/* ]]; then
echo "::error::extracted bundle file is outside expected directory: ${file}"
exit 1
fi
}This eliminates any .. bypass through symlink resolution or path component manipulation.
@copilot please address this.
|
|
||
| validate_extracted_file() { | ||
| local file="$1" | ||
| if [[ -z "${file}" || ! -f "${file}" || -L "${file}" ]]; then |
There was a problem hiding this comment.
[/codebase-design] exit 1 inside validate_extracted_file terminates the entire shell script directly rather than returning a non-zero status to the caller. This is intentional here, but it prevents callers from wrapping the function in conditional logic and makes the function harder to unit-test in isolation.
💡 Suggestion
If the exit-on-failure behaviour is deliberate (consistent with the surrounding script style), add a brief comment explaining that. If reusability is desired, replace exit 1 with return 1 and let callers do validate_extracted_file "$x" || exit 1.
@copilot please address this.
| exit 1 | ||
| fi | ||
| archive_table="$(tar -tvzf "${archive_path}")" | ||
| if awk '$1 ~ /^[lh]/ { found=1; exit 0 } END { exit (found ? 0 : 1) }' <<<"${archive_table}"; then |
There was a problem hiding this comment.
[/diagnosing-bugs] The awk symlink/hardlink check inverts its exit code in a non-obvious way (exit 0 when a link is found, exit 1 when none are found), which the wrapping if then inverts again. This double-negation is easy to misread and may silently pass a malicious archive if awk is not available or the archive_table variable is empty.
💡 Clearer alternative
if echo "${archive_table}" | awk '$1 ~ /^[lh]/ { found=1 } END { exit (found ? 1 : 0) }'; then
: # no links found — OK
else
echo "::error::cloud-hypervisor bundle must not include symbolic or hard links"
exit 1
fiOr more idiomatically:
if echo "${archive_table}" | grep -Eq '^[lh]'; then
echo "::error::cloud-hypervisor bundle must not include symbolic or hard links"
exit 1
fiAlso add a guard for empty archive_table before this block.
@copilot please address this.
| echo "::error::failed to read /dev/kvm ACLs for verification." | ||
| exit 1 | ||
| fi | ||
| if ! grep -Eq "^user:${runner_uid}:rw-?$" <<<"${acl_output}"; then |
There was a problem hiding this comment.
[/diagnosing-bugs] The ACL grep pattern ^user:${runner_uid}:rw-?$ uses an unquoted regex inside [[ ]] which is fine, but the trailing -? makes the dash optional — it would match both rw and rw-. GNU getfacl -cp always emits the three-character mask (rw-), so the optional - is benign. However, a simpler and more explicit pattern would be ^user:${runner_uid}:rw-$ to exactly match the expected ACL format and fail loudly if the format changes.
💡 Suggestion
if ! grep -Eq "^user:${runner_uid}:rw-$" <<<"${acl_output}"; thenThis documents the expected ACL mask precisely and would surface a regression if getfacl output format changes.
@copilot please address this.
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up below, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…nce-security-review Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in |
This change addresses the security review request for the new preview
sandbox.agent.runtime: cloud-hypervisorpath, where privileged KVM access and newer MCP-gateway topology increase blast radius if misconfigured. It tightens runtime guardrails and adds an explicit compiler-time signal for mandatory human security review.KVM access hardening (privileged path)
/dev/kvmas a character device.Bundle extraction and artifact integrity hardening
Governance signal: human review trigger
sandbox.agent.runtime: cloud-hypervisoris selected, instructing explicit human security review before merge/rollout.Targeted test updates
branch refresh requested from run https://github.com/github/gh-aw/actions/runs/31886878354> Generated by 👨🍳 PR Sous Chef · gpt54 · 11.7 AIC · ⌖ 5.9 AIC · ⊞ 8.7K · ◷