[fireball] 50968: [USETUP] - Fix a buffer overflow (overread) when adding a locale key to the registry. The history of this bug is funny: 1. Eric wrote the code, which sets a key of REG_SZ type, as...

Author: fireball
Date: Fri Mar  4 18:18:05 2011
New Revision: 50968

URL: http://svn.reactos.org/svn/reactos?rev=50968&view=rev
Log:
[USETUP]
- Fix a buffer overflow (overread) when adding a locale key to the registry. The history
of this bug is funny:
1. Eric wrote the code, which sets a key of REG_SZ type, as 4 widechars plus terminating
zero, but passes 8 as the bytesize of the buffer. It's not fully correct (a
terminating zero is absent from the bytesize of the buffer, but MSDN doesn't specify
if it should be added or not, and hardcoding "8" is not the best idea too) but
not dramatic. That was revision 9596, 7 years ago.
2. Lentin notices something is not right in this code, and decides to "fix" it
by multiplying that same hardcoded value by.... guess what? sizeof(PWCHAR)! That is, size
of a pointer, which on an x86 would be 4 bytes. Massive out of bounds access obviously
happens. That was revision 31642, 3 years ago.
3. Very soon Colin reshuffles and improves the code based on patch #2635, however the
problem still goes unnoticed (r31655+).
See issue #5810 for more details.

Modified:
    trunk/reactos/base/setup/usetup/settings.c

Modified: trunk/reactos/base/setup/usetup/settings.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/base/setup/usetup/settings…
==============================================================================
--- trunk/reactos/base/setup/usetup/settings.c [iso-8859-1] (original)
+++ trunk/reactos/base/setup/usetup/settings.c [iso-8859-1] Fri Mar  4 18:18:05 2011
@@ -637,6 +637,10 @@
     if (LanguageId == NULL)
         return FALSE;
 
+    /* Skip first 4 zeroes */
+    if (wcslen(LanguageId) >= 4)
+        LanguageId += 4;
+
     /* Open the NLS language key */
     RtlInitUnicodeString(&KeyName,
                         
L"\\Registry\\Machine\\SYSTEM\\CurrentControlSet\\Control\\NLS\\Language");
@@ -660,13 +664,12 @@
     /* Set default language */
     RtlInitUnicodeString(&ValueName,
                          L"Default");
-
     Status = NtSetValueKey(KeyHandle,
                            &ValueName,
                            0,
                            REG_SZ,
-                           (PVOID)(LanguageId + 4),
-                           8 * sizeof(PWCHAR));
+                           (PVOID)LanguageId,
+                           (wcslen(LanguageId) + 1) * sizeof(WCHAR));
     if (!NT_SUCCESS(Status))
     {
         DPRINT1("NtSetValueKey() failed (Status %lx)\n", Status);
@@ -681,8 +684,8 @@
                             &ValueName,
                             0,
                             REG_SZ,
-                            (PVOID)(LanguageId + 4),
-                            8 * sizeof(PWCHAR));
+                            (PVOID)LanguageId,
+                            (wcslen(LanguageId) + 1) * sizeof(WCHAR));
     NtClose(KeyHandle);
     if (!NT_SUCCESS(Status))
     {