Intravision/reactos
1
0
Fork 0
mirror of https://github.com/reactos/reactos.git synced 2025-08-06 08:32:58 +00:00
Code Issues Projects Releases Packages Wiki Activity

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

Browse source 
- 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).
This commit is contained in:
Hermès Bélusca-Maïto 2020-04-30 18:42:16 +02:00
parent 8ee0ee6a88
commit 87e2ec585f
No known key found for this signature in database
GPG key ID: 3B2539C65E7B93D0
5 changed files with 80 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

104
base/system/smss/sminit.c
View file

@ -789,12 +789,13 @@ SmpTranslateSystemPartitionInformation(VOID)
UNICODE_STRING UnicodeString, LinkTarget, SearchString, SystemPartition; UNICODE_STRING UnicodeString, LinkTarget, SearchString, SystemPartition;
OBJECT_ATTRIBUTES ObjectAttributes; OBJECT_ATTRIBUTES ObjectAttributes;
HANDLE KeyHandle, LinkHandle; HANDLE KeyHandle, LinkHandle;
CHAR ValueBuffer[512 + sizeof(KEY_VALUE_PARTIAL_INFORMATION)];
PKEY_VALUE_PARTIAL_INFORMATION PartialInfo = (PVOID)ValueBuffer;
ULONG Length, Context; ULONG Length, Context;
CHAR DirInfoBuffer[512 + sizeof(OBJECT_DIRECTORY_INFORMATION)]; size_t StrLength;
POBJECT_DIRECTORY_INFORMATION DirInfo = (PVOID)DirInfoBuffer;
WCHAR LinkBuffer[MAX_PATH]; 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 */ /* Open the setup key */
RtlInitUnicodeString(&UnicodeString, L"\\Registry\\Machine\\System\\Setup"); RtlInitUnicodeString(&UnicodeString, L"\\Registry\\Machine\\System\\Setup");
@ -806,7 +807,7 @@ SmpTranslateSystemPartitionInformation(VOID)
Status = NtOpenKey(&KeyHandle, KEY_READ, &ObjectAttributes); Status = NtOpenKey(&KeyHandle, KEY_READ, &ObjectAttributes);
if (!NT_SUCCESS(Status)) 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; return;
} }
@ -819,14 +820,22 @@ SmpTranslateSystemPartitionInformation(VOID)
sizeof(ValueBuffer), sizeof(ValueBuffer),
&Length); &Length);
NtClose(KeyHandle); 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; return;
} }
/* Initialize the system partition string string */ /* Initialize the system partition string */
RtlInitUnicodeString(&SystemPartition, (PWCHAR)PartialInfo->Data); 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 */ /* Enumerate the directory looking for the symbolic link string */
RtlInitUnicodeString(&SearchString, L"SymbolicLink"); RtlInitUnicodeString(&SearchString, L"SymbolicLink");
@ -840,7 +849,7 @@ SmpTranslateSystemPartitionInformation(VOID)
NULL); NULL);
if (!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; return;
} }
@ -872,8 +881,8 @@ SmpTranslateSystemPartitionInformation(VOID)
/* Check if it matches the string we had found earlier */ /* Check if it matches the string we had found earlier */
if ((NT_SUCCESS(Status)) && if ((NT_SUCCESS(Status)) &&
((RtlEqualUnicodeString(&SystemPartition, ((RtlEqualUnicodeString(&SystemPartition,
&LinkTarget, &LinkTarget,
TRUE)) || TRUE)) ||
((RtlPrefixUnicodeString(&SystemPartition, ((RtlPrefixUnicodeString(&SystemPartition,
&LinkTarget, &LinkTarget,
TRUE)) && TRUE)) &&
@ -896,7 +905,7 @@ SmpTranslateSystemPartitionInformation(VOID)
} while (NT_SUCCESS(Status)); } while (NT_SUCCESS(Status));
if (!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; return;
} }
@ -911,7 +920,7 @@ SmpTranslateSystemPartitionInformation(VOID)
Status = NtOpenKey(&KeyHandle, KEY_ALL_ACCESS, &ObjectAttributes); Status = NtOpenKey(&KeyHandle, KEY_ALL_ACCESS, &ObjectAttributes);
if (!NT_SUCCESS(Status)) 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); Status);
return; return;
} }
@ -1349,7 +1358,7 @@ NTAPI
SmpProcessModuleImports(IN PVOID Unused, SmpProcessModuleImports(IN PVOID Unused,
IN PCHAR ImportName) IN PCHAR ImportName)
{ {
ULONG Length = 0, Chars; ULONG Length = 0;
WCHAR Buffer[MAX_PATH]; WCHAR Buffer[MAX_PATH];
PWCHAR DllName, DllValue; PWCHAR DllName, DllValue;
ANSI_STRING ImportString; ANSI_STRING ImportString;
@ -1365,20 +1374,22 @@ SmpProcessModuleImports(IN PVOID Unused,
Status = RtlAnsiStringToUnicodeString(&ImportUnicodeString, &ImportString, FALSE); Status = RtlAnsiStringToUnicodeString(&ImportUnicodeString, &ImportString, FALSE);
if (!NT_SUCCESS(Status)) return; if (!NT_SUCCESS(Status)) return;
/* Loop in case we find a forwarder */ /* Loop to find the DLL file extension */
ImportUnicodeString.MaximumLength = ImportUnicodeString.Length + sizeof(UNICODE_NULL);
while (Length < ImportUnicodeString.Length) while (Length < ImportUnicodeString.Length)
{ {
if (ImportUnicodeString.Buffer[Length / sizeof(WCHAR)] == L'.') break; if (ImportUnicodeString.Buffer[Length / sizeof(WCHAR)] == L'.') break;
Length += sizeof(WCHAR); 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; DllValue = ImportUnicodeString.Buffer;
DllName = &ImportUnicodeString.Buffer[ImportUnicodeString.MaximumLength / sizeof(WCHAR)]; DllName = &ImportUnicodeString.Buffer[(ImportUnicodeString.Length + sizeof(UNICODE_NULL)) / sizeof(WCHAR)];
Chars = Length >> 1; RtlStringCbCopyNW(DllName,
wcsncpy(DllName, ImportUnicodeString.Buffer, Chars); ImportUnicodeString.MaximumLength - (ImportUnicodeString.Length + sizeof(UNICODE_NULL)),
DllName[Chars] = 0; ImportUnicodeString.Buffer, Length);
/* Add the DLL to the list */ /* Add the DLL to the list */
SmpSaveRegistryValue(&SmpKnownDllsList, DllName, DllValue, TRUE); SmpSaveRegistryValue(&SmpKnownDllsList, DllName, DllValue, TRUE);
@ -1652,9 +1663,11 @@ SmpCreateDynamicEnvironmentVariables(VOID)
OBJECT_ATTRIBUTES ObjectAttributes; OBJECT_ATTRIBUTES ObjectAttributes;
UNICODE_STRING ValueName, DestinationString; UNICODE_STRING ValueName, DestinationString;
HANDLE KeyHandle, KeyHandle2; HANDLE KeyHandle, KeyHandle2;
ULONG ResultLength;
PWCHAR ValueData; 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 PartialInfo = (PVOID)ValueBuffer;
PKEY_VALUE_PARTIAL_INFORMATION PartialInfo2 = (PVOID)ValueBuffer2; PKEY_VALUE_PARTIAL_INFORMATION PartialInfo2 = (PVOID)ValueBuffer2;
@ -1798,15 +1811,27 @@ SmpCreateDynamicEnvironmentVariables(VOID)
PartialInfo, PartialInfo,
sizeof(ValueBuffer), sizeof(ValueBuffer),
&ResultLength); &ResultLength);
if (!NT_SUCCESS(Status)) if (!NT_SUCCESS(Status) ||
((PartialInfo->Type != REG_SZ) && (PartialInfo->Type != REG_EXPAND_SZ)))
{ {
NtClose(KeyHandle2); NtClose(KeyHandle2);
NtClose(KeyHandle); NtClose(KeyHandle);
DPRINT1("SMSS: Unable to read %wZ\\%wZ - %x\n", DPRINT1("SMSS: Unable to read %wZ\\%wZ (Type %lu, Status 0x%x)\n",
&DestinationString, &ValueName, Status); &DestinationString, &ValueName, PartialInfo->Type, Status);
return 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... */ /* As well as the vendor... */
RtlInitUnicodeString(&ValueName, L"VendorIdentifier"); RtlInitUnicodeString(&ValueName, L"VendorIdentifier");
Status = NtQueryValueKey(KeyHandle2, Status = NtQueryValueKey(KeyHandle2,
@ -1816,23 +1841,27 @@ SmpCreateDynamicEnvironmentVariables(VOID)
sizeof(ValueBuffer2), sizeof(ValueBuffer2),
&ResultLength); &ResultLength);
NtClose(KeyHandle2); 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 */ /* To combine it into a single string */
swprintf((PWCHAR)PartialInfo->Data + wcslen((PWCHAR)PartialInfo->Data), RtlStringCbPrintfW(DestinationString.Buffer + DestinationString.Length / sizeof(WCHAR),
L", %s", DestinationString.MaximumLength - DestinationString.Length,
(PWCHAR)PartialInfo2->Data); 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 */ /* So that we can set this as the PROCESSOR_IDENTIFIER variable */
RtlInitUnicodeString(&ValueName, L"PROCESSOR_IDENTIFIER"); 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, Status = NtSetValueKey(KeyHandle,
&ValueName, &ValueName,
0, 0,
REG_SZ, REG_SZ,
PartialInfo->Data, DestinationString.Buffer,
(wcslen((PWCHAR)PartialInfo->Data) + 1) * sizeof(WCHAR)); DestinationString.Length + sizeof(UNICODE_NULL));
if (!NT_SUCCESS(Status)) if (!NT_SUCCESS(Status))
{ {
DPRINT1("SMSS: Failed writing %wZ environment variable - %x\n", DPRINT1("SMSS: Failed writing %wZ environment variable - %x\n",
@ -1922,7 +1951,9 @@ SmpCreateDynamicEnvironmentVariables(VOID)
sizeof(ValueBuffer), sizeof(ValueBuffer),
&ResultLength); &ResultLength);
NtClose(KeyHandle2); 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 */ /* Convert from the integer value to the correct specifier */
RtlInitUnicodeString(&ValueName, L"SAFEBOOT_OPTION"); RtlInitUnicodeString(&ValueName, L"SAFEBOOT_OPTION");
@ -1957,7 +1988,8 @@ SmpCreateDynamicEnvironmentVariables(VOID)
} }
else 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);
} }
} }

