[ros-diffs] [reactos] 01/01: [SMSS] Use RTL string-safe functions in critical places. Add validity checks for returned NtQueryValueKey() data. (#2704)

[Hermès Bélusca-Maïto]

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

commit 87e2ec585f70ea1c80c0f71c8719ff5b4984b2c4
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Thu Apr 30 18:42:16 2020 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Sat Oct 10 16:25:22 2020 +0200

    [SMSS] Use RTL string-safe functions in critical places. Add validity checks for
returned NtQueryValueKey() data. (#2704)
    
    - Not all the wcscpy() / swprintf() calls have been converted to their
      string-safe equivalents. Instead I used the string-safe functions only
      for places where strings of unknown length were copied into fixed-size
      internal buffers. On the contrary, for known-fixed-length strings being
      copied or numbers being converted to string representations in large
      enough buffers, I kept the original function calls.
    
    - Verify the registry data that has been returned by NtQueryValueKey():
      * When expecting (not multi) strings, check whether the data type is
        either REG_SZ or REG_EXPAND_SZ.
      * When expecting DWORD values, check whether the data type is
        REG_DWORD and whether the data length is (greater or) equal to
        sizeof(ULONG).
---
 base/system/smss/sminit.c   | 104 +++++++++++++++++++++++++++++---------------
 base/system/smss/smsbapi.c  |   2 +-
 base/system/smss/smss.h     |   2 +
 base/system/smss/smsubsys.c |   4 +-
 base/system/smss/smutil.c   |  12 ++---
 5 files changed, 80 insertions(+), 44 deletions(-)

diff --git a/base/system/smss/sminit.c b/base/system/smss/sminit.c
index d671bb7b274..8960010fcc8 100644
--- a/base/system/smss/sminit.c
+++ b/base/system/smss/sminit.c
@@ -789,12 +789,13 @@ SmpTranslateSystemPartitionInformation(VOID)
     UNICODE_STRING UnicodeString, LinkTarget, SearchString, SystemPartition;
     OBJECT_ATTRIBUTES ObjectAttributes;
     HANDLE KeyHandle, LinkHandle;
-    CHAR ValueBuffer[512 + sizeof(KEY_VALUE_PARTIAL_INFORMATION)];
-    PKEY_VALUE_PARTIAL_INFORMATION PartialInfo = (PVOID)ValueBuffer;
     ULONG Length, Context;
-    CHAR DirInfoBuffer[512 + sizeof(OBJECT_DIRECTORY_INFORMATION)];
-    POBJECT_DIRECTORY_INFORMATION DirInfo = (PVOID)DirInfoBuffer;
+    size_t StrLength;
     WCHAR LinkBuffer[MAX_PATH];
+    CHAR ValueBuffer[sizeof(KEY_VALUE_PARTIAL_INFORMATION) + 512];
+    PKEY_VALUE_PARTIAL_INFORMATION PartialInfo = (PVOID)ValueBuffer;
+    CHAR DirInfoBuffer[sizeof(OBJECT_DIRECTORY_INFORMATION) + 512];
+    POBJECT_DIRECTORY_INFORMATION DirInfo = (PVOID)DirInfoBuffer;
 
     /* Open the setup key */
     RtlInitUnicodeString(&UnicodeString,
L"\\Registry\\Machine\\System\\Setup");
@@ -806,7 +807,7 @@ SmpTranslateSystemPartitionInformation(VOID)
     Status = NtOpenKey(&KeyHandle, KEY_READ, &ObjectAttributes);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("SMSS: can't open system setup key for reading: 0x%x\n",
Status);
+        DPRINT1("SMSS: Cannot open system setup key for reading: 0x%x\n",
Status);
         return;
     }
 
@@ -819,14 +820,22 @@ SmpTranslateSystemPartitionInformation(VOID)
                              sizeof(ValueBuffer),
                              &Length);
     NtClose(KeyHandle);
-    if (!NT_SUCCESS(Status))
+    if (!NT_SUCCESS(Status) ||
+        ((PartialInfo->Type != REG_SZ) && (PartialInfo->Type !=
REG_EXPAND_SZ)))
     {
-        DPRINT1("SMSS: can't query SystemPartition value: 0x%x\n",
Status);
+        DPRINT1("SMSS: Cannot query SystemPartition value (Type %lu, Status
0x%x)\n",
+                PartialInfo->Type, Status);
         return;
     }
 
