https://git.reactos.org/?p=reactos.git;a=commitdiff;h=f909e8762d18825b1ba21b...
commit f909e8762d18825b1ba21ba340e0f394a1c93d64 Author: George Bișoc george.bisoc@reactos.org AuthorDate: Tue Nov 9 21:34:36 2021 +0100 Commit: George Bișoc george.bisoc@reactos.org CommitDate: Tue Nov 16 10:55:44 2021 +0100
[NTOS:SE] Validate the SID lengths when capturing them
SIDs are variadic by nature which means their lengths can vary in a given amount of time and certain factors that allow for this happen. This also especially can lead to issues when capturing SIDs and attributes because SeCaptureSidAndAttributesArray might end up overwriting the buffer during the time it's been called.
Therefore when we're copying the SIDs, validate their lengths. In addition to that, update the documentation header accordingly and add some debug prints in code. --- ntoskrnl/include/internal/tag.h | 1 + ntoskrnl/se/sid.c | 138 +++++++++++++++++++++++++++++++++------- 2 files changed, 116 insertions(+), 23 deletions(-)
diff --git a/ntoskrnl/include/internal/tag.h b/ntoskrnl/include/internal/tag.h index b92b4e99edf..38c26245023 100644 --- a/ntoskrnl/include/internal/tag.h +++ b/ntoskrnl/include/internal/tag.h @@ -186,6 +186,7 @@ #define TAG_LOGON_SESSION 'sLeS' #define TAG_LOGON_NOTIFICATION 'nLeS' #define TAG_SID_AND_ATTRIBUTES 'aSeS' +#define TAG_SID_VALIDATE 'vSeS'
/* LPC Tags */ #define TAG_LPC_MESSAGE 'McpL' diff --git a/ntoskrnl/se/sid.c b/ntoskrnl/se/sid.c index 8d9e701f245..509b3777488 100644 --- a/ntoskrnl/se/sid.c +++ b/ntoskrnl/se/sid.c @@ -52,6 +52,12 @@ PSID SeAnonymousLogonSid = NULL; PSID SeLocalServiceSid = NULL; PSID SeNetworkServiceSid = NULL;
+typedef struct _SID_VALIDATE +{ + UCHAR SubAuthorityCount; + PISID ProbeSid; +} SID_VALIDATE, *PSID_VALIDATE; + /* FUNCTIONS ******************************************************************/
/** @@ -450,8 +456,29 @@ SepReleaseSid( * avoiding integer overflows. STATUS_INSUFFICIENT_RESOURCES * is returned if memory pool allocation for the captured SID has failed. * STATUS_BUFFER_TOO_SMALL is returned if the length of the allocated - * buffer is less than the required size. A failure NTSTATUS code is - * returned otherwise. + * buffer is less than the required size. STATUS_INVALID_SID is returned + * if a SID doesn't meet certain requirements to be considered a valid + * SID by the security manager. A failure NTSTATUS code is returned + * otherwise. + * + * @remarks + * A security identifier (SID) is a variable-length data structure that + * can change in size over time, depending on the factors that influence + * this effect in question. An attacker can take advantage of this fact + * and can potentially modify certain properties of a SID making it + * different in size than it was originally before. This is what we'd + * call, a TOCTOU vulnerability. + * + * For this reason, the logic of copying the SIDs and their attributes + * into a new buffer goes like this: first, allocate a buffer array + * that just holds the lengths and subauthority count of each SID. + * Such information is copied in the first loop. Then in a second loop, + * iterate over the array with SID provided and copy them into the final + * array. The moment we're doing this, validate the lengths of each SID + * basing upon the captured lengths we've got before. In this way we + * ensure that the SIDs have remained intact. The validation checks are + * done in user mode as a general rule that we just cannot trust UM and + * whatever data is coming from it. */ NTSTATUS NTAPI @@ -467,12 +494,16 @@ SeCaptureSidAndAttributesArray( _Out_ PULONG ResultLength) { ULONG ArraySize, RequiredLength, SidLength, i; + ULONG TempArrayValidate, TempLengthValidate; PSID_AND_ATTRIBUTES SidAndAttributes; + _SEH2_VOLATILE PSID_VALIDATE ValidateArray; PUCHAR CurrentDest; PISID Sid; NTSTATUS Status; PAGED_CODE();
+ ValidateArray = NULL; + SidAndAttributes = NULL; *CapturedSidAndAttributes = NULL; *ResultLength = 0;
@@ -483,6 +514,7 @@ SeCaptureSidAndAttributesArray(
if (AttributeCount > SE_MAXIMUM_GROUP_LIMIT) { + DPRINT1("SeCaptureSidAndAttributesArray(): Maximum group limit exceeded!\n"); return STATUS_INVALID_PARAMETER; }
@@ -495,14 +527,28 @@ SeCaptureSidAndAttributesArray( ArraySize = AttributeCount * sizeof(SID_AND_ATTRIBUTES); RequiredLength = ALIGN_UP_BY(ArraySize, sizeof(ULONG));
- /* Check for user mode data */ if (PreviousMode != KernelMode) { + /* Check for user mode data */ _SEH2_TRY { /* First probe the whole array */ ProbeForRead(SrcSidAndAttributes, ArraySize, sizeof(ULONG));
+ /* We're in user mode, set up the size for the temporary array */ + TempArrayValidate = AttributeCount * sizeof(SID_VALIDATE); + TempLengthValidate = ALIGN_UP_BY(TempArrayValidate, sizeof(ULONG)); + + /* + * Allocate a buffer for the array that we're going to + * temporarily hold the subauthority count and the SID + * elements. We'll be going to use this array to perform + * validation checks later. + */ + ValidateArray = ExAllocatePoolWithTag(PoolType | POOL_RAISE_IF_ALLOCATION_FAILURE, + TempLengthValidate, + TAG_SID_VALIDATE); + /* Loop the array elements */ for (i = 0; i < AttributeCount; i++) { @@ -510,16 +556,20 @@ SeCaptureSidAndAttributesArray( Sid = SrcSidAndAttributes[i].Sid; ProbeForRead(Sid, sizeof(*Sid), sizeof(ULONG));
- /* Verify that the SID is valid */ - if (((Sid->Revision & 0xF) != SID_REVISION) || - (Sid->SubAuthorityCount > SID_MAX_SUB_AUTHORITIES)) - { - _SEH2_YIELD(return STATUS_INVALID_SID); - } + /* + * Capture the subauthority count and hold it + * into the temporary array for later validation. + * This way we ensure that the said count of each + * SID has remained the same. + */ + ValidateArray[i].SubAuthorityCount = Sid->SubAuthorityCount; + + /* Capture the SID */ + ValidateArray[i].ProbeSid = Sid;
/* Calculate the SID length and probe the full SID */ - SidLength = RtlLengthRequiredSid(Sid->SubAuthorityCount); - ProbeForRead(Sid, SidLength, sizeof(ULONG)); + SidLength = RtlLengthRequiredSid(ValidateArray[i].SubAuthorityCount); + ProbeForRead(ValidateArray[i].ProbeSid, SidLength, sizeof(ULONG));
/* Add the aligned length to the required length */ RequiredLength += ALIGN_UP_BY(SidLength, sizeof(ULONG)); @@ -527,7 +577,8 @@ SeCaptureSidAndAttributesArray( } _SEH2_EXCEPT(EXCEPTION_EXECUTE_HANDLER) { - _SEH2_YIELD(return _SEH2_GetExceptionCode()); + Status = _SEH2_GetExceptionCode(); + _SEH2_YIELD(goto Cleanup); } _SEH2_END; } @@ -558,7 +609,9 @@ SeCaptureSidAndAttributesArray( TAG_SID_AND_ATTRIBUTES); if (SidAndAttributes == NULL) { - return STATUS_INSUFFICIENT_RESOURCES; + DPRINT1("SeCaptureSidAndAttributesArray(): Failed to allocate memory for SID and attributes array (requested size -> %lu)!\n", RequiredLength); + Status = STATUS_INSUFFICIENT_RESOURCES; + goto Cleanup; } } /* Otherwise check if the buffer is large enough */ @@ -570,7 +623,9 @@ SeCaptureSidAndAttributesArray( else { /* Buffer is too small, fail */ - return STATUS_BUFFER_TOO_SMALL; + DPRINT1("SeCaptureSidAndAttributesArray(): The provided buffer is small (expected size -> %lu || current size -> %lu)!\n", RequiredLength, AllocatedLength); + Status = STATUS_BUFFER_TOO_SMALL; + goto Cleanup; }
*CapturedSidAndAttributes = SidAndAttributes; @@ -587,20 +642,50 @@ SeCaptureSidAndAttributesArray( /* Loop the array elements */ for (i = 0; i < AttributeCount; i++) { - /* Get the SID and it's length */ - Sid = SrcSidAndAttributes[i].Sid; - SidLength = RtlLengthRequiredSid(Sid->SubAuthorityCount); + /* + * Get the SID length from the subauthority + * count we've captured before. + */ + SidLength = RtlLengthRequiredSid(ValidateArray[i].SubAuthorityCount);
/* Copy attributes */ SidAndAttributes[i].Attributes = SrcSidAndAttributes[i].Attributes;
/* Copy the SID to the current destination address */ SidAndAttributes[i].Sid = (PSID)CurrentDest; - RtlCopyMemory(CurrentDest, SrcSidAndAttributes[i].Sid, SidLength); + RtlCopyMemory(CurrentDest, ValidateArray[i].ProbeSid, SidLength);
- /* Sanity checks */ - ASSERT(RtlLengthSid(SidAndAttributes[i].Sid) == SidLength); - ASSERT(RtlValidSid(SidAndAttributes[i].Sid)); + /* Obtain the SID we've captured before for validation */ + Sid = SidAndAttributes[i].Sid; + + /* Validate that the subauthority count hasn't changed */ + if (ValidateArray[i].SubAuthorityCount != + Sid->SubAuthorityCount) + { + /* It's changed, bail out */ + DPRINT1("SeCaptureSidAndAttributesArray(): The subauthority counts have changed (captured count -> %u || current count -> %u)\n", + ValidateArray[i].SubAuthorityCount, Sid->SubAuthorityCount); + Status = STATUS_INVALID_SID; + goto Cleanup; + } + + /* Validate that the SID length is the same */ + if (SidLength != RtlLengthSid(Sid)) + { + /* They're no longer the same, bail out */ + DPRINT1("SeCaptureSidAndAttributesArray(): The SID lengths have changed (captured length -> %lu || current length -> %lu)\n", + SidLength, RtlLengthSid(Sid)); + Status = STATUS_INVALID_SID; + goto Cleanup; + } + + /* Check that the SID is valid */ + if (!RtlValidSid(Sid)) + { + DPRINT1("SeCaptureSidAndAttributesArray(): The SID is not valid!\n"); + Status = STATUS_INVALID_SID; + goto Cleanup; + }
/* Update the current destination address */ CurrentDest += ALIGN_UP_BY(SidLength, sizeof(ULONG)); @@ -637,18 +722,25 @@ SeCaptureSidAndAttributesArray( } }
+Cleanup: /* Check for failure */ if (!NT_SUCCESS(Status)) { /* Check if we allocated a new array */ - if (SidAndAttributes != AllocatedMem) + if ((SidAndAttributes != AllocatedMem) && (SidAndAttributes != NULL)) { /* Free the array */ ExFreePoolWithTag(SidAndAttributes, TAG_SID_AND_ATTRIBUTES); }
/* Set returned address to NULL */ - *CapturedSidAndAttributes = NULL ; + *CapturedSidAndAttributes = NULL; + } + + /* Free the temporary validation array */ + if ((PreviousMode != KernelMode) && (ValidateArray != NULL)) + { + ExFreePoolWithTag(ValidateArray, TAG_SID_VALIDATE); }
return Status;
-
George Bișoc