[release-branch.go1.19] 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 #56328
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 <dneil@google.com>
Reviewed-by: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Roland Shoemaker <bracewell@google.com>
TryBot-Result: Security TryBots <security-trybots@go-security-trybots.iam.gserviceaccount.com>
(cherry picked from commit 845accdebb2772c5344ed0c96df9910f3b02d741)
Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/1617553
Run-TryBot: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/446879
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

4 files changed, 52 insertions, 15 deletions

diff --git a/src/os/exec/env_test.go b/src/os/exec/env_test.go
index 112f1e654a..126ec83ced 100644
--- a/src/os/exec/env_test.go
+++ b/src/os/exec/env_test.go
@@ -11,9 +11,10 @@ import (
 
 func TestDedupEnv(t *testing.T) {
 	tests := []struct {
-		noCase bool
-		in     []string
-		want   []string
+		noCase  bool
+		in      []string
+		want    []string
+		wantErr bool
 	}{
 		{
 			noCase: true,
@@ -41,11 +42,17 @@ func TestDedupEnv(t *testing.T) {
 			in:   []string{"dodgy", "entries"},
 			want: []string{"dodgy", "entries"},
 		},
+		{
+			// Filter out entries containing NULs.
+			in:      []string{"A=a\x00b", "B=b", "C\x00C=c"},
+			want:    []string{"B=b"},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
-		got := dedupEnvCase(tt.noCase, tt.in)
-		if !reflect.DeepEqual(got, tt.want) {
-			t.Errorf("Dedup(%v, %q) = %q; want %q", tt.noCase, tt.in, got, tt.want)
+		got, err := dedupEnvCase(tt.noCase, tt.in)
+		if !reflect.DeepEqual(got, tt.want) || (err != nil) != tt.wantErr {
+			t.Errorf("Dedup(%v, %q) = %q, %v; want %q, error:%v", tt.noCase, tt.in, got, err, tt.want, tt.wantErr)
 		}
 	}
 }
diff --git a/src/os/exec/exec.go b/src/os/exec/exec.go
index 737aaab6a7..d3c4e7ae83 100644
--- a/src/os/exec/exec.go
+++ b/src/os/exec/exec.go
@@ -912,7 +912,11 @@ func (c *Cmd) environ() ([]string, error) {
 		}
 	}
 
-	return addCriticalEnv(dedupEnv(env)), err
+	env, dedupErr := dedupEnv(env)
+	if err == nil {
+		err = dedupErr
+	}
+	return addCriticalEnv(env), err
 }
 
 // Environ returns a copy of the environment in which the command would be run
@@ -926,20 +930,27 @@ func (c *Cmd) Environ() []string {
 // 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) {
 	// Construct the output in reverse order, to preserve the
 	// last occurrence of each key.
+	var err error
 	out := make([]string, 0, len(env))
 	saw := make(map[string]bool, len(env))
 	for n := len(env); n > 0; n-- {
 		kv := env[n-1]
 
+		if strings.IndexByte(kv, 0) != -1 {
+			err = errors.New("exec: environment variable contains NUL")
+			continue
+		}
 		i := strings.Index(kv, "=")
 		if i == 0 {
 			// We observe in practice keys with a single leading "=" on Windows.
@@ -974,7 +985,7 @@ func dedupEnvCase(caseInsensitive bool, env []string) []string {
 		out[i], out[j] = out[j], out[i]
 	}
 
-	return out
+	return out, err
 }
 
 // addCriticalEnv adds any critical environment variables that are required
diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 8f79b19eb6..d377f85934 100644
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -1053,6 +1053,15 @@ func TestDedupEnvEcho(t *testing.T) {
 	}
 }
 
+func TestEnvNULCharacter(t *testing.T) {
+	cmd := helperCommand(t, "echoenv", "FOO", "BAR")
+	cmd.Env = append(cmd.Environ(), "FOO=foo\x00BAR=bar")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Errorf("output = %q; want error", string(out))
+	}
+}
+
 func TestString(t *testing.T) {
 	echoPath, err := exec.LookPath("echo")
 	if err != nil {
diff --git a/src/syscall/exec_windows.go b/src/syscall/exec_windows.go
index 92464e089c..45295dedff 100644
--- a/src/syscall/exec_windows.go
+++ b/src/syscall/exec_windows.go
@@ -7,6 +7,7 @@
 package syscall
 
 import (
+	"internal/bytealg"
 	"runtime"
 	"sync"
 	"unicode/utf16"
@@ -115,12 +116,16 @@ func makeCmdLine(args []string) string {
 // the representation required by CreateProcess: a sequence of NUL
 // terminated strings followed by a nil.
 // Last bytes are two UCS-2 NULs, or four NUL bytes.
-func createEnvBlock(envv []string) *uint16 {
+// If any string contains a NUL, it returns (nil, EINVAL).
+func createEnvBlock(envv []string) (*uint16, error) {
 	if len(envv) == 0 {
-		return &utf16.Encode([]rune("\x00\x00"))[0]
+		return &utf16.Encode([]rune("\x00\x00"))[0], nil
 	}
 	length := 0
 	for _, s := range envv {
+		if bytealg.IndexByteString(s, 0) != -1 {
+			return nil, EINVAL
+		}
 		length += len(s) + 1
 	}
 	length += 1
@@ -135,7 +140,7 @@ func createEnvBlock(envv []string) *uint16 {
 	}
 	copy(b[i:i+1], []byte{0})
 
-	return &utf16.Encode([]rune(string(b)))[0]
+	return &utf16.Encode([]rune(string(b)))[0], nil
 }
 
 func CloseOnExec(fd Handle) {
@@ -400,12 +405,17 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle
 		}
 	}
 
+	envBlock, err := createEnvBlock(attr.Env)
+	if err != nil {
+		return 0, 0, err
+	}
+
 	pi := new(ProcessInformation)
 	flags := sys.CreationFlags | CREATE_UNICODE_ENVIRONMENT | _EXTENDED_STARTUPINFO_PRESENT
 	if sys.Token != 0 {
-		err = CreateProcessAsUser(sys.Token, argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, createEnvBlock(attr.Env), dirp, &si.StartupInfo, pi)
+		err = CreateProcessAsUser(sys.Token, argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, envBlock, dirp, &si.StartupInfo, pi)
 	} else {
-		err = CreateProcess(argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, createEnvBlock(attr.Env), dirp, &si.StartupInfo, pi)
+		err = CreateProcess(argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, willInheritHandles, flags, envBlock, dirp, &si.StartupInfo, pi)
 	}
 	if err != nil {
 		return 0, 0, err