src/os/exec/env_test.go | 19 +++++++++++++------ src/os/exec/exec.go | 19 +++++++++++++++---- src/os/exec/exec_test.go | 9 +++++++++ src/syscall/exec_windows.go | 20 +++++++++++++++----- diff --git a/src/os/exec/env_test.go b/src/os/exec/env_test.go index 112f1e654a104bf81dfefc84ae828080b00b1c19..126ec83cedf45c777a27ad17fcb4bae0f9824e9f 100644 --- a/src/os/exec/env_test.go +++ b/src/os/exec/env_test.go @@ -11,9 +11,10 @@ ) 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 @@ // (Maybe filter them out or error out on them at some point.) 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 737aaab6a7dc3a3a5f829ee120a8fbd97c7c3eb8..d3c4e7ae83b56176d0f01fbc06e04125758e2556 100644 --- a/src/os/exec/exec.go +++ b/src/os/exec/exec.go @@ -912,7 +912,11 @@ } } } - 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 @@ // 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 @@ j := len(out) - i - 1 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 8f79b19eb60ac965da26d981fe06c7eb48d435fa..d377f85934053618eec8fad7f1a2d2c780046586 100644 --- a/src/os/exec/exec_test.go +++ b/src/os/exec/exec_test.go @@ -1053,6 +1053,15 @@ t.Errorf("output = %q; want %q", got, want) } } +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 92464e089c5e844b4987bd6add031cb3d1215ee9..45295dedffbe73a964004a1b0123667eff2797c4 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 @@ // createEnvBlock converts an array of environment strings into // 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 @@ i = i + l + 1 } 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 @@ return 0, 0, err } } + 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