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

[Gianluca Varenni]

Sebastian,

I will look into the problems you pointed out in the next week or so and get 
back to you.


Thanks for the report
GV

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