[Winpcap-bugs] Bugs in WinPCap 4.1 beta 3 npf.sys driver

[Sebastian Gottschalk]

Dear Sir or Madam,

I found two memory leaks and one vulnerability in the NPF.SYS driver.

One issue is within getTcpBindings(). It allocates memory for the variable 
'valueInfoP' to hold a result from a registry query. If the call is not 
successful, doesn't return the expected length or type, the memory is not 
freed. Since the registry key is only writable by administrators, this is 
generally not exploitable... almost, since there's a known vulnerability in 
all recent Windows versions which allow an attacker to deny the system from 
reading a registry key by aquiring multiple locks on arbitray registry keys 
until the lock limit pulls in. At any rate, even without this bugs it's 
clearly a stability issue.
The solution is obviously to free the memory in the error conditions.

The second issue is highly hypothetical, since it would require an extremely 
large amount of CPUs in the system, such that &Open + nCpu * 
sizeof(OPEN_INSTANCE) results in an integer overflow. Then 
NdisAqquireSpinLock and NdisReleaseSpinLock would result in an array 
underflow, which in turn would result in a memory access violation or random 
kernel memory corruption. Since none of the parameters is under control of 
the user, this is not exploitable.

However, at the very same place one can see that 'dim' is obtained by 
dereferencing Irp->AssociatedIrp.SystemBuffer. The size of this buffer is 
checked, however it's not checked whether the memory is actually mapped and 
readable. Since this is under the control of the user, this would result in 
an invalid memory access and therefore to a BSOD with ACCESS_VIOLATION.
The solution is to wrap the entire access within try/except and use the 
ProbeForRead macro. About so:

__try {
	if ((Irp->RequestorMode == UserMode) || FlagOn(IrpSp->Flags,
						SL_FORCE_ACCESS_CHECK))
		ProbeForRead(dim,sizeof(ULONG),TYPE_ALIGNMENT(ULONG));
	dim = *((PULONG)Irp->AssociatedIrp.SystemBuffer);
} __except((GetExceptionCode() == STATUS_ACCESS_VIOLATION) ||
	   (GetExceptionCode() == STATUS_DATATYPE_MISALIGNMENT) ?
	    EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
	IoCompleteRequest(Irp, IO_NO_INCREMENT);
	return STATUS_INVALID_PARAMETER;
}

For all other warnings from PreFast I can assert that they're unjustified, 
so you may safely add suppress annotations. I haven't done any further 
checks for parameter validation problems though, so I'll report back when 
I'm done.

Sincerely,
Sebastian Gottschalk