[release-branch.go1.19] os/exec: allow NUL in environment variables o…

…n Plan 9

Plan 9 uses NUL as os.PathListSeparator, so it's almost always going
to appear in the environment variable list. Exempt GOOS=plan9 from the
check for NUL in environment variables.

For #56284.
For #56544.
Fixes #56551.

Change-Id: I23df233cdf20c0a9a606fd9253e15a9b5482575a
Reviewed-on: https://go-review.googlesource.com/c/go/+/447715
Reviewed-by: David du Colombier <0intro@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/447799
Run-TryBot: David du Colombier <0intro@gmail.com>

src/os/exec/env_test.go

@@ -12,6 +12,7 @@ import (
 func TestDedupEnv(t *testing.T) {
 	tests := []struct {
 		noCase  bool
+		nulOK   bool
 		in      []string
 		want    []string
 		wantErr bool
@@ -48,9 +49,15 @@ func TestDedupEnv(t *testing.T) {
 			want:    []string{"B=b"},
 			wantErr: true,
 		},
+		{
+			// Plan 9 needs to preserve environment variables with NUL (#56544).
+			nulOK: true,
+			in:    []string{"path=one\x00two"},
+			want:  []string{"path=one\x00two"},
+		},
 	}
 	for _, tt := range tests {
-		got, err := dedupEnvCase(tt.noCase, tt.in)
+		got, err := dedupEnvCase(tt.noCase, tt.nulOK, 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)
 		}

src/os/exec/exec.go

@@ -930,15 +930,16 @@ 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.
-// Items containing NUL characters are removed, and an error is returned along with
-// the remaining values.
+// Except on Plan 9, 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)
+	return dedupEnvCase(runtime.GOOS == "windows", runtime.GOOS == "plan9", 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, error) {
+// If nulOK is false, items containing NUL characters are allowed.
+func dedupEnvCase(caseInsensitive, nulOK bool, env []string) ([]string, error) {
 	// Construct the output in reverse order, to preserve the
 	// last occurrence of each key.
 	var err error
@@ -947,10 +948,13 @@ func dedupEnvCase(caseInsensitive bool, env []string) ([]string, error) {
 	for n := len(env); n > 0; n-- {
 		kv := env[n-1]
 
-		if strings.IndexByte(kv, 0) != -1 {
+		// Reject NUL in environment variables to prevent security issues (#56284);
+		// except on Plan 9, which uses NUL as os.PathListSeparator (#56544).
+		if !nulOK && 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.

src/os/exec/exec_test.go

@@ -1054,6 +1054,9 @@ func TestDedupEnvEcho(t *testing.T) {
 }
 
 func TestEnvNULCharacter(t *testing.T) {
+	if runtime.GOOS == "plan9" {
+		t.Skip("plan9 explicitly allows NUL in the enviroment")
+	}
 	cmd := helperCommand(t, "echoenv", "FOO", "BAR")
 	cmd.Env = append(cmd.Environ(), "FOO=foo\x00BAR=bar")
 	out, err := cmd.CombinedOutput()