From: Patrick Mezard <patrick@mezard.eu>
Date: Sat, 9 May 2015 13:51:45 +0000 (+0200)
Subject: internal/syscall/windows/registry: fix read overrun in GetStringsValue
X-Git-Tag: go1.5beta1~581
X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=335e44d265e7b7741b00237f4fcd97a1b80bfd9a;p=gostls13.git

internal/syscall/windows/registry: fix read overrun in GetStringsValue

According to MSDN RegQueryValueEx page:

  If the data has the REG_SZ, REG_MULTI_SZ or REG_EXPAND_SZ type, the
  string may not have been stored with the proper terminating null
  characters. Therefore, even if the function returns ERROR_SUCCESS, the
  application should ensure that the string is properly terminated before
  using it; otherwise, it may overwrite a buffer. (Note that REG_MULTI_SZ
  strings should have two terminating null characters.)

Test written by Alex Brainman <alex.brainman@gmail.com>

Change-Id: I8c0852e0527e27ceed949134ed5e6de944189986
Reviewed-on: https://go-review.googlesource.com/9806
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
---

diff --git a/src/internal/syscall/windows/registry/export_test.go b/src/internal/syscall/windows/registry/export_test.go
new file mode 100644
index 0000000000..8badf6fdcf
--- /dev/null
+++ b/src/internal/syscall/windows/registry/export_test.go
@@ -0,0 +1,11 @@
+// Copyright 2015 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build windows
+
+package registry
+
+func (k Key) SetValue(name string, valtype uint32, data []byte) error {
+	return k.setValue(name, valtype, data)
+}
diff --git a/src/internal/syscall/windows/registry/registry_test.go b/src/internal/syscall/windows/registry/registry_test.go
index 5f75febd27..07eccb23d8 100644
--- a/src/internal/syscall/windows/registry/registry_test.go
+++ b/src/internal/syscall/windows/registry/registry_test.go
@@ -611,3 +611,68 @@ func TestExpandString(t *testing.T) {
 		t.Errorf("want %q string expanded, got %q", want, got)
 	}
 }
+
+func TestInvalidValues(t *testing.T) {
+	softwareK, err := registry.OpenKey(registry.CURRENT_USER, "Software", registry.QUERY_VALUE)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer softwareK.Close()
+
+	testKName := randKeyName("TestInvalidValues_")
+
+	k, exist, err := registry.CreateKey(softwareK, testKName, registry.CREATE_SUB_KEY|registry.QUERY_VALUE|registry.SET_VALUE)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer k.Close()
+
+	if exist {
+		t.Fatalf("key %q already exists", testKName)
+	}
+
+	defer registry.DeleteKey(softwareK, testKName)
+
+	var tests = []struct {
+		Type uint32
+		Name string
+		Data []byte
+	}{
+		{registry.DWORD, "Dword1", nil},
+		{registry.DWORD, "Dword2", []byte{1, 2, 3}},
+		{registry.QWORD, "Qword1", nil},
+		{registry.QWORD, "Qword2", []byte{1, 2, 3}},
+		{registry.QWORD, "Qword3", []byte{1, 2, 3, 4, 5, 6, 7}},
+		{registry.MULTI_SZ, "MultiString1", nil},
+		{registry.MULTI_SZ, "MultiString2", []byte{0}},
+		{registry.MULTI_SZ, "MultiString3", []byte{'a', 'b', 0}},
+		{registry.MULTI_SZ, "MultiString4", []byte{'a', 0, 0, 'b', 0}},
+		{registry.MULTI_SZ, "MultiString5", []byte{'a', 0, 0}},
+	}
+
+	for _, test := range tests {
+		err := k.SetValue(test.Name, test.Type, test.Data)
+		if err != nil {
+			t.Fatalf("SetValue for %q failed: %v", test.Name, err)
+		}
+	}
+
+	for _, test := range tests {
+		switch test.Type {
+		case registry.DWORD, registry.QWORD:
+			value, valType, err := k.GetIntegerValue(test.Name)
+			if err == nil {
+				t.Errorf("GetIntegerValue(%q) succeeded. Returns type=%d value=%v", test.Name, valType, value)
+			}
+		case registry.MULTI_SZ:
+			value, valType, err := k.GetStringsValue(test.Name)
+			if err == nil {
+				if len(value) != 0 {
+					t.Errorf("GetStringsValue(%q) succeeded. Returns type=%d value=%v", test.Name, valType, value)
+				}
+			}
+		default:
+			t.Errorf("unsupported type %d for %s value", test.Type, test.Name)
+		}
+	}
+}
diff --git a/src/internal/syscall/windows/registry/value.go b/src/internal/syscall/windows/registry/value.go
index 814fe445b9..bb45a23643 100644
--- a/src/internal/syscall/windows/registry/value.go
+++ b/src/internal/syscall/windows/registry/value.go
@@ -150,9 +150,17 @@ func (k Key) GetStringsValue(name string) (val []string, valtype uint32, err err
 	if typ != MULTI_SZ {
 		return nil, typ, ErrUnexpectedType
 	}
-	val = make([]string, 0, 5)
+	if len(data) == 0 {
+		return nil, typ, nil
+	}
 	p := (*[1 << 24]uint16)(unsafe.Pointer(&data[0]))[:len(data)/2]
-	p = p[:len(p)-1] // remove terminating nil
+	if len(p) == 0 {
+		return nil, typ, nil
+	}
+	if p[len(p)-1] == 0 {
+		p = p[:len(p)-1] // remove terminating null
+	}
+	val = make([]string, 0, 5)
 	from := 0
 	for i, c := range p {
 		if c == 0 {