[ruby-core:70395] [Ruby trunk - Bug #11439] Win32 Registry corruption when writing REG_MULTI_SZ values

Issue #11439 has been updated by cremno phobia.


Ethan Brown wrote:
> I still believe the logic in `SetValue` should be `size ||= data.size + WCHAR_SIZE` given the data has been encoded as `WCHAR` there...

That's only half correct. It actually has to be `size ||= data.bytesize + WCHAR_SIZE`. The `cbData` argument must be the size in bytes but `String#size` returns the length in characters.

----------------------------------------
Bug #11439: Win32 Registry corruption when writing REG_MULTI_SZ values
https://bugs.ruby-lang.org/issues/11439#change-53796

* Author: Ethan Brown
* Status: Closed
* Priority: Normal
* Assignee: 
* ruby -v: ruby 2.1.5p273 (2014-11-13 revision 48405) [x64-mingw32]
* Backport: 2.0.0: UNKNOWN, 2.1: REQUIRED, 2.2: REQUIRED
----------------------------------------
A simple reproduction of the problem that breaks on first corrupted write:

~~~ruby
require 'win32/registry'

root = Win32::Registry::HKEY_CURRENT_USER
root.create('SOFTWARE\rubyfail') do |reg|
  1000.times do |i|
    value = ('a'..'z').to_a.shuffle[0,20].join
    reg.write('foo', Win32::Registry::REG_MULTI_SZ, [value])

    if reg['foo'] != [value]
      puts "iteration #{i} - expected value #{value}, got #{reg['foo']}"
      break
    end
  end
end
~~~

A message should be emitted like

~~~
iteration 4 - expected value notemdubgxlryjiqhcaf, got ["notemdubgxlryjiqhcaf", "\u6B00"]
~~~

Looking at the code, there are issues with how Ruby is handling strings during Registry writes.
https://github.com/ruby/ruby/blob/v2_1_6/ext/win32/lib/win32/registry.rb#L730-L735

* For `REG_MULTI_SZ`, Ruby is encoding each string in UTF-16LE and joining them with a `WCHAR_NUL`, but is only appending a single `WCHAR_NUL`, when it should be appending two.  The last 4 bytes of the buffer used for `REG_MULTI_SZ` should always be [0, 0, 0, 0].  Because the second NULL is missing, whatever data happens to be stored in the final 2 bytes is being written to the registry in the above example.

* Additionally, for `REG_SZ` or `REG_EXPAND_SZ`, Ruby is only encoding in UTF-16LE, but is not appending the appropriate NULL terminator prior to passing the string to `SetValue`.  Simply calling `data.encode(WCHAR)` does not NULL terminate a string.



My suggestion is to rewrite the method like:

~~~ruby
def write(name, type, data)
    case type
    when REG_SZ, REG_EXPAND_SZ
      data = data.encode(WCHAR) + WCHAR_NUL
    when REG_MULTI_SZ
      data = data.to_a.map {|s| s.encode(WCHAR)}.join(WCHAR_NUL) << WCHAR_NUL << WCHAR_NUL
    when REG_BINARY
      data = data.to_s
    when REG_DWORD
      data = API.packdw(data.to_i)
    when REG_DWORD_BIG_ENDIAN
      data = [data.to_i].pack('N')
    when REG_QWORD
      data = API.packqw(data.to_i)
    else
      raise TypeError, "Unsupported type #{type}"
    end
    API.SetValue(@hkey, name, type, data, data.bytesize)
end
~~~

Note that `termsize` has been removed as it is unnecessary if `data` has the appropriate bytes already.


Furthermore, there is also a bug in the `SetValue` implementation for when the default size for a string is set, when no length is passed:
https://github.com/ruby/ruby/blob/v2_1_6/ext/win32/lib/win32/registry.rb#L318

Since UTF-16LE strings are always an even number of bytes, it would only make sense to `size ||= data.size + 2` rather than `size ||= data.size + 1`.  That of course assumes that the incoming string is already NULL terminated, which it may not be.  My suggestion would be to remove this line altogether unless there is intent to verify the last byte.



Please see the [MSDN Documentation for SetRegValueEx](https://msdn.microsoft.com/en-us/library/windows/desktop/ms724923\(v=vs.85\).aspx) for the specifics.  The most important parts are:

~~~
lpData [in]

The data to be stored.

For string-based types, such as REG_SZ, the string must be null-terminated. With the REG_MULTI_SZ data type, the string must be terminated with two null characters. String-literal values must be formatted using a backslash preceded by another backslash as an escape character. For example, specify "C:\\mydir\\myfile" to store the string "C:\mydir\myfile".
Note  lpData indicating a null value is valid, however, if this is the case, cbData must be set to '0'.

cbData [in]

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.
~~~





-- 
https://bugs.ruby-lang.org/