A few bugs to fix.

[JustasMasiulis]

General notes

• Pretty much every single IOCTL handler has incorrect input buffer checking.
  • size % sizeof(X) == 0 is true when size == 0, so passing an empty buffer to pretty much any of them will crash you.
  • None of the user supplied pointers in the buffers are are probed for length/alignment/actually being an UM pointer.
  • Data inside user supplied pointers is not copied and creates TOCTOU vulnerabilities.
• For none of the things you end up signature scanning, do you validate whether the dereferenced offsets make any sense or are even inside the image.
• Use mutexes consistently. Some of your APIs expect the caller to acquire the mutex, some of them acquire it themselves. There are no comments that indicate this and FAST_MUTEX is not recursive. IIRC there were places that the data wasn't protected by a mutex at all.
• What is the point of copying over every single signature to its own paged pool, instead of just using it directly (or at least to a stack buffer since size is known at compile time).
• You already use AutoLock. Same thing can be done for 90% of the allocations to avoid the goto Cleanup spam.
• There are no SAL annotations for anything.

Bugs list

• Incorrect iteration / removal from sparse array.

  • If you're iterating an array and decrementing its size while doing that, you'll miss/leak half of the entries.
  • If the array is sparse and you remove not from the end, decrementing the size will make you miss/leak last entry.
    

    Nidhogg/Nidhogg/AntiAnalysis.cpp

    Lines 759 to 768 in d8697ca

    	for (ULONG i = 0; i < this->DisabledCallbacksCount; i++) {
    	if (this->DisabledCallbacks[i].CallbackAddress == Callback->CallbackAddress) {
    	DisabledCallback->CallbackAddress = this->DisabledCallbacks[i].CallbackAddress;
    	DisabledCallback->Entry = this->DisabledCallbacks[i].Entry;
    	DisabledCallback->Type = this->DisabledCallbacks[i].Type;
    	this->DisabledCallbacksCount--;
    	status = STATUS_SUCCESS;
    	break;
    	}
    	}

    
    

    Nidhogg/Nidhogg/FileUtils.cpp

    Lines 16 to 20 in d8697ca

    	for (ULONG i = 0; i < this->Files.FilesCount; i++) {
    	ExFreePoolWithTag(this->Files.FilesPath[i], DRIVER_TAG);
    	this->Files.FilesPath[i] = nullptr;
    	this->Files.FilesCount--;
    	}

    
    

    Nidhogg/Nidhogg/FileUtils.cpp

    Lines 194 to 200 in d8697ca

    	for (ULONG i = 0; i < this->Files.FilesCount; i++)
    	if (_wcsicmp(this->Files.FilesPath[i], path) == 0) {
    	ExFreePoolWithTag(this->Files.FilesPath[i], DRIVER_TAG);
    	this->Files.FilesPath[i] = nullptr;
    	this->Files.FilesCount--;
    	return true;
    	}

    
    

    Nidhogg/Nidhogg/MemoryUtils.cpp

    Lines 16 to 18 in d8697ca

    	for (ULONG i = 0; i < this->hiddenDrivers.Count; i++) {
    	RemoveHiddenDriver(i);
    	}

    
    

    Nidhogg/Nidhogg/MemoryUtils.cpp

    Lines 1268 to 1277 in d8697ca

    	for (ULONG i = 0; i < this->hiddenDrivers.Count; i++) {
    	if (this->hiddenDrivers.Items[i].DriverName != nullptr)
    	if (_wcsicmp(this->hiddenDrivers.Items[i].DriverName, item.DriverName) == 0) {
    	ExFreePoolWithTag(this->hiddenDrivers.Items[i].DriverName, DRIVER_TAG);
    	this->hiddenDrivers.Items[i].DriverName = NULL;
    	this->hiddenDrivers.Items[i].originalEntry = NULL;
    	this->hiddenDrivers.Count--;
    	return true;
    	}
    	}

    
    

    Nidhogg/Nidhogg/ProcessUtils.cpp

    Lines 304 to 311 in d8697ca

    	for (ULONG i = 0; i < this->HiddenProcesses.PidsCount; i++) {
    	if (this->HiddenProcesses.Processes[i].Pid == pid) {
    	entry = this->HiddenProcesses.Processes[i].ListEntry;
    	this->HiddenProcesses.Processes[i].Pid = 0;
    	this->HiddenProcesses.PidsCount--;
    	break;
    	}
    	}

    
    

    Nidhogg/Nidhogg/ProcessUtils.cpp

    Lines 420 to 425 in d8697ca

    	for (ULONG i = 0; i < this->ProtectedThreads.TidsCount; i++)
    	if (this->ProtectedThreads.Threads[i] == tid) {
    	this->ProtectedThreads.Threads[i] = 0;
    	this->ProtectedThreads.TidsCount--;
    	return true;
    	}

