[Winpcap-users] WinPCap driver BIOCSETOID / BIOCQUERYOID bug

[Gianluca Varenni]

--------------------------------------------------
From: ""Fish" (David B. Trout)" <fish at infidels.org>
Sent: Thursday, June 03, 2010 8:16 AM
To: <winpcap-users at winpcap.org>
Subject: [Winpcap-users] WinPCap driver BIOCSETOID / BIOCQUERYOID bug

> WinPCap version: 4.1.1
> Source member:   Packet.c
> Function:        NPF_IoControl
> Case:            BIOCSETOID / BIOCQUERYOID
> Statement/line:  1532
> Discussion:
>
> The 'if' statement is enforcing the artificial restriction that the
> DeviceIoControl caller's input and output buffers be the same size. As
> currently designed the caller might as well pass the exact same buffer for
> both input and the output.
>
> This is silly.
>
> DeviceIoControl has always supported completely separate input and output
> buffers and thus completely different input and output buffer sizes.
>
> The patch is attached.
>
> PLEASE NOTE: the patch modifies BIOCSETOID / BIOCQUERYOID so that only the
> object id (OID) needs to be passed [to WinPCap] in the input buffer (the
> buffer size to be passed in the NDIS request is implied based on the sizes
> of the input or output buffers passed to WinPCap) and thus DOES BREAK
> EXISTING BIOCSETOID / BIOCQUERYOID CODE.
>
> THEREFORE to be technically correct my "fix" should be considered as 
> simply
> illustrative of the way new [to be #defined] BIOCSETOID_EX / 
> BIOCQUERYOID_EX
> ioctls should behave only, and NOT an actual per se fix to existing code.
> Rather, my patch defines how two new proposed as-yet-to-be-determined 
> ioctl
> codes should behave.

I think that the right fix is this one i.e.

1. create a new set of IOCTLs that allow separate buffers to be passed for 
input and output and
2. add a new Packet API (like PacketRequestEx) that uses the new IOCTL 
codes.

>
> The idea here is all that a user needs to do to set/query an OID should be
> as simple as:
>
>
>  BYTE queryoidbuf[bufsiz];
>  DWORD Oid = OID_WHATEVER;
>  DeviceIoControl(handle, BIOCQUERYOID, &Oid, sizeof(Oid), queryoidbuf,
> bufsiz, ... );
>
>
> and:
>
>
>  BYTE setoidbuff[bufsiz];
>  *((DWORD*)setoidbuff) = OID_WHATEVER;
>  memcpy(&setoidbuff[sizeof(DWORD)], whatever, bufsiz - sizeof(DWORD));
>  DeviceIoControl(handle, BIOCSETOID, setoidbuff, bufsiz, NULL, 0, ...);
>

Which is something that a WinPcap-based application developer should never 
do. The interface between Packet.dll and the driver is not documented and is 
subject to change at any time (as a matter of facts, this holds true for the 
packet.dll API as well). I definitely know that at the moment the only way 
to send OIDs is using packet.dll, even though we clearly state in the 
documentation that the Packet API should not be used. Those APIs should have 
never documented and published in the first place, mistakes of the past.
Having the input and output buffers of PacketRequest as a single buffer can 
be an annoyance in some cases, but I don't think it limits the use of the 
API itself. For sure it's not the best API signature and if I had to rewrite 
it, I would probably define a more meaningful API signature that for example 
separates the OID code from the input and output buffers. However, the 
packet.dll and wpcap.dll APIs are literally full of "stinky" APIs that are 
still there and will not be changed for the sake of compatibility.

Just my two cents
GV

>
> Thanks for listening.
>
> -- 
> "Fish" (David B. Trout)
> fish at softdevlabs.com
>
>
>



> _______________________________________________
> Winpcap-users mailing list
> Winpcap-users at winpcap.org
> https://www.winpcap.org/mailman/listinfo/winpcap-users
>