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 (file)
index 0000000..8badf6f
--- /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 5f75febd27c9bca2613b514010fba9bb4b13d6cd..07eccb23d8ffd70ae97f7ba5e1cb3a1f51329be9 100644 (file)
--- 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 814fe445b9be16f20a144dcf999831fefeca6714..bb45a23643416e517ff99a4060671cac70cf1083 100644 (file)
--- 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 {