2
base/system/smss/smsbapi.c
View file

@ -94,7 +94,7 @@ SmpSbCreateSession(IN PVOID Reserved,
NtClose(ProcessInformation->ProcessHandle); NtClose(ProcessInformation->ProcessHandle);
NtClose(ProcessInformation->ThreadHandle); NtClose(ProcessInformation->ThreadHandle);
SmpDereferenceSubsystem(KnownSubsys); SmpDereferenceSubsystem(KnownSubsys);
DbgPrint("SmpSbCreateSession: NtDuplicateObject (Thread) Failed %lx\n", Status); DPRINT1("SmpSbCreateSession: NtDuplicateObject (Thread) Failed %lx\n", Status);
return Status; return Status;
} }

2
base/system/smss/smss.h
View file

@ -31,6 +31,8 @@
#include <ndk/umfuncs.h> #include <ndk/umfuncs.h>
#include <ndk/kefuncs.h> #include <ndk/kefuncs.h>
#include <ntstrsafe.h>
/* SM Protocol Header */ /* SM Protocol Header */
#include <sm/smmsg.h> #include <sm/smmsg.h>

4
base/system/smss/smsubsys.c
View file

@ -656,8 +656,8 @@ SmpLoadSubSystemsForMuSession(IN PULONG MuSessionId,
if ((NT_SUCCESS(Status2)) && (InitialCommandBuffer[0])) if ((NT_SUCCESS(Status2)) && (InitialCommandBuffer[0]))
{ {
/* Put the debugger string with the Winlogon string */ /* Put the debugger string with the Winlogon string */
wcscat(InitialCommandBuffer, L" "); RtlStringCbCatW(InitialCommandBuffer, sizeof(InitialCommandBuffer), L" ");
wcscat(InitialCommandBuffer, InitialCommand->Buffer); RtlStringCbCatW(InitialCommandBuffer, sizeof(InitialCommandBuffer), InitialCommand->Buffer);
RtlInitUnicodeString(InitialCommand, InitialCommandBuffer); RtlInitUnicodeString(InitialCommand, InitialCommandBuffer);
} }
} }

