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

Issue #11439 has been updated by Ethan Brown.


Ah I did not realize that strings all implicitly null-terminated.  Thanks for that explanation cremno phobia - that makes sense.  That would also explain why I could repro only against `REG_MULTI_SZ` but not against `REG_SZ`.

I feel like the implementation is a little sneaky given it relies on that internal implementation detail (so from the outside looking in, it feels a bit magical to increase the buffer size passed to the Windows API they way it's being done).  But that's probably well understood by Ruby maintainers.

I still believe the logic in `SetValue` should be `size ||= data.size + WCHAR_SIZE` given the data has been encoded as `WCHAR` there... but I'm not sure that any callers are calling `SetValue`without passing a size anyhow?

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

* Author: Ethan Brown
* Status: Feedback
* 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/