Skip to content

Commit 25a33df

Browse files
committed
(PUP-11122) Ensure reg values are null terminated
RegQueryValueExW doesn't guarantee the returned buffer for the `lpData` 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 `lpData` buffer, then the resulting UTF-16LE encoded byte array will contain: \x41 \x00 \x00 \x00 \x42 \x00 \x00 \x00 \x00 \x00
1 parent 493a8b7 commit 25a33df

File tree

1 file changed

+35
-2
lines changed

1 file changed

+35
-2
lines changed

lib/puppet/util/windows/registry.rb

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,12 +273,45 @@ 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+
base_bytes_len = length_ptr.read_dword
291+
pad_bytes_len = case type_ptr.read_dword
292+
when Win32::Registry::REG_SZ, Win32::Registry::REG_EXPAND_SZ
293+
WCHAR_SIZE
294+
when Win32::Registry::REG_MULTI_SZ
295+
WCHAR_SIZE * 2
296+
else
297+
0
298+
end
299+
300+
FFI::MemoryPointer.new(:byte, base_bytes_len + pad_bytes_len) do |buffer_ptr|
277301
result = RegQueryValueExW(key.hkey, name_ptr,
278302
FFI::Pointer::NULL, type_ptr,
279303
buffer_ptr, length_ptr)
280304

281-
if result != FFI::ERROR_SUCCESS
305+
# Ensure buffer is null terminated with 1 or 2 wchar nulls, depending on the type
306+
if result == FFI::ERROR_SUCCESS
307+
case type_ptr.read_dword
308+
when Win32::Registry::REG_SZ, Win32::Registry::REG_EXPAND_SZ
309+
buffer_ptr.put_uint16(base_bytes_len, 0)
310+
when Win32::Registry::REG_MULTI_SZ
311+
buffer_ptr.put_uint16(base_bytes_len, 0)
312+
buffer_ptr.put_uint16(base_bytes_len + WCHAR_SIZE, 0)
313+
end
314+
else
282315
# buffer is raw bytes, *not* chars - less a NULL terminator
283316
name_length = (name_ptr.size / WCHAR_SIZE) - 1 if name_ptr.size > 0
284317
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)