-    /* Initialize the system partition string string */
-    RtlInitUnicodeString(&SystemPartition, (PWCHAR)PartialInfo->Data);
+    /* Initialize the system partition string */
+    RtlInitEmptyUnicodeString(&SystemPartition,
+                              (PWCHAR)PartialInfo->Data,
+                              PartialInfo->DataLength);
+    RtlStringCbLengthW(SystemPartition.Buffer,
+                       SystemPartition.MaximumLength,
+                       &StrLength);
+    SystemPartition.Length = (USHORT)StrLength;
 
     /* Enumerate the directory looking for the symbolic link string */
     RtlInitUnicodeString(&SearchString, L"SymbolicLink");
@@ -840,7 +849,7 @@ SmpTranslateSystemPartitionInformation(VOID)
                                     NULL);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("SMSS: can't find drive letter for system partition\n");
+        DPRINT1("SMSS: Cannot find drive letter for system partition\n");
         return;
     }
 
@@ -872,8 +881,8 @@ SmpTranslateSystemPartitionInformation(VOID)
                 /* Check if it matches the string we had found earlier */
                 if ((NT_SUCCESS(Status)) &&
                     ((RtlEqualUnicodeString(&SystemPartition,
-                                           &LinkTarget,
-                                           TRUE)) ||
+                                            &LinkTarget,
+                                            TRUE)) ||
                     ((RtlPrefixUnicodeString(&SystemPartition,
                                              &LinkTarget,
                                              TRUE)) &&
@@ -896,7 +905,7 @@ SmpTranslateSystemPartitionInformation(VOID)
     } while (NT_SUCCESS(Status));
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("SMSS: can't find drive letter for system partition\n");
+        DPRINT1("SMSS: Cannot find drive letter for system partition\n");
         return;
     }
 
@@ -911,7 +920,7 @@ SmpTranslateSystemPartitionInformation(VOID)
     Status = NtOpenKey(&KeyHandle, KEY_ALL_ACCESS, &ObjectAttributes);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("SMSS: can't open software setup key for writing: 0x%x\n",
+        DPRINT1("SMSS: Cannot open software setup key for writing: 0x%x\n",
                 Status);
         return;
     }