12
base/system/smss/smutil.c
View file

@ -380,7 +380,7 @@ SmpParseCommandLine(IN PUNICODE_STRING CommandLine,
else else
{ {
/* Caller doesn't want flags, probably wants the image itself */ /* 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; UNICODE_STRING KeyName, ValueName;
OBJECT_ATTRIBUTES ObjectAttributes; OBJECT_ATTRIBUTES ObjectAttributes;
HANDLE KeyHandle; HANDLE KeyHandle;
ULONG Length;
WCHAR ValueBuffer[VALUE_BUFFER_SIZE]; WCHAR ValueBuffer[VALUE_BUFFER_SIZE];
PKEY_VALUE_PARTIAL_INFORMATION PartialInfo = (PVOID)ValueBuffer; PKEY_VALUE_PARTIAL_INFORMATION PartialInfo = (PVOID)ValueBuffer;
ULONG Length;
/* Open the key */ /* Open the key */
RtlInitUnicodeString(&KeyName, RtlInitUnicodeString(&KeyName,
@ -442,7 +442,7 @@ SmpQueryRegistrySosOption(VOID)
Status = NtOpenKey(&KeyHandle, KEY_READ, &ObjectAttributes); Status = NtOpenKey(&KeyHandle, KEY_READ, &ObjectAttributes);
if (!NT_SUCCESS(Status)) 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; return FALSE;
} }
@ -456,9 +456,11 @@ SmpQueryRegistrySosOption(VOID)
&Length); &Length);
ASSERT(Length < VALUE_BUFFER_SIZE); ASSERT(Length < VALUE_BUFFER_SIZE);
NtClose(KeyHandle); 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; return FALSE;
} }