[ros-diffs] [reactos] 03/12: [ADVAPI32] Fix a buffer overflow in RegQueryValueExA

[Timo Kreuzer]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=c5158963a3be6c19da30a…

commit c5158963a3be6c19da30a2f9000e88e7f88a4986
Author:     Timo Kreuzer <timo.kreuzer(a)reactos.org>
AuthorDate: Sat Dec 10 14:07:16 2022 +0200
Commit:     Timo Kreuzer <timo.kreuzer(a)reactos.org>
CommitDate: Wed Jan 4 10:32:28 2023 +0100

    [ADVAPI32] Fix a buffer overflow in RegQueryValueExA
    
    The code was trying to check whether the output string was already NULL terminated by
RtlUnicodeToMultiByteN before NULL terminating it by checking DataStr[*count - 1] for a
NULL terminator. But since RtlUnicodeToMultiByteSize always returns the size without the
NULL terminator, DataStr[*count - 1] would always be the last actual character, never an
optional NULL terminator.
    For 0 sized strings this would actually lead to accessing the output buffer at
position -1 (on 32 bit)  or 0xFFFFFFFF (on 64 bit).
    Fix this by removing the check. This fixes a crash in advapi32_winetest:registry on
x64.
---
 dll/win32/advapi32/reg/reg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/dll/win32/advapi32/reg/reg.c b/dll/win32/advapi32/reg/reg.c
index 9beecc99d2c..904ca010eca 100644
--- a/dll/win32/advapi32/reg/reg.c
+++ b/dll/win32/advapi32/reg/reg.c
@@ -4088,6 +4088,7 @@ RegQueryValueExA(
     /* We don't need this anymore */
     RtlFreeUnicodeString(&nameW);
 
+    /* Get the length for the multi-byte string (without the terminating NULL!) */
     DataLength = *count;
     RtlUnicodeToMultiByteSize(count, Buffer, BufferSize);
 
@@ -4101,7 +4102,7 @@ RegQueryValueExA(
     RtlUnicodeToMultiByteN(DataStr, DataLength, NULL, Buffer, BufferSize);
 
     /* NULL-terminate if there is enough room */
-    if ((DataLength > *count) && (DataStr[*count - 1] != '\0'))
+    if (DataLength > *count)
         DataStr[*count] = '\0';
 
     RtlFreeHeap(RtlGetProcessHeap(), 0, Buffer);