Skip to content

(PUP-11122) Ensure reg values are null terminated #9205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 23, 2024
Merged

(PUP-11122) Ensure reg values are null terminated #9205

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
493a8b7
Refactor wchar size
joshcooper Jan 10, 2024
25a33df
(PUP-11122) Ensure reg values are null terminated
joshcooper Jan 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions lib/puppet/util/windows/registry.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ module Registry

ERROR_NO_MORE_ITEMS = 259

WCHAR_SIZE = FFI.type_size(:wchar)

def root(name)
Win32::Registry.const_get(name)
rescue NameError
Expand Down Expand Up @@ -234,7 +236,7 @@ def read(key, name_ptr, *rtype)

string_length = 0
# buffer is raw bytes, *not* chars - less a NULL terminator
string_length = (byte_length / FFI.type_size(:wchar)) - 1 if byte_length > 0
string_length = (byte_length / WCHAR_SIZE) - 1 if byte_length > 0

begin
result = case type
Expand Down Expand Up @@ -271,14 +273,47 @@ 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 = length_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 / FFI.type_size(:wchar)) - 1 if name_ptr.size > 0
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 }
raise Puppet::Util::Windows::Error.new(msg, result)
end
Expand Down