@@ -1349,7 +1358,7 @@ NTAPI
 SmpProcessModuleImports(IN PVOID Unused,
                         IN PCHAR ImportName)
 {
-    ULONG Length = 0, Chars;
+    ULONG Length = 0;
     WCHAR Buffer[MAX_PATH];
     PWCHAR DllName, DllValue;
     ANSI_STRING ImportString;
@@ -1365,20 +1374,22 @@ SmpProcessModuleImports(IN PVOID Unused,
     Status = RtlAnsiStringToUnicodeString(&ImportUnicodeString, &ImportString,
FALSE);
     if (!NT_SUCCESS(Status)) return;
 
-    /* Loop in case we find a forwarder */
-    ImportUnicodeString.MaximumLength = ImportUnicodeString.Length +
sizeof(UNICODE_NULL);
+    /* Loop to find the DLL file extension */
     while (Length < ImportUnicodeString.Length)
     {
         if (ImportUnicodeString.Buffer[Length / sizeof(WCHAR)] == L'.') break;
         Length += sizeof(WCHAR);
     }
 
-    /* Break up the values as needed */
+    /*
+     * Break up the values as needed; the buffer acquires the form:
+     * "dll_name.dll\0dll_name\0"
+     */
     DllValue = ImportUnicodeString.Buffer;
-    DllName = &ImportUnicodeString.Buffer[ImportUnicodeString.MaximumLength /
sizeof(WCHAR)];
-    Chars = Length >> 1;
-    wcsncpy(DllName, ImportUnicodeString.Buffer, Chars);
-    DllName[Chars] = 0;
+    DllName = &ImportUnicodeString.Buffer[(ImportUnicodeString.Length +
sizeof(UNICODE_NULL)) / sizeof(WCHAR)];
+    RtlStringCbCopyNW(DllName,
+                      ImportUnicodeString.MaximumLength - (ImportUnicodeString.Length +
sizeof(UNICODE_NULL)),
+                      ImportUnicodeString.Buffer, Length);
 
     /* Add the DLL to the list */
     SmpSaveRegistryValue(&SmpKnownDllsList, DllName, DllValue, TRUE);
@@ -1652,9 +1663,11 @@ SmpCreateDynamicEnvironmentVariables(VOID)
     OBJECT_ATTRIBUTES ObjectAttributes;
     UNICODE_STRING ValueName, DestinationString;
     HANDLE KeyHandle, KeyHandle2;
-    ULONG ResultLength;
     PWCHAR ValueData;
-    WCHAR ValueBuffer[512], ValueBuffer2[512];
+    ULONG ResultLength;
+    size_t StrLength;
+    WCHAR ValueBuffer[sizeof(KEY_VALUE_PARTIAL_INFORMATION) + 512];
+    WCHAR ValueBuffer2[sizeof(KEY_VALUE_PARTIAL_INFORMATION) + 512];
     PKEY_VALUE_PARTIAL_INFORMATION PartialInfo = (PVOID)ValueBuffer;
     PKEY_VALUE_PARTIAL_INFORMATION PartialInfo2 = (PVOID)ValueBuffer2;
 
@@ -1798,15 +1811,27 @@ SmpCreateDynamicEnvironmentVariables(VOID)
                              PartialInfo,
                              sizeof(ValueBuffer),
                              &ResultLength);
-    if (!NT_SUCCESS(Status))
+    if (!NT_SUCCESS(Status) ||
+        ((PartialInfo->Type != REG_SZ) && (PartialInfo->Type !=
REG_EXPAND_SZ)))
     {
         NtClose(KeyHandle2);
         NtClose(KeyHandle);
-        DPRINT1("SMSS: Unable to read %wZ\\%wZ - %x\n",
-                &DestinationString, &ValueName, Status);
+        DPRINT1("SMSS: Unable to read %wZ\\%wZ (Type %lu, Status 0x%x)\n",
+                &DestinationString, &ValueName, PartialInfo->Type, Status);
         return Status;
     }
 
+    /* Initialize the string so that it can be large enough
+     * to contain both the identifier and the vendor strings. */
+    RtlInitEmptyUnicodeString(&DestinationString,
+                              (PWCHAR)PartialInfo->Data,
+                              sizeof(ValueBuffer) -
+                                FIELD_OFFSET(KEY_VALUE_PARTIAL_INFORMATION, Data));
+    RtlStringCbLengthW(DestinationString.Buffer,
+                       PartialInfo->DataLength,
+                       &StrLength);
+    DestinationString.Length = (USHORT)StrLength;
+
     /* As well as the vendor... */
     RtlInitUnicodeString(&ValueName, L"VendorIdentifier");
     Status = NtQueryValueKey(KeyHandle2,
@@ -1816,23 +1841,27 @@ SmpCreateDynamicEnvironmentVariables(VOID)
                              sizeof(ValueBuffer2),
                              &ResultLength);
     NtClose(KeyHandle2);
-    if (NT_SUCCESS(Status))
+    if (NT_SUCCESS(Status) &&
+        ((PartialInfo2->Type == REG_SZ) || (PartialInfo2->Type == REG_EXPAND_SZ)))
     {
         /* To combine it into a single string */
-        swprintf((PWCHAR)PartialInfo->Data + wcslen((PWCHAR)PartialInfo->Data),
-                 L", %s",
-                 (PWCHAR)PartialInfo2->Data);
+        RtlStringCbPrintfW(DestinationString.Buffer + DestinationString.Length /
sizeof(WCHAR),
+                           DestinationString.MaximumLength - DestinationString.Length,
+                           L", %.*s",
+                           PartialInfo2->DataLength / sizeof(WCHAR),
+                           (PWCHAR)PartialInfo2->Data);
+        DestinationString.Length = (USHORT)(wcslen(DestinationString.Buffer) *
sizeof(WCHAR));
     }
 
     /* So that we can set this as the PROCESSOR_IDENTIFIER variable */
     RtlInitUnicodeString(&ValueName, L"PROCESSOR_IDENTIFIER");
-    DPRINT("Setting %wZ to %S\n", &ValueName, PartialInfo->Data);
+    DPRINT("Setting %wZ to %wZ\n", &ValueName, &DestinationString);
     Status = NtSetValueKey(KeyHandle,
                            &ValueName,
                            0,
                            REG_SZ,
-                           PartialInfo->Data,
-                           (wcslen((PWCHAR)PartialInfo->Data) + 1) * sizeof(WCHAR));
+                           DestinationString.Buffer,
+                           DestinationString.Length + sizeof(UNICODE_NULL));
     if (!NT_SUCCESS(Status))
     {
         DPRINT1("SMSS: Failed writing %wZ environment variable - %x\n",
@@ -1922,7 +1951,9 @@ SmpCreateDynamicEnvironmentVariables(VOID)
                                  sizeof(ValueBuffer),
                                  &ResultLength);
         NtClose(KeyHandle2);
-        if (NT_SUCCESS(Status))
+        if (NT_SUCCESS(Status) &&
+            (PartialInfo->Type == REG_DWORD) &&
+            (PartialInfo->DataLength >= sizeof(ULONG)))
         {
             /* Convert from the integer value to the correct specifier */
             RtlInitUnicodeString(&ValueName, L"SAFEBOOT_OPTION");
@@ -1957,7 +1988,8 @@ SmpCreateDynamicEnvironmentVariables(VOID)
         }
         else
         {
-            DPRINT1("SMSS: Failed querying safeboot option = %x\n", Status);
+            DPRINT1("SMSS: Failed to query SAFEBOOT option (Type %lu, Status
0x%x)\n",
+                    PartialInfo->Type, Status);
         }
     }
 
