(MODULES-1921) Ruby registry writes corrupt string

 - The Ruby implementation of write is buggy at :
  https://github.com/ruby/ruby/blob/v2_1_6/ext/win32/lib/win32/registry.rb#L727-L748

  When writing a REG_MULTI_SZ, the MSDN documentation for RegSetValueEx
  https://msdn.microsoft.com/en-us/library/windows/desktop/ms724923(v=vs.85).aspx
  states that for the lpData parameter that "With the REG_MULTI_SZ data
  type, the string must be terminated with two null characters."  It
  further states for the cbData parameter that "The size of the
  information pointed to by the lpData parameter, in bytes. If the data
  is of type REG_SZ, REG_EXPAND_SZ, or REG_MULTI_SZ, cbData must include
  the size of the terminating null character or characters."

  Looking at the implementation, we can see that the length for a
  REG_MULTI_SZ is calculated properly, but the second terminating NULL
  is never written to the string.

  The REG_SZ and REG_EXPAND_SZ handling is affected by the same issue as
  the length calculation takes into account a terminator that is not
  present.

  This can lead to intermittent memory corruption based on what's
  present in the last 2 bytes of a given buffer. There is no guarantee
  that this memory is zeroed out prior to use.

  Unfortunately, the only way to work around this issue is to do a
  reimplementation of write that terminates the array of bytes properly
  (with a double NULL terminator that is UTF16-LE).  Note that
  the end of the byte array should be 4 NULL bytes like [0, 0, 0, 0]

 - Add additional specs for writing and destroying all of the supported
   registry types.

 - Note: No support has been added for REG_DWORD_BIG_ENDIAN

lib/puppet/provider/registry_value/registry.rb

@@ -158,7 +158,7 @@ def write_value
     begin
       hive.open(subkey, Win32::Registry::KEY_ALL_ACCESS | access) do |reg|
         ary = to_native(resource[:type], resource[:data])
-        reg.write(valuename, ary[0], ary[1])
+        write(reg, valuename, ary[0], ary[1])
       end
     rescue Win32::Registry::Error => detail
       error = case detail.code
@@ -174,6 +174,65 @@ def write_value
     end
   end
 
+  BITS_PER_BYTE = 8
+  BYTE_MASK = 0xFF
+
+  def to_byte_array(num, of_length)
+    bytes = []
+    while
+      # mask only least significant byte and prepend
+      bytes.unshift(num & BYTE_MASK)
+
+      # nothing left to shave off
+      break if (num <= BYTE_MASK)
+
+      # shift off the least significant byte and continue
+      num >>= BITS_PER_BYTE
+    end
+
+    pad = of_length - bytes.length
+    bytes.concat([0] * pad) if pad > 0
+
+    bytes
+  end
+
+  def data_to_bytes(type, data)
+    bytes = []
+
+    case type
+      when Win32::Registry::REG_SZ, Win32::Registry::REG_EXPAND_SZ
+        bytes = wide_string(data).bytes.to_a
+      when Win32::Registry::REG_MULTI_SZ
+        # each wide string is already NULL terminated
+        bytes = data.map { |s| wide_string(s).bytes.to_a }.flat_map { |a| a }
+        # requires an additional NULL terminator to terminate properly
+        bytes << 0 << 0
+      when Win32::Registry::REG_BINARY
+        bytes = data.bytes.to_a
+      when Win32::Registry::REG_DWORD
+        bytes = to_byte_array(data, FFI::Type::UINT32.size)
+      when Win32::Registry::REG_QWORD
+        bytes = to_byte_array(data, FFI::Type::UINT64.size)
+      else
+        raise TypeError, "Unsupported type #{type}"
+    end
+
+    bytes
+  end
+
+  def write(reg, name, type, data)
+    from_string_to_wide_string(valuename) do |name_ptr|
+      bytes = data_to_bytes(type, data)
+      FFI::MemoryPointer.new(:uchar, bytes.length) do |data_ptr|
+        data_ptr.write_array_of_uchar(bytes)
+        if RegSetValueExW(reg.hkey, name_ptr, 0,
+          type, data_ptr, data_ptr.size) != 0
+            raise Puppet::Util::Windows::Error.new("Failed to write registry value")
+        end
+      end
+    end
+  end
+
   def path
     @path ||= PuppetX::Puppetlabs::Registry::RegistryValuePath.new(resource.parameter(:path).value)
   end

