[reactos] 01/01: [SMSS] Use RTL string-safe functions in critical places. Add validity checks for returned NtQueryValueKey() data. (#2704)
https://git.reactos.org/?p=reactos.git;a=commitdiff;h=87e2ec585f70ea1c80c0f7...
commit 87e2ec585f70ea1c80c0f71c8719ff5b4984b2c4 Author: Hermès Bélusca-Maïto hermes.belusca-maito@reactos.org AuthorDate: Thu Apr 30 18:42:16 2020 +0200 Commit: Hermès Bélusca-Maïto hermes.belusca-maito@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; }
-
Hermès Bélusca-Maïto