Skip to content

Harden cloud-hypervisor privileged runtime path and add explicit human-review warning #52757

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

Merged
pelikhan merged 6 commits into from
Aug 15, 2026
Merged

Harden cloud-hypervisor privileged runtime path and add explicit human-review warning #52757

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7b3bdb
Initial plan
Copilot Aug 14, 2026
3f5eeec
Harden cloud-hypervisor preview security checks
Copilot Aug 14, 2026
8daa546
Address review follow-ups for cloud-hypervisor hardening
Copilot Aug 14, 2026
d98767c
Merge branch 'main' into copilot/uk-ai-resilience-security-review
github-actions[bot] Aug 15, 2026
f35c15c
Address cloud-hypervisor review follow-ups
Copilot Aug 15, 2026
2fc078d
Merge remote-tracking branch 'origin/main' into copilot/uk-ai-resilie…
Copilot Aug 15, 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 actions/setup/sh/cloud_hypervisor_host_preflight.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,10 @@ if ! test -e /dev/kvm; then
exit 1
fi

if ! test -c /dev/kvm; then
echo "::error::/dev/kvm must be a character device."
exit 1
fi

echo "runner is eligible for cloud-hypervisor preview"
echo "::endgroup::"
19 changes: 19 additions & 0 deletions actions/setup/sh/cloud_hypervisor_kvm_access.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,37 @@ if [[ ! -e /dev/kvm ]]; then
exit 1
fi

if [[ ! -c /dev/kvm ]]; then
echo "::error::/dev/kvm must be a character device."
exit 1
fi

if ! command -v setfacl >/dev/null 2>&1; then
echo "::error::setfacl is required to grant scoped access to /dev/kvm."
exit 1
fi

runner_uid="$(id -u)"
if [[ ! "${runner_uid}" =~ ^[0-9]+$ ]]; then
echo "::error::failed to resolve a numeric runner UID."
exit 1
fi
sudo setfacl -m "u:${runner_uid}:rw" /dev/kvm

if [[ ! -r /dev/kvm || ! -w /dev/kvm ]]; then
echo "::error::failed to grant the runner user read/write access to /dev/kvm."
exit 1
fi

acl_output="$(getfacl -cp /dev/kvm || true)"
if [[ -z "${acl_output}" ]]; then
echo "::error::failed to read /dev/kvm ACLs for verification."
exit 1
fi
if ! grep -Eq "^user:${runner_uid}:rw-?$" <<<"${acl_output}"; then

@github-actions github-actions Bot Aug 15, 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 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
fi

or, even better, query the permissions with a tool/output format that is not sensitive to cosmetic formatting differences.

@github-actions github-actions Bot Aug 15, 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] 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}"; then

This documents the expected ACL mask precisely and would surface a regression if getfacl output format changes.

@copilot please address this.

echo "::error::failed to verify scoped ACL entry for the runner user on /dev/kvm."
exit 1
fi

echo "runner user has scoped read/write access to /dev/kvm"
echo "::endgroup::"
37 changes: 36 additions & 1 deletion actions/setup/sh/cloud_hypervisor_setup_bundle.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,22 @@ curl -fsSL -o "${bundle_root}/${manifest_name}" "${asset_base_url}/${manifest_na
echo "downloaded release assets"
echo "::endgroup::"

echo "::group::Validate cloud-hypervisor bundle archive structure"
archive_path="${bundle_root}/${asset_name}"
if tar -tzf "${archive_path}" | grep -Eq '(^/|(^|/)\.\.(/|$))'; then
Comment thread
Copilot marked this conversation as resolved.
Outdated
echo "::error::cloud-hypervisor bundle contains unsafe archive paths"
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

@github-actions github-actions Bot Aug 15, 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.

L47: shrink: awk one-liner to detect symlink/hardlink tar entries. grep -Eq '^[lh]' on the same table, 1 line.

@github-actions github-actions Bot Aug 15, 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] 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
fi

Or more idiomatically:

if echo "${archive_table}" | grep -Eq '^[lh]'; then
  echo "::error::cloud-hypervisor bundle must not include symbolic or hard links"
  exit 1
fi

Also add a guard for empty archive_table before this block.

@copilot please address this.

echo "::error::cloud-hypervisor bundle must not include symbolic or hard links"
exit 1
fi
echo "archive structure validated"
echo "::endgroup::"

echo "::group::Extract cloud-hypervisor bundle"
tar -xzf "${bundle_root}/${asset_name}" -C "${extract_dir}"
tar --no-same-owner --no-same-permissions -xzf "${archive_path}" -C "${extract_dir}"
echo "bundle extracted to ${extract_dir}"
echo "::endgroup::"

Expand Down Expand Up @@ -97,6 +111,21 @@ verify_sha256() {
fi
}

