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

[Gianluca Varenni]

----- Original Message ----- 
From: "Sebastian Gottschalk" <seppig_relay at gmx.de>
To: <winpcap-bugs at winpcap.org>
Sent: Friday, February 08, 2008 3:44 PM
Subject: [Winpcap-bugs] Bugs in WinPCap 4.1 beta 3 npf.sys driver


> 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 problem is just a memory leak. We are not acquiring any lock, it's just 
memory that is leaked.
I've checked in a fix for it.

>
> 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.

Uhm, I don't understand the problem. Can you point me out to the code that 
is affected by the problem?

>
> 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;
> }
>

No. I think you are talking about BIOCSETBUFFERSIZE. That IOCTL uses 
METHOD_BUFFERED. In this case, the I/O manager is responsible for
1. validating the buffer passed by the user
2. allocating a chunk of non-paged memory that fits the buffer passed by the 
user
3. copying the user buffer into the non-paged memory
4. putting a pointer to this non-paged memory into 
Irp->AssociatedIrp.SystemBuffer

This is clearly explained in the Oney book "Programming the Windows Driver 
Model 2nd edition" at page 488-489.

Have a nice day
GV


> 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
> _______________________________________________
> Winpcap-bugs mailing list
> Winpcap-bugs at winpcap.org
> https://www.winpcap.org/mailman/listinfo/winpcap-bugs