From c8f2ed221437d7381609bd1c0e13afd1e65b97b9 Mon Sep 17 00:00:00 2001 From: ErenAri Date: Sun, 28 Jun 2026 13:33:37 +0300 Subject: [PATCH] security: shell-quote interpolated values in guest command strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the GitHub Code Quality (AI) findings on internal/vm/qemu.go: guest command strings were assembled by concatenation, relying on upstream manifest/ profile validation rather than explicit quoting. This adds defense-in-depth so a validation gap can never turn an interpolated value into shell syntax. - Reuse the existing shellQuote helper (was only used by virtme_ng.go) and apply it across the qemu run path: * validator run command — validator/artifact/manifest/functional-plan/out/ log-dir/stderr/exit paths and attach-mode are each single-quoted * mapFixupArgs / progTypeArgs / progVariantArgs / probe-companions operands * guestKernelInstallCmd — kernel release and .deb package URLs - Document shellQuote's escaping contract; add TestShellQuote covering embedded single quotes and shell metacharacters. - Update arg-builder tests to expect the quoted operands. All values here are still validated upstream; this is belt-and-suspenders, not a fix for a known-exploitable path. No behavior change for valid inputs (the shell strips the added quotes). Co-Authored-By: Claude Opus 4.8 --- internal/vm/qemu.go | 49 +++++++++++++++++++++------------------- internal/vm/qemu_test.go | 33 ++++++++++++++++++++++----- internal/vm/virtme_ng.go | 5 ++++ 3 files changed, 58 insertions(+), 29 deletions(-) diff --git a/internal/vm/qemu.go b/internal/vm/qemu.go index 9827cc0..0b8bae7 100644 --- a/internal/vm/qemu.go +++ b/internal/vm/qemu.go @@ -64,14 +64,14 @@ func mapFixupArgs(fixups []MapFixup) string { var b strings.Builder for _, fixup := range fixups { if fixup.MaxEntries != "" { - fmt.Fprintf(&b, " --set-map-max-entries %s=%s", fixup.Name, fixup.MaxEntries) + fmt.Fprintf(&b, " --set-map-max-entries %s", shellQuote(fmt.Sprintf("%s=%s", fixup.Name, fixup.MaxEntries))) } if fixup.InnerRingbufBytes > 0 { - fmt.Fprintf(&b, " --set-map-inner-ringbuf %s=%d", fixup.Name, fixup.InnerRingbufBytes) + fmt.Fprintf(&b, " --set-map-inner-ringbuf %s", shellQuote(fmt.Sprintf("%s=%d", fixup.Name, fixup.InnerRingbufBytes))) } if fixup.InnerMapType != "" { - fmt.Fprintf(&b, " --set-map-inner-map %s=%s:%d:%d:%d", fixup.Name, - fixup.InnerMapType, fixup.InnerKeySize, fixup.InnerValueSize, fixup.InnerMaxEntries) + fmt.Fprintf(&b, " --set-map-inner-map %s", shellQuote(fmt.Sprintf("%s=%s:%d:%d:%d", fixup.Name, + fixup.InnerMapType, fixup.InnerKeySize, fixup.InnerValueSize, fixup.InnerMaxEntries))) } } return b.String() @@ -88,7 +88,7 @@ type ProgTypeOverride struct { func progTypeArgs(overrides []ProgTypeOverride) string { var b strings.Builder for _, ov := range overrides { - fmt.Fprintf(&b, " --set-prog-type %s=%s", ov.Selector, ov.Type) + fmt.Fprintf(&b, " --set-prog-type %s", shellQuote(fmt.Sprintf("%s=%s", ov.Selector, ov.Type))) } return b.String() } @@ -96,17 +96,19 @@ func progTypeArgs(overrides []ProgTypeOverride) string { func progVariantArgs(groups []ProgVariantGroup) string { var b strings.Builder for _, group := range groups { - fmt.Fprintf(&b, " --prog-variants %s=", group.Group) + var operand strings.Builder + fmt.Fprintf(&operand, "%s=", group.Group) for i, variant := range group.Variants { if i > 0 { - b.WriteByte(',') + operand.WriteByte(',') } if variant.TrialProbe { - fmt.Fprintf(&b, "%s:trial", variant.Name) + fmt.Fprintf(&operand, "%s:trial", variant.Name) } else { - fmt.Fprintf(&b, "%s:%d", variant.Name, variant.HelperID) + fmt.Fprintf(&operand, "%s:%d", variant.Name, variant.HelperID) } } + fmt.Fprintf(&b, " --prog-variants %s", shellQuote(operand.String())) } return b.String() } @@ -116,7 +118,7 @@ func progVariantArgs(groups []ProgVariantGroup) string { func validatorTuningArgs(req ExecutionRequest) string { args := mapFixupArgs(req.MapFixups) + progTypeArgs(req.ProgTypes) + progVariantArgs(req.ProgVariants) if len(req.ProbeCompanions) > 0 { - args += " --probe-companions " + strings.Join(req.ProbeCompanions, ",") + args += " --probe-companions " + shellQuote(strings.Join(req.ProbeCompanions, ",")) } return args } @@ -362,7 +364,7 @@ func ExecuteProfile(ctx context.Context, req ExecutionRequest) (result Execution result.InfraError = err.Error() return } - manifestArg = fmt.Sprintf(" --manifest %s", manifestRemotePath) + manifestArg = fmt.Sprintf(" --manifest %s", shellQuote(manifestRemotePath)) } functionalPlanArg := "" @@ -372,7 +374,7 @@ func ExecuteProfile(ctx context.Context, req ExecutionRequest) (result Execution result.InfraError = err.Error() return } - functionalPlanArg = fmt.Sprintf(" --functional-plan %s", functionalPlanRemotePath) + functionalPlanArg = fmt.Sprintf(" --functional-plan %s", shellQuote(functionalPlanRemotePath)) } remoteResultPath := filepath.ToSlash(filepath.Join(remoteRoot, "out", "result.json")) @@ -383,17 +385,18 @@ func ExecuteProfile(ctx context.Context, req ExecutionRequest) (result Execution if attachMode == "" { attachMode = "best-effort" } - runCmd := fmt.Sprintf("sudo %s --artifact %s%s%s%s --attach-mode %s --out %s --log-dir %s/out 2>%s; code=$?; echo \"$code\" > %s; exit 0", - validatorRemotePath, - artifactRemotePath, + remoteLogDir := filepath.ToSlash(filepath.Join(remoteRoot, "out")) + runCmd := fmt.Sprintf("sudo %s --artifact %s%s%s%s --attach-mode %s --out %s --log-dir %s 2>%s; code=$?; echo \"$code\" > %s; exit 0", + shellQuote(validatorRemotePath), + shellQuote(artifactRemotePath), manifestArg, functionalPlanArg, validatorTuningArgs(req), - attachMode, - remoteResultPath, - remoteRoot, - remoteErrPath, - remoteExitPath, + shellQuote(attachMode), + shellQuote(remoteResultPath), + shellQuote(remoteLogDir), + shellQuote(remoteErrPath), + shellQuote(remoteExitPath), ) if err := sshRun(ctx, target, runCmd); err != nil { result.InfraError = fmt.Sprintf("run validator: %v", err) @@ -480,16 +483,16 @@ func guestKernelInstallCmd(release string, packageURLs []string) string { if len(packageURLs) > 0 { b.WriteString("mkdir -p /tmp/bpfcompat-kernel; cd /tmp/bpfcompat-kernel; ") for i, pkg := range packageURLs { - fmt.Fprintf(&b, "curl -fsSL --retry 2 -o pkg%02d.deb '%s'; ", i, pkg) + fmt.Fprintf(&b, "curl -fsSL --retry 2 -o pkg%02d.deb %s; ", i, shellQuote(pkg)) } b.WriteString("sudo -E dpkg -i pkg*.deb; ") } else { b.WriteString("sudo -E apt-get -o DPkg::Lock::Timeout=180 -q update; ") - fmt.Fprintf(&b, "sudo -E apt-get -o DPkg::Lock::Timeout=180 -q -y --no-install-recommends install linux-image-%s; ", release) + fmt.Fprintf(&b, "sudo -E apt-get -o DPkg::Lock::Timeout=180 -q -y --no-install-recommends install %s; ", shellQuote("linux-image-"+release)) } b.WriteString("sudo sed -i 's/^GRUB_DEFAULT=.*/GRUB_DEFAULT=saved/' /etc/default/grub; ") b.WriteString("sudo update-grub; ") - fmt.Fprintf(&b, "sudo grub-set-default 'Advanced options for Ubuntu>Ubuntu, with Linux %s'", release) + fmt.Fprintf(&b, "sudo grub-set-default %s", shellQuote("Advanced options for Ubuntu>Ubuntu, with Linux "+release)) return b.String() } diff --git a/internal/vm/qemu_test.go b/internal/vm/qemu_test.go index 09ae89a..764d7bb 100644 --- a/internal/vm/qemu_test.go +++ b/internal/vm/qemu_test.go @@ -422,9 +422,9 @@ func TestMapFixupArgs(t *testing.T) { {Name: "auxiliary_maps", MaxEntries: "cpus"}, {Name: "ringbuf_maps", MaxEntries: "16", InnerRingbufBytes: 8388608}, } - want := " --set-map-max-entries auxiliary_maps=cpus" + - " --set-map-max-entries ringbuf_maps=16" + - " --set-map-inner-ringbuf ringbuf_maps=8388608" + want := " --set-map-max-entries 'auxiliary_maps=cpus'" + + " --set-map-max-entries 'ringbuf_maps=16'" + + " --set-map-inner-ringbuf 'ringbuf_maps=8388608'" if got := mapFixupArgs(fixups); got != want { t.Fatalf("unexpected args:\n got %q\nwant %q", got, want) } @@ -434,7 +434,7 @@ func TestMapFixupArgsInnerMap(t *testing.T) { fixups := []MapFixup{ {Name: "kubearmor_visibility", InnerMapType: "hash", InnerKeySize: 4, InnerValueSize: 4, InnerMaxEntries: 64}, } - want := " --set-map-inner-map kubearmor_visibility=hash:4:4:64" + want := " --set-map-inner-map 'kubearmor_visibility=hash:4:4:64'" if got := mapFixupArgs(fixups); got != want { t.Fatalf("unexpected inner-map args:\n got %q\nwant %q", got, want) } @@ -448,7 +448,7 @@ func TestProgVariantArgs(t *testing.T) { {Name: "recvmmsg_old_x"}, }, }} - want := " --prog-variants recvmmsg_x=recvmmsg_x:181,recvmmsg_old_x:0" + want := " --prog-variants 'recvmmsg_x=recvmmsg_x:181,recvmmsg_old_x:0'" if got := progVariantArgs(groups); got != want { t.Fatalf("unexpected args:\n got %q\nwant %q", got, want) } @@ -481,8 +481,29 @@ func TestProgTypeArgs(t *testing.T) { {Selector: "socket1", Type: "socket_filter"}, {Selector: "ig_trace_dns", Type: "socket_filter"}, }) - want := " --set-prog-type socket1=socket_filter --set-prog-type ig_trace_dns=socket_filter" + want := " --set-prog-type 'socket1=socket_filter' --set-prog-type 'ig_trace_dns=socket_filter'" if got != want { t.Fatalf("unexpected prog-type args:\n got %q\nwant %q", got, want) } } + +func TestShellQuote(t *testing.T) { + cases := []struct { + in string + want string + }{ + {"plain", "'plain'"}, + {"with space", "'with space'"}, + {"name=value:1:2:3", "'name=value:1:2:3'"}, + // An embedded single quote must be broken out and re-quoted so the + // value cannot terminate the surrounding quoting and inject syntax. + {"a'b", `'a'"'"'b'`}, + {"$(rm -rf /)", "'$(rm -rf /)'"}, + {"; reboot #", "'; reboot #'"}, + } + for _, tc := range cases { + if got := shellQuote(tc.in); got != tc.want { + t.Fatalf("shellQuote(%q) = %q, want %q", tc.in, got, tc.want) + } + } +} diff --git a/internal/vm/virtme_ng.go b/internal/vm/virtme_ng.go index d683073..f589cd9 100644 --- a/internal/vm/virtme_ng.go +++ b/internal/vm/virtme_ng.go @@ -240,6 +240,11 @@ func shellSafeWord(value string) string { return value } +// shellQuote wraps value in POSIX single quotes, escaping any embedded single +// quote, so it survives intact as a single argument when an assembled command +// string is run by a remote shell (sshRun) or the in-guest validator. Manifest +// and profile values are validated upstream; quoting here is defense-in-depth +// so a validation gap can never turn an interpolated value into shell syntax. func shellQuote(value string) string { return "'" + strings.ReplaceAll(value, "'", "'\"'\"'") + "'" }