validate_extracted_file() {
local file="$1"
if [[ -z "${file}" || ! -f "${file}" || -L "${file}" ]]; then

@github-actions github-actions Bot Aug 15, 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.

[/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.

echo "::error::invalid extracted cloud-hypervisor bundle file: ${file}"
exit 1
fi
case "${file}" in

@github-actions github-actions Bot Aug 15, 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 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.

@github-actions github-actions Bot Aug 15, 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.

L120-126: shrink: case/esac to check directory prefix. [[ "${file}" != "${extract_dir}"/* ]] && { echo ...; exit 1; }, no esac needed.

"${extract_dir}"/*) ;;

@github-actions github-actions Bot Aug 15, 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.

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.

@github-actions github-actions Bot Aug 15, 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] 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.

*)
echo "::error::extracted bundle file is outside expected directory: ${file}"
exit 1
;;
esac
}

# Artifact names are fixed by the gh-aw-firewall cloud-hypervisor release contract.
binary_rel="cloud-hypervisor"
kernel_rel="vmlinux.bin"
Expand All @@ -115,6 +144,12 @@ if [[ -z "${binary_path}" || -z "${kernel_path}" || -z "${rootfs_path}" || -z "$
exit 1
fi

validate_extracted_file "${binary_path}"
validate_extracted_file "${kernel_path}"
validate_extracted_file "${rootfs_path}"
validate_extracted_file "${supervisor_path}"
validate_extracted_file "${virtiofsd_path}"

if [[ "$(dirname "${binary_path}")" != "$(dirname "${virtiofsd_path}")" ]]; then
echo "::error::virtiofsd must be colocated with the cloud-hypervisor binary"
exit 1
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/cloud_hypervisor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,15 +350,15 @@ func TestCloudHypervisorShellScriptContent(t *testing.T) {
}{
{
script: "cloud_hypervisor_kvm_access.sh",
contains: []string{"RUNNER_ENVIRONMENT", "github-hosted", "ImageOS", "setfacl", "u:${runner_uid}:rw", "/dev/kvm", "-r /dev/kvm", "-w /dev/kvm"},
contains: []string{"RUNNER_ENVIRONMENT", "github-hosted", "ImageOS", "setfacl", "u:${runner_uid}:rw", "/dev/kvm", "-c /dev/kvm", "getfacl -cp /dev/kvm", "-r /dev/kvm", "-w /dev/kvm"},
},
{
script: "cloud_hypervisor_host_preflight.sh",
contains: []string{"RUNNER_ENVIRONMENT", "github-hosted", "ImageOS", "/dev/kvm", "cloud-hypervisor preview"},
contains: []string{"RUNNER_ENVIRONMENT", "github-hosted", "ImageOS", "/dev/kvm", "test -c /dev/kvm", "cloud-hypervisor preview"},
},
{
script: "cloud_hypervisor_setup_bundle.sh",
contains: []string{"cloud-hypervisor-test-x86_64.tar.gz", "cloud-hypervisor-test-x86_64.SHA256SUMS", "cloud-hypervisor-test-x86_64.manifest.json", "vmlinux.bin", "rootfs.ext4", "awf-supervisor", "virtiofsd", "virtiofsd_path=", "virtiofsd_sha256="},
contains: []string{"cloud-hypervisor-test-x86_64.tar.gz", "cloud-hypervisor-test-x86_64.SHA256SUMS", "cloud-hypervisor-test-x86_64.manifest.json", "archive structure validated", "tar --no-same-owner --no-same-permissions", "validate_extracted_file", "vmlinux.bin", "rootfs.ext4", "awf-supervisor", "virtiofsd", "virtiofsd_path=", "virtiofsd_sha256="},
},
}

Expand Down
6 changes: 6 additions & 0 deletions pkg/workflow/compiler_validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,12 @@ func (c *Compiler) emitGeneralToolWarnings(workflowData *WorkflowData, markdownP
"Unsupported hosts are rejected; gh-aw and AWF do not fall back to docker or gvisor."))
c.IncrementWarningCount()
}
if isCloudHypervisorRuntime(workflowData) {
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning",
"sandbox.agent.runtime: cloud-hypervisor uses a privileged KVM preview path with an attached MCP gateway topology. "+
"Require a human security review before merge or rollout, and record explicit approval in your change process."))
c.IncrementWarningCount()
}
if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.AssignToAgent != nil &&
workflowData.SafeOutputs.GitHubApp != nil && workflowData.SafeOutputs.AssignToAgent.GitHubToken == "" {
fmt.Fprintln(os.Stderr, console.FormatWarningMessageStderr(
Expand Down
36 changes: 35 additions & 1 deletion pkg/workflow/compiler_validators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@ func TestEmitExperimentalFeatureWarningsGHAWDetection(t *testing.T) {
os.Stderr = w
t.Cleanup(func() {
os.Stderr = oldStderr
_ = w.Close()
_ = r.Close()
})

Expand All @@ -218,6 +217,41 @@ func TestEmitExperimentalFeatureWarningsGHAWDetection(t *testing.T) {
}
}

func TestEmitGeneralToolWarningsCloudHypervisorReviewTrigger(t *testing.T) {
compiler := NewCompiler()
workflowData := &WorkflowData{
SandboxConfig: &SandboxConfig{
Agent: &AgentSandboxConfig{
Runtime: AgentRuntimeCloudHypervisor,
},
},
}

oldStderr := os.Stderr
r, w, err := os.Pipe()
require.NoError(t, err)
os.Stderr = w
t.Cleanup(func() {
os.Stderr = oldStderr
_ = w.Close()
_ = r.Close()
})

compiler.emitGeneralToolWarnings(workflowData, "test.md")

require.NoError(t, w.Close())

@github-actions github-actions Bot Aug 15, 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] 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 above

This mirrors the pattern the adjacent test already uses after its own cleanup fix in this diff.

@copilot please address this.

os.Stderr = oldStderr

var buf bytes.Buffer
_, err = io.Copy(&buf, r)
require.NoError(t, err)
stderrOutput := buf.String()

assert.Contains(t, stderrOutput, "sandbox.agent.runtime: cloud-hypervisor uses a privileged KVM preview path")
assert.Contains(t, stderrOutput, "Require a human security review before merge or rollout")
assert.Equal(t, 1, compiler.GetWarningCount())
}

// TestValidatePermissions tests permission parsing and MCP tool constraint validation.
func TestValidatePermissions(t *testing.T) {
tests := []struct {
Expand Down