ff447d8 fix(executor): refuse invalid env names, accept bracketed IPv6 registries, document the accepted CLI-env risk (GO-004-R3, GO-005-R) · ermite-runner

M internal/executor/actions_docker.go +3 -0


@@ -203,6 +203,9 @@ func (j job) runDocker(s stepRun, def actionDef, in inputSet, env *ordered,
203	203		v, _ := runtime.Get(k)
204	204		env.Set(k, v)
205	205		}
	206	+	if err := checkEnvNames(env.Keys()); err != nil {
	207	+	return fail(err)
	208	+	}
206	209		cenv := newOrdered()
207	210		for _, k := range env.Keys() {
208	211		v, _ := env.Get(k)


@@ -203,6 +203,9 @@ func (j job) runDocker(s stepRun, def actionDef, in inputSet, env *ordered,
203	v, _ := runtime.Get(k)	203		v, _ := runtime.Get(k)
204	env.Set(k, v)	204		env.Set(k, v)
205	}	205		}
		206	+	if err := checkEnvNames(env.Keys()); err != nil {
		207	+	return fail(err)
		208	+	}
206	cenv := newOrdered()	209		cenv := newOrdered()
207	for _, k := range env.Keys() {	210		for _, k := range env.Keys() {
208	v, _ := env.Get(k)	211		v, _ := env.Get(k)

M internal/executor/containers_docker.go +22 -0


@@ -27,6 +27,14 @@ func eqOpt(flag, value string) dockerPart {
27	27		return dockerPart{line: flag + `="` + value + `"`, args: []string{flag + "=" + value}}
28	28		}
29	29
	30	+	// Accepted risk (review of the pull request, GO-004-R1/R2): a workflow can set
	31	+	// DOCKER_*, proxy, PATH or SSH_AUTH_SOCK variables for its container, and the
	32	+	// ones listed here travel as NAME=value on the arguments of the docker command,
	33	+	// which the docker CLI reads. The CLI runs in the job's own sandbox, which is
	34	+	// the containment boundary: the job can already run docker with any
	35	+	// environment and read its own process arguments, and the C# runner has the
	36	+	// same exposure. Values still go through the masker in the logs.
	37	+	//
30	38		// cliSensitive are the variables the docker CLI itself reads (its daemon, its
31	39		// configuration, its proxy and TLS settings, the loader): handing one of them
32	40		// to the process of docker as the value of a container variable would change

@@ -71,6 +79,20 @@ func envLookup(env []string, key string) (string, bool) {
71	79		return "", false
72	80		}
73	81
	82	+	// checkEnvNames refuses the names `docker -e NAME` would take for NAME=value:
	83	+	// an empty one, or one holding "=" or NUL. Such a key would let a workflow set
	84	+	// a variable of the docker CLI itself ("HTTP_PROXY=http://x/?y") past the exact
	85	+	// name check of cliSensitive. The runner passes them on and docker decides; a
	86	+	// name like that is never valid in the environment of a process.
	87	+	func checkEnvNames(keys []string) error {
	88	+	for _, k := range keys {
	89	+	if k == "" \|\| strings.ContainsAny(k, "=\x00") {
	90	+	return ghErrorf("Invalid environment variable name '%s': a name cannot be empty or contain '=' or NUL", strings.ReplaceAll(k, "\x00", `\0`))
	91	+	}
	92	+	}
	93	+	return nil
	94	+	}
	95	+
74	96		// envKV is DockerUtil.CreateEscapedOption("-e", key, value), or ("-e", key)
75	97		// for an empty value, as the runner echoes it.
76	98		func (j *job) envKV(key, value string) dockerPart {


@@ -27,6 +27,14 @@ func eqOpt(flag, value string) dockerPart {
27	return dockerPart{line: flag + `="` + value + `"`, args: []string{flag + "=" + value}}	27		return dockerPart{line: flag + `="` + value + `"`, args: []string{flag + "=" + value}}
28	}	28		}
29		29
		30	+	// Accepted risk (review of the pull request, GO-004-R1/R2): a workflow can set
		31	+	// DOCKER_*, proxy, PATH or SSH_AUTH_SOCK variables for its container, and the
		32	+	// ones listed here travel as NAME=value on the arguments of the docker command,
		33	+	// which the docker CLI reads. The CLI runs in the job's own sandbox, which is
		34	+	// the containment boundary: the job can already run docker with any
		35	+	// environment and read its own process arguments, and the C# runner has the
		36	+	// same exposure. Values still go through the masker in the logs.
		37	+	//
30	// cliSensitive are the variables the docker CLI itself reads (its daemon, its	38		// cliSensitive are the variables the docker CLI itself reads (its daemon, its
31	// configuration, its proxy and TLS settings, the loader): handing one of them	39		// configuration, its proxy and TLS settings, the loader): handing one of them
32	// to the process of docker as the value of a container variable would change	40		// to the process of docker as the value of a container variable would change

@@ -71,6 +79,20 @@ func envLookup(env []string, key string) (string, bool) {
71	return "", false	79		return "", false
72	}	80		}
73		81
		82	+	// checkEnvNames refuses the names `docker -e NAME` would take for NAME=value:
		83	+	// an empty one, or one holding "=" or NUL. Such a key would let a workflow set
		84	+	// a variable of the docker CLI itself ("HTTP_PROXY=http://x/?y") past the exact
		85	+	// name check of cliSensitive. The runner passes them on and docker decides; a
		86	+	// name like that is never valid in the environment of a process.
		87	+	func checkEnvNames(keys []string) error {
		88	+	for _, k := range keys {
		89	+	if k == "" \|\| strings.ContainsAny(k, "=\x00") {
		90	+	return ghErrorf("Invalid environment variable name '%s': a name cannot be empty or contain '=' or NUL", strings.ReplaceAll(k, "\x00", `\0`))
		91	+	}
		92	+	}
		93	+	return nil
		94	+	}
		95	+
74	// envKV is DockerUtil.CreateEscapedOption("-e", key, value), or ("-e", key)	96		// envKV is DockerUtil.CreateEscapedOption("-e", key, value), or ("-e", key)
75	// for an empty value, as the runner echoes it.	97		// for an empty value, as the runner echoes it.
76	func (j *job) envKV(key, value string) dockerPart {	98		func (j *job) envKV(key, value string) dockerPart {

M internal/executor/containers_exec.go +5 -1


@@ -101,7 +101,11 @@ func (j job) containerScript(c container, plan ShellPlan, in scriptIn, s *step
101	101		if len(sc.Argv) > 1 {
102	102		rest = sc.Argv[1:]
103	103		}
104		-	a, penv := j.execArgv(c, wd, j.handlerEnv(s.env), plan.CommandPath, rest)
	104	+	henv := j.handlerEnv(s.env)
	105	+	if err := checkEnvNames(henv.Keys()); err != nil {
	106	+	return nil, err
	107	+	}
	108	+	a, penv := j.execArgv(c, wd, henv, plan.CommandPath, rest)
105	109		s.rawEnvList = penv
106	110		return a, nil
107	111		}

 	if len(sc.Argv) > 1 {
 		rest = sc.Argv[1:]
 	}
-	a, penv := j.execArgv(c, wd, j.handlerEnv(s.env), plan.CommandPath, rest)
+	henv := j.handlerEnv(s.env)
+	if err := checkEnvNames(henv.Keys()); err != nil {
+		return nil, err
+	}
+	a, penv := j.execArgv(c, wd, henv, plan.CommandPath, rest)
 	s.rawEnvList = penv
 	return a, nil
 }

M internal/executor/containers_node.go +5 -1


@@ -86,7 +86,11 @@ func (j job) runNodeInContainer(s stepRun, c container, script string, env o
86	86		j.mu.Lock()
87	87		wd := j.state.GitHub("workspace")
88	88		j.mu.Unlock()
89		-	argv, penv := j.execArgv(c, c.resolvePath(wd), j.handlerEnv(env), file, []string{c.resolvePath(`"` + script + `"`)})
	89	+	henv := j.handlerEnv(env)
	90	+	if err := checkEnvNames(henv.Keys()); err != nil {
	91	+	return fail(err)
	92	+	}
	93	+	argv, penv := j.execArgv(c, c.resolvePath(wd), henv, file, []string{c.resolvePath(`"` + script + `"`)})
90	94		s.rawEnvList = penv
91	95		return j.runProc(s, file, j.state.Layout().WorkRoot, argv, "Node Action run completed with exit code %d", cancel)
92	96		}

 	j.mu.Lock()
 	wd := j.state.GitHub("workspace")
 	j.mu.Unlock()
-	argv, penv := j.execArgv(c, c.resolvePath(wd), j.handlerEnv(env), file, []string{c.resolvePath(`"` + script + `"`)})
+	henv := j.handlerEnv(env)
+	if err := checkEnvNames(henv.Keys()); err != nil {
+		return fail(err)
+	}
+	argv, penv := j.execArgv(c, c.resolvePath(wd), henv, file, []string{c.resolvePath(`"` + script + `"`)})
 	s.rawEnvList = penv
 	return j.runProc(s, file, j.state.Layout().WorkRoot, argv, "Node Action run completed with exit code %d", cancel)
 }

M internal/executor/containers_spec.go +13 -1


@@ -276,11 +276,20 @@ func containerStrings(v expr.Value, what string) ([]string, error) {
276	276		// that the text is one argument that cannot be taken for an option (the first
277	277		// character is a letter or a digit) and holds no space or control character.
278	278		var imageRefRe = regexp.MustCompile(
279		-	`^(?:(?:[A-Za-z0-9]\|[A-Za-z0-9][A-Za-z0-9-][A-Za-z0-9])(?:\.(?:[A-Za-z0-9]\|[A-Za-z0-9][A-Za-z0-9-][A-Za-z0-9]))*(?::[0-9]+)?/)?` +
	279	+	`^(?:(?:` + domainName + `\|` + ipv6Address + `)(?::[0-9]+)?/)?` +
280	280		`[A-Za-z0-9]+(?:(?:[._]\|__\|-+)[A-Za-z0-9]+)(?:/[A-Za-z0-9]+(?:(?:[._]\|__\|-+)[A-Za-z0-9]+))*` +
281	281		`(?::[A-Za-z0-9_][A-Za-z0-9_.-]{0,127})?` +
282	282		`(?:@[A-Za-z][A-Za-z0-9](?:[-_+.][A-Za-z][A-Za-z0-9])*:[0-9A-Fa-f]{32,})?$`)
283	283
	284	+	// domainName and ipv6Address are the domain grammar of distribution/reference
	285	+	// (reference/regexp.go: domainNameComponent, domainName, ipv6address),
	286	+	// copied here instead of importing the module.
	287	+	const (
	288	+	domainNameComponent = `(?:[A-Za-z0-9]\|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])`
	289	+	domainName = domainNameComponent + `(?:\.` + domainNameComponent + `)*`
	290	+	ipv6Address = `\[(?:[A-Fa-f0-9:]+)\]`
	291	+	)
	292	+
284	293		// validImageRef reports whether image can be passed to docker pull and create.
285	294		func validImageRef(image string) bool { return len(image) <= 1024 && imageRefRe.MatchString(image) }
286	295

@@ -360,6 +369,9 @@ func containerEnv(v expr.Value) ([]Pair, error) {
360	369		}
361	370		var out []Pair
362	371		for _, p := range d.Pairs() {
	372	+	if err := checkEnvNames([]string{p.Key}); err != nil {
	373	+	return nil, err
	374	+	}
363	375		s, err := asContainerString(p.Value, "container env "+p.Key)
364	376		if err != nil {
365	377		return nil, err

 // that the text is one argument that cannot be taken for an option (the first
 // character is a letter or a digit) and holds no space or control character.
 var imageRefRe = regexp.MustCompile(
-	`^(?:(?:[A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])(?:\.(?:[A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9]))*(?::[0-9]+)?/)?` +
+	`^(?:(?:` + domainName + `|` + ipv6Address + `)(?::[0-9]+)?/)?` +
 		`[A-Za-z0-9]+(?:(?:[._]|__|-+)[A-Za-z0-9]+)*(?:/[A-Za-z0-9]+(?:(?:[._]|__|-+)[A-Za-z0-9]+)*)*` +
 		`(?::[A-Za-z0-9_][A-Za-z0-9_.-]{0,127})?` +
 		`(?:@[A-Za-z][A-Za-z0-9]*(?:[-_+.][A-Za-z][A-Za-z0-9]*)*:[0-9A-Fa-f]{32,})?$`)
+// domainName and ipv6Address are the domain grammar of distribution/reference
+// (reference/regexp.go: domainNameComponent, domainName, ipv6address),
+// copied here instead of importing the module.
+const (
+	domainNameComponent = `(?:[A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])`
+	domainName          = domainNameComponent + `(?:\.` + domainNameComponent + `)*`
+	ipv6Address         = `\[(?:[A-Fa-f0-9:]+)\]`
+)
 // validImageRef reports whether image can be passed to docker pull and create.
 func validImageRef(image string) bool { return len(image) <= 1024 && imageRefRe.MatchString(image) }
 	}
 	var out []Pair
 	for _, p := range d.Pairs() {
+		if err := checkEnvNames([]string{p.Key}); err != nil {
+			return nil, err
+		}
 		s, err := asContainerString(p.Value, "container env "+p.Key)
 		if err != nil {
 			return nil, err

M internal/executor/containers_spec_test.go +3 -2


@@ -100,8 +100,9 @@ func TestConvertContainerForms(t *testing.T) {
100	100
101	101		func TestImageReferenceGrammar(t *testing.T) {
102	102		good := []string{"ubuntu", "ubuntu:24.04", "ghcr.io/o/img:1", "localhost:5000/a/b:tag", "a/b_c.d-e", "redis@sha256:" + strings.Repeat("a", 64),
103		-	"r.io:443/x/y:1@sha256:" + strings.Repeat("0", 64), "Ubuntu:24.04"}
104		-	bad := []string{"", "--help", "-x", "-", " a", "a b", "a\nb", "a;b", "a$(x)", "a:", "a:-x", "a@sha256:zz", "/a", "a//b", "a:b:c", strings.Repeat("a", 1100)}
	103	+	"r.io:443/x/y:1@sha256:" + strings.Repeat("0", 64), "Ubuntu:24.04",
	104	+	"[::1]:5000/x", "[::1]/x/y:tag", "[2001:db8::1]:443/a/b@sha256:" + strings.Repeat("a", 64)}
	105	+	bad := []string{"", "--help", "-x", "-", " a", "a b", "a\nb", "a;b", "a$(x)", "a:", "a:-x", "a@sha256:zz", "/a", "a//b", "a:b:c", "[::1/x", "[]/x", "[zz]/x", "[::1]x", "[::1]:/x", strings.Repeat("a", 1100)}
105	106		for _, g := range good {
106	107		if !validImageRef(g) {
107	108		t.Errorf("rejected %q", g)

 func TestImageReferenceGrammar(t *testing.T) {
 	good := []string{"ubuntu", "ubuntu:24.04", "ghcr.io/o/img:1", "localhost:5000/a/b:tag", "a/b_c.d-e", "redis@sha256:" + strings.Repeat("a", 64),
-		"r.io:443/x/y:1@sha256:" + strings.Repeat("0", 64), "Ubuntu:24.04"}
+		"r.io:443/x/y:1@sha256:" + strings.Repeat("0", 64), "Ubuntu:24.04",
-	bad := []string{"", "--help", "-x", "-", " a", "a b", "a\nb", "a;b", "a$(x)", "a:", "a:-x", "a@sha256:zz", "/a", "a//b", "a:b:c", strings.Repeat("a", 1100)}
+		"[::1]:5000/x", "[::1]/x/y:tag", "[2001:db8::1]:443/a/b@sha256:" + strings.Repeat("a", 64)}
+	bad := []string{"", "--help", "-x", "-", " a", "a b", "a\nb", "a;b", "a$(x)", "a:", "a:-x", "a@sha256:zz", "/a", "a//b", "a:b:c", "[::1/x", "[]/x", "[zz]/x", "[::1]x", "[::1]:/x", strings.Repeat("a", 1100)}
 	for _, g := range good {
 		if !validImageRef(g) {
 			t.Errorf("rejected %q", g)