[Winpcap-bugs] security vulnerabilities in WinPCap 4.1 Beta 3

[Sebastian Gottschalk]

Dear Sir or Madam,

I completed my analysis of WinPCap 4.1 Beta 3 and found 3 major problems in 
at least 17 functions, aside from those I had already reported to you.

Problem 1: NPF_Open doesn't check whether a device has already been opened, 
and since createDevice() creates devices with non-exclusive access (even 
though exclusive access would have to be enforced by the driver anyway), 
opening a device more than once causes the structure 'Open' to be 
initialized more than once, always leaking the memory, spinlock and event 
resources associated with it. Since NPF_Close doesn't care either, opening a 
device twice and closing it just once makes 'Open' effectively become a 
dangling pointer, which will mess up NPF_Cleanup. Even further, closing the 
device makes IrpSp->DeviceExtension->FsContext an undefined variable, and 
since this function read 'Open' from there another memory leak and another 
dangling pointer may occur.

Microsoft recommends as a solution to keep track of all FsContext in 
NPF_Open and rejecting all duplicates. They also provide two functions 
(FsRtlLookupPerStreamContext, FsRtlInsertPerStreamContext) to handle this 
issue on behalf of the driver, which are available on Windows 2000 SP4 + 
Rollup and Windows XP and later.

Problem 2: The following fields of 'Open' aren't locked and therefore not 
properly synchronized: Mode, SkipSentPackets, ReadEvent, ReaderSN, WriterSN, 
Size, Timeout, NWrites, MinToCopy. At least 'Size' leads to a race condition 
and into a buffer overflow, 'ReadEvent' causes a leak of event resource.

Problem 3: Irp->AssociatedIrp->SystemBuffer is almost never validated, and 
even if so, totally improperly. Only NP_IoControl!BIOCGSTATS does a little 
bit of validation, but utilizes memory locking (causing a Denial of Service 
if a sufficiently large buffer is provided) and fails to catch the exception 
EXCEPTION_DATATYPE_MISALIGNMENT. It also validates in cases where no 
validation would be necessary (f.e. if it's called by a lower level filter 
driver that has already validated the buffer). NPF_Read seems to do 
something that might be intended as validation, but surely isn't.

This is quite bad since proper and correct validation can be actually very 
simple and performant, about like this:

include <rx.h> // for FlagOn macro
IrpSp = IoGetIrpStackLocation(Irp);
PWHATEVER foo = (PWHATEVER)Irp->AssociatedIrp.SystemBuffer;
__try {
	if (	(Irp->RequestorMode == UserMode) || FlagOn(irpSp->Flags,
		SL_FORCE_ACCESS_CHECK))
		ProbeForRead((PWHATEVER)foo,sizeof(WHATEVER)
				,TYPE_ALIGNMENT(WHATEVER));
		// use ProbeForWrite for write access
	// all subsequent read/write access must be within the try/except
	ULONG u = foo->field;

} __except(	(GetExceptionCode() == STATUS_ACCESS_VIOLATION) ||
		(GetExceptionCode() == STATUS_DATATYPE_MISALIGNMENT) ?
		 EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH)
		// if the exception was not our fault, then propagate it
{
	// cleanup
	IoCompleteRequest(Irp, IO_NO_INCREMENT);
	return STATUS_INVALID_PARAMETER;
}


Sincerely,
Sebastian Gottschalk