From aba57b07721cac39b4b7cf70f8dfbf9e2e299187 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Mon, 17 Oct 2022 17:38:29 -0700 Subject: [release-branch.go1.18] syscall, os/exec: reject environment variables containing NULs Check for and reject environment variables containing NULs. The conventions for passing environment variables to subprocesses cause most or all systems to interpret a NUL as a separator. The syscall package rejects environment variables containing a NUL on most systems, but erroneously did not do so on Windows. This causes an environment variable such as "FOO=a\x00BAR=b" to be interpreted as "FOO=a", "BAR=b". Check for and reject NULs in environment variables passed to syscall.StartProcess on Windows. Add a redundant check to os/exec as extra insurance. Updates #56284 Fixes #56327 Fixes CVE-2022-41716 Change-Id: I2950e2b0cb14ebd26e5629be1521858f66a7d4ae Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1609434 Run-TryBot: Damien Neil Reviewed-by: Tatiana Bradley Reviewed-by: Roland Shoemaker TryBot-Result: Security TryBots (cherry picked from commit 845accdebb2772c5344ed0c96df9910f3b02d741) Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1617552 Run-TryBot: Tatiana Bradley Reviewed-by: Damien Neil Reviewed-on: https://go-review.googlesource.com/c/go/+/446915 Reviewed-by: Heschi Kreinick Run-TryBot: Matthew Dempsky TryBot-Result: Gopher Robot Reviewed-by: Tatiana Bradley --- src/os/exec/exec.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) (limited to 'src/os/exec/exec.go') diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go index ecddee690d..056a7e07d7 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -422,10 +422,14 @@ func (c *Cmd) Start() error { return err } + env, err := dedupEnv(envv) + if err != nil { + return err + } c.Process, err = os.StartProcess(c.Path, c.argv(), &os.ProcAttr{ Dir: c.Dir, Files: c.childFiles, - Env: addCriticalEnv(dedupEnv(envv)), + Env: addCriticalEnv(env), Sys: c.SysProcAttr, }) if err != nil { @@ -741,16 +745,23 @@ func minInt(a, b int) int { // dedupEnv returns a copy of env with any duplicates removed, in favor of // later values. // Items not of the normal environment "key=value" form are preserved unchanged. -func dedupEnv(env []string) []string { +// Items containing NUL characters are removed, and an error is returned along with +// the remaining values. +func dedupEnv(env []string) ([]string, error) { return dedupEnvCase(runtime.GOOS == "windows", env) } // dedupEnvCase is dedupEnv with a case option for testing. // If caseInsensitive is true, the case of keys is ignored. -func dedupEnvCase(caseInsensitive bool, env []string) []string { +func dedupEnvCase(caseInsensitive bool, env []string) ([]string, error) { + var err error out := make([]string, 0, len(env)) saw := make(map[string]int, len(env)) // key => index into out for _, kv := range env { + if strings.IndexByte(kv, 0) != -1 { + err = errors.New("exec: environment variable contains NUL") + continue + } k, _, ok := strings.Cut(kv, "=") if !ok { out = append(out, kv) @@ -766,7 +777,7 @@ func dedupEnvCase(caseInsensitive bool, env []string) []string { saw[k] = len(out) out = append(out, kv) } - return out + return out, err } // addCriticalEnv adds any critical environment variables that are required -- cgit v1.3