Skip to content

Commit

Permalink
(PUP-11122) Ensure reg values are null terminated
Browse files Browse the repository at this point in the history
RegQueryValueExW doesn't guarantee the returned buffer for the `lpcbData`
parameter is null terminated, so ensure that it is.

When retrieving a registry value of type REG_SZ or REG_EXPAND_SZ extend the
buffer by 1 wchar (2 bytes) so we can always write a wchar null terminator that
is guaranteed not to overwrite user data.

The resulting wchar array is then guaranteed to be properly wchar null
terminated:

    \u0041
    \u0042
    \u0000

which when UTF-16LE encoded will result in the byte array:

    \x41 \x00
    \x42 \x00
    \x00 \x00

If Windows does null terminate, then we will add an additional wchar null
terminator, which won't hurt anything.

The same applies to REG_MULTI_SZ, except it is supposed to have two wchar null
terminators, for a total of 4 bytes. One terminator is for the last string in
the array, and one terminator is for the entire array. For example, if the array
contains the strings ['A', 'B'] and Windows does not null terminate the
`lpcbData` buffer, then the resulting UTF-16LE encoded byte array will contain:

    \x41 \x00
    \x00 \x00
    \x42 \x00
    \x00 \x00
    \x00 \x00
  • Loading branch information
joshcooper committed Jan 18, 2024
1 parent 493a8b7 commit a8fc33d
Showing 1 changed file with 35 additions and 2 deletions.
37 changes: 35 additions & 2 deletions lib/puppet/util/windows/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,45 @@ def query_value_ex(key, name_ptr, &block)
FFI::Pointer::NULL, type_ptr,
FFI::Pointer::NULL, length_ptr)

FFI::MemoryPointer.new(:byte, length_ptr.read_dword) do |buffer_ptr|
# The call to RegQueryValueExW below is potentially unsafe:
# https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regqueryvalueexw
#
# "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.)"
#
# Since we don't know if the values will be properly null terminated,
# extend the buffer to guarantee we can append one or two wide null
# characters, without overwriting any data.
base_bytes_len = type_ptr.read_dword
pad_bytes_len = case type_ptr.read_dword
when Win32::Registry::REG_SZ, Win32::Registry::REG_EXPAND_SZ
WCHAR_SIZE
when Win32::Registry::REG_MULTI_SZ
WCHAR_SIZE * 2
else
0
end

FFI::MemoryPointer.new(:byte, base_bytes_len + pad_bytes_len) do |buffer_ptr|
result = RegQueryValueExW(key.hkey, name_ptr,
FFI::Pointer::NULL, type_ptr,
buffer_ptr, length_ptr)

if result != FFI::ERROR_SUCCESS
# Ensure buffer is null terminated with 1 or 2 wchar nulls, depending on the type
if result == FFI::ERROR_SUCCESS
case type_ptr.read_dword
when Win32::Registry::REG_SZ, Win32::Registry::REG_EXPAND_SZ
buffer_ptr.put_uint16(base_bytes_len, 0)
when Win32::Registry::REG_MULTI_SZ
buffer_ptr.put_uint16(base_bytes_len, 0)
buffer_ptr.put_uint16(base_bytes_len + WCHAR_SIZE, 0)
end
else
# buffer is raw bytes, *not* chars - less a NULL terminator
name_length = (name_ptr.size / WCHAR_SIZE) - 1 if name_ptr.size > 0
msg = _("Failed to read registry value %{value} at %{key}") % { value: name_ptr.read_wide_string(name_length), key: key.keyname }
Expand Down

0 comments on commit a8fc33d

Please sign in to comment.