Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For which Packet.dll calls can GetLastError() be used if the call fails? #1542

Open
guyharris opened this Issue Apr 5, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@guyharris
Copy link

guyharris commented Apr 5, 2019

Somebody reported a "failed to set hardware filter to promiscuous mode)" error; that comes from a PacketSetHwFilter() call, which ultimately does a DeviceIoControl() for NDIS adapters.

libpcap currently doesn't call GetLastError() and report the error code as part of the error if if PacketSetHwFilter() fails. That might provide more useful debugging information.

@dmiller-nmap

This comment has been minimized.

Copy link

dmiller-nmap commented Apr 8, 2019

This is a good question. We didn't try to convert the original Packet.dll documentation from Doxygen like we did for the rest of the guide, mostly because the original WinPcap developers avoided generating HTML docs from it and specifically noted that the Packet API is not intended to be used directly. Their reasoning was that end users should be using the libpcap API, and since they were essentially maintaining their own fork of libpcap 1.0.0, they could make any changes to Packet API that they saw fit because they'd just patch libpcap in the same release. Now that we're aiming for a pure upstream libpcap, it makes more sense to make the Packet API more understandable.

Initial quick scan shows the following:

  • PacketOpenAdapter appears to use SetLastError in a few functions it calls, notably PacketOpenAdapterNPF and PacketSetReadEvt.
  • PacketGetAdapterNames uses SetLastError, but sets the error to ERROR_INSUFFICIENT_BUFFER both when the input buffer is too small and when there are no adapters found. In the second case, the BufferSize output parameter will be set to 0.
  • Any function that ends up calling DeviceIoControl() ought to reasonably set the last error, though I've found a few places where the error returned by the npcap.sys driver was less than helpful. Most recently, I added several useful error values to PacketSendPackets that would previously only return ERROR_GEN_FAILURE.

In the specific case of PacketSetHwFilter(), here are the failure code paths and how any error is reported:

  1. If the adapter is an unsupported type (specifically, not an NDIS adapter), the return value is FALSE and no error is set.
  2. If the adapter is supported, but Packet.DLL fails to allocate a buffer for the OID_GEN_CURRENT_PACKET_FILTER, return FALSE. I believe GlobalAlloc() will call SetLastError(), but in debug builds there may be intervening trace prints that reset it.
  3. The OID is sent with PacketRequest(), which calls DeviceIoControl(), which sets the last error if there is an error, but immediately afterwards GlobalFree() is called, which will probably clobber the error. On debug builds, trace prints may also reset the error.

This is pretty typical from what I can see. There are a few places where an effort has been made to ensure the error is propagated, but mostly the only reliable source of info is the return value. I'd like to make this better, but I'm not sure how high a priority it is at the moment.

@guyharris

This comment has been minimized.

Copy link
Author

guyharris commented Apr 8, 2019

Thanks.

So:

In the short term, do you know what might be causing PacketSetHwFilter() to fail in this ask.wireshark.org question? The path to the device has a GUID in it, so it's presumably an NDIS adapter.

In the slightly longer term, we might want to have pcap-npf.c return PCAP_WARNING_PROMISC_NOTSUP if that call fails, so that the caller of pcap_activate() could report a slightly friendlier message. (Are special privileges required to enable promiscuous mode? If so, then "you don't have those privileges" should turn into PCAP_ERROR_PROMISC_PERM_DENIED).

In the longer term, yes, we should try to make it possible to get a Windows error, so that for some failures we can report more diagnostic information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.