• Comparing thread ID with thread object.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 1168 in d8697ca

  if (info->Threads[i].ClientId.UniqueThread == PsGetCurrentThread())

• Missing buffer size check and assumption that an UNICODE_STRING will be null terminated. MAX_PATH Buffers and assumption of C disk isn't something you'd expect from a driver either.
  

  Nidhogg/Nidhogg/FileUtils.cpp

  Lines 39 to 48 in d8697ca

  	WCHAR fullPath[MAX_PATH + 1] = DEFAULT_DRIVE_LETTER;
  	auto stack = IoGetCurrentIrpStackLocation(Irp);
  	if (!stack || !stack->FileObject)
  	return ((tNtfsIrpFunction)NidhoggFileUtils->GetNtfsCallback(0).Address)(DeviceObject, Irp);
  	if (stack->FileObject->FileName.Length == 0)
  	return ((tNtfsIrpFunction)NidhoggFileUtils->GetNtfsCallback(0).Address)(DeviceObject, Irp);
  	wcscat(fullPath, stack->FileObject->FileName.Buffer);

• Dereferencing object you have not acquired if PsLookupThreadByThreadId is unsuccessful.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Lines 1171 to 1177 in d8697ca

  	status = PsLookupThreadByThreadId(info->Threads[i].ClientId.UniqueThread, Thread);
  	if (!NT_SUCCESS(status) || PsIsThreadTerminating(*Thread)) {
  	ObDereferenceObject(*Thread);
  	*Thread = NULL;
  	continue;
  	}

• Incorrect size for terminating character.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 1234 in d8697ca

  WCHAR* buffer = (WCHAR*)ExAllocatePoolWithTag(PagedPool, wcslen(item.DriverName) * sizeof(WCHAR) + 1, DRIVER_TAG);

