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

[Gianluca Varenni]

----- Original Message ----- 
From: "Sebastian Gottschalk" <seppig_relay at gmx.de>
To: <winpcap-bugs at winpcap.org>
Sent: Wednesday, February 13, 2008 3:12 PM
Subject: [Winpcap-bugs] security vulnerabilities in WinPCap 4.1 Beta 3


> 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),

No. We explicitely want to be able to open the same device multiple times, 
to allow multiple applications to capture from the same network device.

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

Not true. The Open instance is allocated per file object in the 
IRP_MJ_CREATE handler, and storing a pointer to it in the FsContext is the 
right way to store information that is associated to a specific file object 
and not to the device object.

The documentation about FILE_OBJECT here

http://msdn2.microsoft.com/en-us/library/aa906961.aspx

explains the use of FsContext quite clearly.


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

Those functions are for file system drivers. WinPcap is not a file system 
driver.

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

For several of these variables, I've actually spent a lot of reasoning as to 
whether was needed or not. Almost all those variables are 32bit properly 
aligned variables, and they are basically flags, so you don't actually need 
to lock in most cases (the operation is atomic on 32/64bit platforms is the 
variable is aligned on 32bit boundaries in memory). The lock would be needed 
if you need to atomically set two of these. I need to double check on it.

For Size, ReaderSN and WriterSN, yes, I need to double check, i might have 
spotted some race conditions.

ReadEvent: which is the scenario in which you might have a leak?

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

Please see the explanation I gave you in the other e-mails today.

Have a nice day
GV


>
> Sincerely,
> Sebastian Gottschalk
> _______________________________________________
> Winpcap-bugs mailing list
> Winpcap-bugs at winpcap.org
> https://www.winpcap.org/mailman/listinfo/winpcap-bugs