diff --git a/CHANGELOG.md b/CHANGELOG.md index 0dfbf52..f35f844 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ Line wrap the file at 100 chars. Th * **Security**: in case of vulnerabilities. ## [Unreleased] +### Fixed +Correct unfortunate application of NT_ASSERT, the illustrious. Critical logic that updates the + process tree was being omitted from release builds. ## [1.1.0.0] - 2021-05-26 ### Changed diff --git a/src/procmgmt/procmgmt.cpp b/src/procmgmt/procmgmt.cpp index ec89fc2..01062a7 100644 --- a/src/procmgmt/procmgmt.cpp +++ b/src/procmgmt/procmgmt.cpp @@ -13,6 +13,56 @@ namespace procmgmt namespace { +// +// EmitAddEntryErrorEvents() +// +// During process arrival event processing, it may happen that `procregistry::AddEntry()` +// fails to complete successfully. +// +// This has to be communicated through an event. +// +// Additionally, if the arriving process would have been split, then this is a compound +// failure that is communicated separately. +// +void +EmitAddEntryErrorEvents +( + eventing::CONTEXT *Eventing, + NTSTATUS ErrorCode, + HANDLE ProcessId, + LOWER_UNICODE_STRING *ImageName, + bool EmitSplittingEvent +) +{ + NT_ASSERT(!NT_SUCCESS(ErrorCode)); + + DbgPrint("Failed to add entry for arriving process\n"); + DbgPrint(" Status: 0x%X\n", ErrorCode); + DbgPrint(" PID: %p\n", ProcessId); + DbgPrint(" Imagename: %wZ\n", (UNICODE_STRING*)ImageName); + + util::StopIfDebugBuild(); + + DECLARE_CONST_UNICODE_STRING(errorMessage, L"Failed in call to procregistry::AddEntry()"); + + auto errorEvent = eventing::BuildErrorMessageEvent(ErrorCode, &errorMessage); + + eventing::Emit(Eventing, &errorEvent); + + if (!EmitSplittingEvent) + { + return; + } + + auto splittingErrorEvent = eventing::BuildStartSplittingErrorEvent + ( + ProcessId, + ImageName + ); + + eventing::Emit(Eventing, &splittingErrorEvent); +} + // // ValidateCollision() // @@ -159,10 +209,6 @@ HandleProcessArriving const procmon::PROCESS_EVENT *Record ) { - //DbgPrint("Process arriving: 0x%X\n", Record->ProcessId); - //DbgPrint(" Parent: 0x%X\n", Record->Details->ParentProcessId); - //DbgPrint(" Path: %wZ\n", Record->Details->Path); - // // State lock is held and is locking out IOCTL handlers. // @@ -186,8 +232,10 @@ HandleProcessArriving if (!NT_SUCCESS(status)) { - DbgPrint("Failed to initialize entry for arriving process: status 0x%X\n", status); - DbgPrint(" PID of arriving process %p\n", Record->ProcessId); + DbgPrint("Failed to initialize entry for arriving process\n"); + DbgPrint(" Status: 0x%X\n", status); + DbgPrint(" PID: %p\n", Record->ProcessId); + DbgPrint(" Imagename: %wZ\n", &(Record->Details->ImageName)); return; } @@ -195,7 +243,8 @@ HandleProcessArriving ArrivalEvent arrivalEvent = { .SplittingReason = ST_SPLITTING_REASON_PROCESS_ARRIVING, - .EmitEvent = false + .EmitEvent = false, + .Imagename = { 0, 0, NULL } }; if (Context->EngagedStateActive(Context->CallbackContext)) @@ -248,17 +297,12 @@ HandleProcessArriving // event will already have been emitted. // - auto validationStatus = ValidateCollision(Context, ®istryEntry); + const auto validationStatus = ValidateCollision(Context, ®istryEntry); - if (!validationStatus && arrivalEvent.EmitEvent) + if (!validationStatus) { - auto splittingErrorEvent = eventing::BuildStartSplittingErrorEvent - ( - Record->ProcessId, - ®istryEntry.ImageName - ); - - eventing::Emit(Context->Eventing, &splittingErrorEvent); + EmitAddEntryErrorEvents(Context->Eventing, status, registryEntry.ProcessId, + ®istryEntry.ImageName, arrivalEvent.EmitEvent); } procregistry::ReleaseEntry(®istryEntry); @@ -269,19 +313,8 @@ HandleProcessArriving // General error handling. // - DbgPrint("Failed to add entry for arriving process: status 0x%X.\n", status); - DbgPrint(" PID of arriving process %p\n", Record->ProcessId); - - if (arrivalEvent.EmitEvent) - { - auto splittingErrorEvent = eventing::BuildStartSplittingErrorEvent - ( - Record->ProcessId, - ®istryEntry.ImageName - ); - - eventing::Emit(Context->Eventing, &splittingErrorEvent); - } + EmitAddEntryErrorEvents(Context->Eventing, status, registryEntry.ProcessId, + ®istryEntry.ImageName, arrivalEvent.EmitEvent); procregistry::ReleaseEntry(®istryEntry); } @@ -414,9 +447,25 @@ HandleProcessDeparting WdfSpinLockAcquire(processRegistry->Lock); - NT_ASSERT(procregistry::DeleteEntry(processRegistry->Instance, registryEntry)); + const bool deleteSuccessful = procregistry::DeleteEntry(processRegistry->Instance, registryEntry); WdfSpinLockRelease(processRegistry->Lock); + + NT_ASSERT(deleteSuccessful); + + // + // This is unlikely to ever be an issue, + // but if it was, we'd want to know about it. + // + + if (!deleteSuccessful) + { + DECLARE_CONST_UNICODE_STRING(errorMessage, L"Failed in call to procregistry::DeleteEntry()"); + + auto errorEvent = eventing::BuildErrorMessageEvent(STATUS_UNSUCCESSFUL, &errorMessage); + + eventing::Emit(Context->Eventing, &errorEvent); + } } void