Bug #11439: Win32 Registry corruption when writing REG_MULTI_SZ values

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: ruby 2.1.5p273 (2014-11-13 revision 48405) [x64-mingw32]
• Backport: 2.0.0: UNKNOWN, 2.1: REQUIRED, 2.2: DONE

[Iristyle (Ethan Brown)]

A simple reproduction of the problem that breaks on first corrupted write:

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:

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

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Feedback

It seems working with the trunk.

[Iristyle (Ethan Brown)]

Since this is a corruption that depends on the memory state of the environment (whether or not it's zeroed), you might want to increase the number of iterations, or change the random string length in my reproduction case.

I can reproduce this very easily on Windows 2008R2 x64 (running inside VMWare fusion) on the stated Ruby version.

The errant code is still present in trunk at https://github.com/ruby/ruby/blob/trunk/ext/win32/lib/win32/registry.rb#L738-L759

[cremno (cremno phobia)]

This code relies on the fact that Ruby strings are implicitly null-terminated. Otherwise you'd always have to either create a copy or modify the original when calling functions that expect a null-terminated string.

Since 2.1 that's even the case for non-8-bit-encodings. E.g. for UTF-16 the implicit null terminator should be two null bytes (or one null character). But here it isn't: only one null byte instead of two are written in 2.1 (see the emitted message). So at least r49408 needs to be backported (Array#join and lot of other methods internally call str_buf_cat()). The 2.2 backport is r49644. It contains other related fixes that likely need to be backported too.

[Iristyle (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 SetValuewithout passing a size anyhow?

[nobu (Nobuyoshi Nakada)]

• Status changed from Feedback to Closed

Applied in changeset r51575.

---

win32/registry.rb: terminator size

• ext/win32/lib/win32/registry.rb (API#SetValue): add terminator
  size, not 1 byte. [ruby-core:70365] [Bug #11439]

[cremno (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.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.0.0: UNKNOWN, 2.1: REQUIRED, 2.2: REQUIRED to 2.0.0: UNKNOWN, 2.1: REQUIRED, 2.2: DONE

Backported into ruby_2_2 branch at r51618.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.0.0: UNKNOWN, 2.1: REQUIRED, 2.2: DONE to 2.0.0: UNKNOWN, 2.1: DONE, 2.2: DONE

ruby_2_1 r51620 merged revision(s) 51575,51584.

[cremno (cremno phobia)]

Usaku NAKAMURA wrote:

ruby_2_1 r51620 merged revision(s) 51575,51584.

This issue is about two different bugs. r49405 to r49408 need to be backported too.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.0.0: UNKNOWN, 2.1: DONE, 2.2: DONE to 2.0.0: UNKNOWN, 2.1: REQUIRED, 2.2: DONE

Oh, thanks for telling!