lib/puppet_x/puppetlabs/registry/provider_base.rb

@@ -51,6 +51,19 @@ def self.define_ffi(base)
     attach_function :RegDeleteValueW,
       [:handle, :pointer], :win32_long
 
+    # https://msdn.microsoft.com/en-us/library/windows/desktop/ms724923(v=vs.85).aspx
+    # LONG WINAPI RegSetValueEx(
+    #   _In_             HKEY    hKey,
+    #   _In_opt_         LPCTSTR lpValueName,
+    #   _Reserved_       DWORD   Reserved,
+    #   _In_             DWORD   dwType,
+    #   _In_       const BYTE    *lpData,
+    #   _In_             DWORD   cbData
+    # );
+    ffi_lib :advapi32
+    attach_function :RegSetValueExW,
+      [:handle, :pointer, :dword, :dword, :pointer, :dword], :win32_long
+
     # this duplicates code found in puppet, but necessary for backwards compat
     class << base
       # note that :uchar is aliased in Puppet to :byte
@@ -140,6 +153,10 @@ def from_string_to_wide_string(str, &block)
     self.class.from_string_to_wide_string(str, &block)
   end
 
+  def wide_string(str)
+    self.class.wide_string(str)
+  end
+
   def hkeys
     self.class.hkeys
   end

spec/unit/puppet/provider/registry_value_spec.rb

@@ -79,25 +79,48 @@
   end
 
   describe "#destroy" do
-    it "can destroy a randomly created value" do
-
-      guid = SecureRandom.uuid
-      reg_value = type.new(:path => "hklm\\#{puppet_key}\\#{subkey_name}\\#{guid}",
-        :type => 'string',
-        :data => guid,
+    let (:path) { path = "hklm\\#{puppet_key}\\#{subkey_name}\\#{SecureRandom.uuid}" }
+    def create_and_destroy(path, reg_type, data)
+      reg_value = type.new(:path => path,
+        :type => reg_type,
+        :data => data,
         :provider => described_class.name)
       already_exists = reg_value.provider.exists?
       already_exists.should be_false
 
       # something has gone terribly wrong here, pull the ripcord
-      break if already_exists
+      fail if already_exists
 
       reg_value.provider.create
       reg_value.provider.exists?.should be_true
 
       reg_value.provider.destroy
       reg_value.provider.exists?.should be_false
     end
+
+    it "can destroy a randomly created REG_SZ value" do
+      create_and_destroy(path, :string, SecureRandom.uuid)
+    end
+
+    it "can destroy a randomly created REG_EXPAND_SZ value" do
+      create_and_destroy(path, :expand, "#{SecureRandom.uuid} system root is %SystemRoot%")
+    end
+
+    it "can destroy a randomly created REG_BINARY value" do
+      create_and_destroy(path, :binary, '01011010')
+    end
+
+    it "can destroy a randomly created REG_DWORD value" do
+      create_and_destroy(path, :dword, rand(2 ** 32 - 1))
+    end
+
+    it "can destroy a randomly created REG_QWORD value" do
+      create_and_destroy(path, :qword, rand(2 ** 64 - 1))
+    end
+
+    it "can destroy a randomly created REG_MULTI_SZ value" do
+      create_and_destroy(path, :array, [SecureRandom.uuid, SecureRandom.uuid])
+    end
   end
 
   context "when reading non-ASCII values" do