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 <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/+/
1617552
Run-TryBot: Tatiana Bradley <tatianabradley@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/446915
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
func TestDedupEnv(t *testing.T) {
tests := []struct {
- noCase bool
- in []string
- want []string
+ noCase bool
+ in []string
+ want []string
+ wantErr bool
}{
{
noCase: true,
in: []string{"=a", "=b", "foo", "bar"},
want: []string{"=b", "foo", "bar"},
},
+ {
+ // 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)
}
}
}
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 {
// 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)
saw[k] = len(out)
out = append(out, kv)
}
- return out
+ return out, err
}
// addCriticalEnv adds any critical environment variables that are required
}
}
+func TestEnvNULCharacter(t *testing.T) {
+ cmd := helperCommand(t, "echoenv", "FOO", "BAR")
+ cmd.Env = append(cmd.Env, "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 {
package syscall
import (
+ "internal/bytealg"
"runtime"
"sync"
"unicode/utf16"
// 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
}
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) {
}
}
+ 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