Fix command injection in gitfed-ctl/gitfed-install's docker-save|import step
Found in a security review: sh -c "docker save '<tag>' | k3s ctr images import -" quoted the tag with shellQuote, a bare wrap in single quotes that doesn't escape an embedded single quote. A VERSION file (or a cloned source's VERSION) containing one could break out and run arbitrary shell commands as root. Both tools now pipe docker save | k3s ctr images import directly in Go (runPiped, no shell involved) instead — nothing left to escape.
7 files changed
+163 −14
M
CHANGELOG.md
+4 −0
M
cmd/gitfed-ctl/exec.go
+39 −0
A
cmd/gitfed-ctl/exec_test.go
+69 −0
M
cmd/gitfed-ctl/update_pipeline.go
+6 −4
M
cmd/gitfed-ctl/update_pipeline_test.go
+0 −6
M
cmd/gitfed-install/actions.go
+2 −4
M
cmd/gitfed-install/exec.go
+43 −0
CHANGELOG.md
@@ -2,6 +2,10 @@
A bullet starting with `**BREAKING:**` flags a change gitfed-ctl's update wizard makes you acknowledge individually before it will let you upgrade past that version.
+## 1.2.15
+
+- Fixed a real command-injection bug found during a security review: `gitfed-ctl` and `gitfed-install` both built `sh -c "docker save '<tag>' | k3s ctr images import -"` with `shellQuote`, a bare wrap in single quotes that doesn't escape an embedded single quote — a `VERSION` file (or a cloned source's VERSION) containing one could break out of the quoting and run arbitrary shell commands as root. Both now pipe `docker save`/`k3s ctr images import` directly in Go (`runPiped`, no shell at all), so there's nothing to escape. Drop-in fix, no action needed.
+
## 1.2.14
- Fixed a real bug found live (Thomas's instance): `gitfed-ctl`'s "current version" came from the checkout's own `VERSION` file, not what's actually deployed. If an earlier run's `git pull` succeeded but a later step (build/import/apply/rollout) failed, the checkout ends up ahead of the live deployment — the next check then read the already-advanced `VERSION` and reported "up to date" even though production was still running the old image, silently skipping the breaking-change wizard for a version that was never actually deployed. "Current version" now comes from `kubectl get deployment gitfed`'s actual image tag, not the checkout.
cmd/gitfed-ctl/exec.go
@@ -81,6 +81,45 @@ func runQuiet(ctx context.Context, dir, name string, args ...string) (string, er
return string(out), err
}
+// runPiped runs first | second without a shell — first's stdout feeds
+// second's stdin directly through an in-process pipe. This replaces a
+// former `sh -c "cmd1 '<arg>' | cmd2"` pattern whose quoting (a bare wrap
+// in single quotes) didn't escape an embedded single quote, letting a
+// crafted argument (e.g. a VERSION file containing one) break out and
+// inject arbitrary shell commands running as root. No shell involved
+// here, so there's nothing to escape.
+func runPiped(ctx context.Context, first, second []string) error {
+ c1 := exec.CommandContext(ctx, first[0], first[1:]...)
+ c2 := exec.CommandContext(ctx, second[0], second[1:]...)
+
+ pr, pw := io.Pipe()
+ c1.Stdout = pw
+ c2.Stdin = pr
+
+ var stderr1, stderr2 bytes.Buffer
+ c1.Stderr = &stderr1
+ c2.Stderr = &stderr2
+
+ if err := c2.Start(); err != nil {
+ return fmt.Errorf("%s: %w", second[0], err)
+ }
+ if err := c1.Start(); err != nil {
+ return fmt.Errorf("%s: %w", first[0], err)
+ }
+
+ err1 := c1.Wait()
+ pw.Close()
+ err2 := c2.Wait()
+
+ if err1 != nil {
+ return fmt.Errorf("%s: %w: %s", first[0], err1, strings.TrimSpace(stderr1.String()))
+ }
+ if err2 != nil {
+ return fmt.Errorf("%s: %w: %s", second[0], err2, strings.TrimSpace(stderr2.String()))
+ }
+ return nil
+}
+
// runToFile runs name(args...) and streams its raw stdout straight into w
// — used for the one step whose output is binary (backup.go's `tar` pull),
// where treating it as line-oriented text like runStreamed would corrupt
cmd/gitfed-ctl/exec_test.go
@@ -0,0 +1,69 @@
+package main
+
+import (
+ "context"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+)
+
+func TestRunPipedConnectsStdoutToStdin(t *testing.T) {
+ err := runPiped(context.Background(),
+ []string{"echo", "-n", "hello"},
+ []string{"cat"},
+ )
+ if err != nil {
+ t.Fatalf("runPiped: %v", err)
+ }
+}
+
+// TestRunPipedNoShellInjection reproduces the real bug: the previous
+// `sh -c "docker save '<tag>' | k3s ctr images import -"` pattern's
+// quoting (shellQuote, a bare wrap in single quotes) didn't escape an
+// embedded single quote, so a tag/version string containing one could
+// break out and run arbitrary shell commands. runPiped never invokes a
+// shell at all, so the same string is inert — it's just a literal
+// argument to `cat`, not code.
+func TestRunPipedNoShellInjection(t *testing.T) {
+ dir := t.TempDir()
+ proof := filepath.Join(dir, "proof")
+ malicious := "gitfed:1.2.10'; touch " + proof + "; echo '"
+
+ err := runPiped(context.Background(),
+ []string{"echo", "-n", malicious},
+ []string{"cat"},
+ )
+ if err != nil {
+ t.Fatalf("runPiped: %v", err)
+ }
+ if _, statErr := os.Stat(proof); statErr == nil {
+ t.Fatal("injection succeeded: a shell interpreted the piped argument")
+ }
+}
+
+func TestRunPipedFirstCommandFails(t *testing.T) {
+ err := runPiped(context.Background(),
+ []string{"sh", "-c", "echo bad-times >&2; exit 3"},
+ []string{"cat"},
+ )
+ if err == nil {
+ t.Fatal("expected an error when the first command exits non-zero")
+ }
+ if !strings.Contains(err.Error(), "bad-times") {
+ t.Errorf("expected the first command's stderr in the error, got: %v", err)
+ }
+}
+
+func TestRunPipedSecondCommandFails(t *testing.T) {
+ err := runPiped(context.Background(),
+ []string{"echo", "hi"},
+ []string{"sh", "-c", "echo nope >&2; exit 3"},
+ )
+ if err == nil {
+ t.Fatal("expected an error when the second command exits non-zero")
+ }
+ if !strings.Contains(err.Error(), "nope") {
+ t.Errorf("expected the second command's stderr in the error, got: %v", err)
+ }
+}
cmd/gitfed-ctl/update_pipeline.go
@@ -26,8 +26,6 @@ func bumpDeploymentImageTag(sourceDir, newVersion string) error {
return os.WriteFile(path, []byte(updated), 0644)
}
-func shellQuote(s string) string { return "'" + s + "'" }
-
// versionResolvedMsg reports the version actually deployed, read from
// VERSION right after the pull — this is what the running/success screens
// display, since the target shown at the "check" screen was read from the
@@ -72,10 +70,14 @@ func runUpdatePipeline(ctx context.Context, ch chan<- tea.Msg, sourceDir string,
return
}
- if err := runStreamed(ctx, ch, "import", "", "sh", "-c",
- "docker save "+shellQuote("gitfed:"+newVersion)+" | k3s ctr images import -"); err != nil {
+ if err := runPiped(ctx,
+ []string{"docker", "save", "gitfed:" + newVersion},
+ []string{"k3s", "ctr", "images", "import", "-"},
+ ); err != nil {
+ ch <- stepResultMsg{stream: "import", err: err}
return
}
+ ch <- stepResultMsg{stream: "import"}
if err := runStreamed(ctx, ch, "apply", "", "kubectl", "-n", "gitfed", "apply",
"-f", sourceDir+"/deploy/k8s/deployment.yaml"); err != nil {
cmd/gitfed-ctl/update_pipeline_test.go
@@ -48,9 +48,3 @@ spec:
t.Errorf("unrelated lines were modified:\n%s", got)
}
}
-
-func TestShellQuote(t *testing.T) {
- if got := shellQuote("gitfed:1.2.10"); got != "'gitfed:1.2.10'" {
- t.Errorf("shellQuote(%q) = %q", "gitfed:1.2.10", got)
- }
-}
cmd/gitfed-install/actions.go
@@ -110,12 +110,10 @@ func runBuildImage(ctx context.Context, ch chan<- tea.Msg, sourceURL, workDir st
return
}
- runStreamed(ctx, ch, "import", "", "sh", "-c",
- "docker save "+shellQuote(tag)+" | sudo k3s ctr images import -")
+ err = runPiped(ctx, []string{"docker", "save", tag}, []string{"sudo", "k3s", "ctr", "images", "import", "-"})
+ ch <- stepResultMsg{stream: "import", err: err}
}
-func shellQuote(s string) string { return "'" + s + "'" }
-
// runDeploy applies the four unmodified manifests plus the two generated
// ones (configmap/ingress — see manifests.go), in the order INSTALL.md
// describes, one apply per message so the UI can show live per-file
cmd/gitfed-install/exec.go
@@ -2,8 +2,12 @@ package main
import (
"bufio"
+ "bytes"
"context"
+ "fmt"
+ "io"
"os/exec"
+ "strings"
tea "github.com/charmbracelet/bubbletea"
)
@@ -87,3 +91,42 @@ func runQuiet(ctx context.Context, dir, name string, args ...string) (string, er
out, err := cmd.CombinedOutput()
return string(out), err
}
+
+// runPiped runs first | second without a shell — first's stdout feeds
+// second's stdin directly through an in-process pipe. This replaces a
+// former `sh -c "cmd1 '<arg>' | cmd2"` pattern whose quoting (a bare wrap
+// in single quotes) didn't escape an embedded single quote, letting a
+// crafted argument (e.g. a VERSION file containing one) break out and
+// inject arbitrary shell commands. No shell involved here, so there's
+// nothing to escape.
+func runPiped(ctx context.Context, first, second []string) error {
+ c1 := exec.CommandContext(ctx, first[0], first[1:]...)
+ c2 := exec.CommandContext(ctx, second[0], second[1:]...)
+
+ pr, pw := io.Pipe()
+ c1.Stdout = pw
+ c2.Stdin = pr
+
+ var stderr1, stderr2 bytes.Buffer
+ c1.Stderr = &stderr1
+ c2.Stderr = &stderr2
+
+ if err := c2.Start(); err != nil {
+ return fmt.Errorf("%s: %w", second[0], err)
+ }
+ if err := c1.Start(); err != nil {
+ return fmt.Errorf("%s: %w", first[0], err)
+ }
+
+ err1 := c1.Wait()
+ pw.Close()
+ err2 := c2.Wait()
+
+ if err1 != nil {
+ return fmt.Errorf("%s: %w: %s", first[0], err1, strings.TrimSpace(stderr1.String()))
+ }
+ if err2 != nil {
+ return fmt.Errorf("%s: %w: %s", second[0], err2, strings.TrimSpace(stderr2.String()))
+ }
+ return nil
+}