• Missing removal of hooks on driver unload.
  

  Nidhogg/Nidhogg/AntiAnalysis.cpp

  Line 14 in d8697ca

  AntiAnalysis::~AntiAnalysis() {

• Object is not dereferenced anywhere.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 187 in d8697ca

  status = FindAlertableThread(pid, &TargetThread);

  
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 425 in d8697ca

  status = PsLookupProcessByProcessId(ULongToHandle(ModuleInformation->Pid), &targetProcess);

• Missing output buffer length validation for the callbacks array that is populated from an unbounded list.
  

  Nidhogg/Nidhogg/AntiAnalysis.cpp

  Lines 396 to 409 in d8697ca

  	for (ULONG i = 0; i < callbacksListCount; i++) {
  	if (ReplacedFunction && ReplacerFunction) {
  	if (currentCallback->Function == ReplacedFunction)
  	currentCallback->Function = ReplacerFunction;
  	}
  	else {
  	Callbacks->Callbacks[i].CallbackAddress = (ULONG64)currentCallback->Function;
  	Callbacks->Callbacks[i].Context = currentCallback->Context;
  	if (NT_SUCCESS(MatchCallback((PVOID)Callbacks->Callbacks[i].CallbackAddress, driverName)))
  	strcpy_s(Callbacks->Callbacks[i].DriverName, driverName);
  	}
  	currentCallback = (PCM_CALLBACK)currentCallback->List.Flink;
  	}

  
  

  Nidhogg/Nidhogg/AntiAnalysis.cpp

  Lines 568 to 582 in d8697ca

  	for (SIZE_T i = 0; i < routinesCount; i++) {
  	currentRoutine = *(PULONG64)(routinesList + (i * 8));
  	currentRoutine &= ~(1ULL << 3) + 1;
  	if (ReplacedFunction && ReplacerFunction) {
  	if (*(PULONG64)(currentRoutine) == ReplacedFunction)
  	*(PULONG64)(currentRoutine) = ReplacerFunction;
  	}
  	else {
  	Callbacks->Routines[i].CallbackAddress = *(PULONG64)(currentRoutine);
  	if (NT_SUCCESS(MatchCallback((PVOID)Callbacks->Routines[i].CallbackAddress, driverName)))
  	strcpy_s(Callbacks->Routines[i].DriverName, driverName);
  	}
  	}

• Missing null checks.
  

  Nidhogg/Nidhogg/Nidhogg.cpp

  Lines 210 to 214 in d8697ca

  	NidhoggProccessUtils = new ProcessUtils();
  	NidhoggFileUtils = new FileUtils();
  	NidhoggMemoryUtils = new MemoryUtils();
  	NidhoggAntiAnalysis = new AntiAnalysis();
  	NidhoggRegistryUtils = new RegistryUtils();

• Missing lock for iteration of PsLoadedModuleList.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 497 in d8697ca

  for (PLIST_ENTRY pListEntry = PsLoadedModuleList->InLoadOrderLinks.Flink;

  
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 545 in d8697ca

  PLIST_ENTRY pListEntry = PsLoadedModuleList->InLoadOrderLinks.Flink;

• Missing address space / VAD locks.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 672 in d8697ca

  NTSTATUS MemoryUtils::VadHideObject(PEPROCESS Process, ULONG_PTR TargetAddress) {

• Trusting usermode PEB pointers to be valid or for the address space to be intact at all.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 454 in d8697ca

  for (PLIST_ENTRY pListEntry = targetPeb->LoaderData->InLoadOrderModuleList.Flink;

  
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 822 in d8697ca

  for (PLIST_ENTRY pListEntry = targetPeb->LoaderData->InLoadOrderModuleList.Flink;

• Trusting any RVA to be correct while parsing PE headers or for the pointer to be valid at all.
  

  Nidhogg/Nidhogg/MemoryUtils.cpp

  Line 850 in d8697ca

  PVOID MemoryUtils::GetFunctionAddress(PVOID moduleBase, CHAR* functionName) {

• All allocated features are leaked in case of failure in driver entry.
  

  Nidhogg/Nidhogg/Nidhogg.cpp

  Line 46 in d8697ca

  InitializeFeatures();

• MmHighestUserAddress, MmUserProbeAddress, MmSystemRangeStart are a better choice. Not that this really does any useful validation.
  

  Nidhogg/Nidhogg/pch.h

  Lines 9 to 11 in d8697ca

  	#define VALID_USERMODE_MEMORY(MemAddress)(MemAddress > 0 && MemAddress < 0x7FFFFFFFFFFFFFFF)
  	#define VALID_KERNELMODE_MEMORY(MemAddress)(MemAddress > 0x8000000000000000 && MemAddress < 0xFFFFFFFFFFFFFFFF)
  	#define VALID_ADDRESS(MemAddress)(VALID_USERMODE_MEMORY(MemAddress) || VALID_KERNELMODE_MEMORY(MemAddress))

• Operator new is not allowed to return null, consider using std::nothrow version instead. The opposite is true for operator delete where it allows being passed in a null pointer, but ExFreePoolWithTag doesn't.
  

  Nidhogg/Nidhogg/AntiAnalysis.hpp

  Lines 119 to 121 in d8697ca

  	void* operator new(size_t size) {
  	return ExAllocatePoolWithTag(NonPagedPool, size, DRIVER_TAG);
  	}

• Missing NTSTATUS check.
  

  Nidhogg/Nidhogg/AntiAnalysis.cpp

  Line 72 in d8697ca

  MmCopyVirtualMemory(PsGetCurrentProcess(), &this->PrevEtwTiValue, PsGetCurrentProcess(), &enableProviderInfo->IsEnabled, sizeof(ULONG), KernelMode, &bytesWritten);