diff --git a/base/system/smss/smsbapi.c b/base/system/smss/smsbapi.c
index 847974da924..0cd49e1ed19 100644
--- a/base/system/smss/smsbapi.c
+++ b/base/system/smss/smsbapi.c
@@ -94,7 +94,7 @@ SmpSbCreateSession(IN PVOID Reserved,
                 NtClose(ProcessInformation->ProcessHandle);
                 NtClose(ProcessInformation->ThreadHandle);
                 SmpDereferenceSubsystem(KnownSubsys);
-                DbgPrint("SmpSbCreateSession: NtDuplicateObject (Thread) Failed
%lx\n", Status);
+                DPRINT1("SmpSbCreateSession: NtDuplicateObject (Thread) Failed
%lx\n", Status);
                 return Status;
             }
 
diff --git a/base/system/smss/smss.h b/base/system/smss/smss.h
index 831aeeaecb0..c59ecfff605 100644
--- a/base/system/smss/smss.h
+++ b/base/system/smss/smss.h
@@ -31,6 +31,8 @@
 #include <ndk/umfuncs.h>
 #include <ndk/kefuncs.h>
 
+#include <ntstrsafe.h>
+
 /* SM Protocol Header */
 #include <sm/smmsg.h>
 
diff --git a/base/system/smss/smsubsys.c b/base/system/smss/smsubsys.c
index 819ebe3392b..a504a185900 100644
--- a/base/system/smss/smsubsys.c
+++ b/base/system/smss/smsubsys.c
@@ -656,8 +656,8 @@ SmpLoadSubSystemsForMuSession(IN PULONG MuSessionId,
         if ((NT_SUCCESS(Status2)) && (InitialCommandBuffer[0]))
         {
             /* Put the debugger string with the Winlogon string */
-            wcscat(InitialCommandBuffer, L" ");
-            wcscat(InitialCommandBuffer, InitialCommand->Buffer);
+            RtlStringCbCatW(InitialCommandBuffer, sizeof(InitialCommandBuffer), L"
");
+            RtlStringCbCatW(InitialCommandBuffer, sizeof(InitialCommandBuffer),
InitialCommand->Buffer);
             RtlInitUnicodeString(InitialCommand, InitialCommandBuffer);
         }
     }
diff --git a/base/system/smss/smutil.c b/base/system/smss/smutil.c
index f3d5da09a18..8b61ab37813 100644
--- a/base/system/smss/smutil.c
+++ b/base/system/smss/smutil.c
@@ -380,7 +380,7 @@ SmpParseCommandLine(IN PUNICODE_STRING CommandLine,
         else
         {
             /* Caller doesn't want flags, probably wants the image itself */
-            wcscpy(PathBuffer, Token.Buffer);
+            RtlStringCbCopyW(PathBuffer, sizeof(PathBuffer), Token.Buffer);
         }
     }
 
@@ -427,9 +427,9 @@ SmpQueryRegistrySosOption(VOID)
     UNICODE_STRING KeyName, ValueName;
     OBJECT_ATTRIBUTES ObjectAttributes;
     HANDLE KeyHandle;
+    ULONG Length;
     WCHAR ValueBuffer[VALUE_BUFFER_SIZE];
     PKEY_VALUE_PARTIAL_INFORMATION PartialInfo = (PVOID)ValueBuffer;
-    ULONG Length;
 
     /* Open the key */
     RtlInitUnicodeString(&KeyName,
@@ -442,7 +442,7 @@ SmpQueryRegistrySosOption(VOID)
     Status = NtOpenKey(&KeyHandle, KEY_READ, &ObjectAttributes);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("SMSS: can't open control key: 0x%x\n", Status);
+        DPRINT1("SMSS: Cannot open control key (Status 0x%x)\n", Status);
         return FALSE;
     }
 
@@ -456,9 +456,11 @@ SmpQueryRegistrySosOption(VOID)
                              &Length);
     ASSERT(Length < VALUE_BUFFER_SIZE);
     NtClose(KeyHandle);
-    if (!NT_SUCCESS(Status))
+    if (!NT_SUCCESS(Status) ||
+        ((PartialInfo->Type != REG_SZ) && (PartialInfo->Type !=
REG_EXPAND_SZ)))
     {
-        DPRINT1("SMSS: can't query value key: 0x%x\n", Status);
+        DPRINT1("SMSS: Cannot query value key (Type %lu, Status 0x%x)\n",
+                PartialInfo->Type, Status);
         return FALSE;
     }