Skip to content

Commit a062fe0

Browse files
committed
Ensure reg values are null terminated
RegQueryValueExW doesn't guarantee the returned buffer for the `lpcbData` parameter is null terminated, so ensure that it is. For REG_SZ & REG_EXPAND_SZ extend the buffer by 1 wchar (2 bytes) so we can write a wide null terminator that is guaranteed not to overwrite user data. If Windows doesn't null terminate the string, then it will have the following byte sequence (as UTF-16LE): \x41 \x00 \x42 \x00 \x00 If Windows does null terminate, then we will add an extra wide null terminator, which won't hurt anything: \x41 \x00 \x42 \x00 \x00 \x00 \x00 The same applies to REG_MULTI_SZ, except it is supposed to have two wide null terminators (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'], then in UTF-16LE it should be represented as the following: \x41 \x00 \x00 \x00 \x42 \x00 \x00 \x00 \x00 \x00 Where the overall binary string ends with 2 wide null terminators.
1 parent 493a8b7 commit a062fe0

File tree

1 file changed

+33
-2
lines changed

1 file changed

+33
-2
lines changed

lib/puppet/util/windows/registry.rb

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,12 +273,43 @@ def query_value_ex(key, name_ptr, &block)
273273
FFI::Pointer::NULL, type_ptr,
274274
FFI::Pointer::NULL, length_ptr)
275275

276-
FFI::MemoryPointer.new(:byte, length_ptr.read_dword) do |buffer_ptr|
276+
# The call to RegQueryValueExW below is potentially unsafe:
277+
# https://learn.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regqueryvalueexw
278+
#
279+
# "If the data has the REG_SZ, REG_MULTI_SZ or REG_EXPAND_SZ type,
280+
# the string may not have been stored with the proper terminating
281+
# null characters. Therefore, even if the function returns
282+
# ERROR_SUCCESS, the application should ensure that the string is
283+
# properly terminated before using it; otherwise, it may overwrite a
284+
# buffer. (Note that REG_MULTI_SZ strings should have two
285+
# terminating null characters.)"
286+
#
287+
# Since we don't know if the values will be properly null terminated,
288+
# extend the buffer to guarantee we can append one or two wide null
289+
# characters, without overwriting any data.
290+
buf_bytes_len = case type_ptr.read_dword
291+
when Win32::Registry::REG_SZ, Win32::Registry::REG_EXPAND_SZ
292+
length_ptr.read_dword + WCHAR_SIZE
293+
when Win32::Registry::REG_MULTI_SZ
294+
length_ptr.read_dword + (WCHAR_SIZE * 2)
295+
else
296+
length_ptr.read_dword
297+
end
298+
299+
FFI::MemoryPointer.new(:byte, buf_bytes_len) do |buffer_ptr|
277300
result = RegQueryValueExW(key.hkey, name_ptr,
278301
FFI::Pointer::NULL, type_ptr,
279302
buffer_ptr, length_ptr)
280303

281-
if result != FFI::ERROR_SUCCESS
304+
if result == FFI::ERROR_SUCCESS
305+
case type_ptr.read_dword
306+
when Win32::Registry::REG_SZ, Win32::Registry::REG_EXPAND_SZ
307+
buffer_ptr.put_uint16(buf_bytes_len - WCHAR_SIZE, 0)
308+
when Win32::Registry::REG_MULTI_SZ
309+
buffer_ptr.put_uint16(buf_bytes_len - WCHAR_SIZE - WCHAR_SIZE, 0)
310+
buffer_ptr.put_uint16(buf_bytes_len - WCHAR_SIZE, 0)
311+
end
312+
else
282313
# buffer is raw bytes, *not* chars - less a NULL terminator
283314
name_length = (name_ptr.size / WCHAR_SIZE) - 1 if name_ptr.size > 0
284315
msg = _("Failed to read registry value %{value} at %{key}") % { value: name_ptr.read_wide_string(name_length), key: key.keyname }

0 commit comments

Comments
 (0)