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

Snaplen requires BPF filter to be applied #201

Closed
dmiller-nmap opened this issue Jul 2, 2020 · 10 comments
Closed

Snaplen requires BPF filter to be applied #201

dmiller-nmap opened this issue Jul 2, 2020 · 10 comments

Comments

@dmiller-nmap
Copy link
Contributor

@dmiller-nmap dmiller-nmap commented Jul 2, 2020

Inherited problem from WinPcap: snaplen is implemented as a modification to BPF filter in pcap_compile(), which means that the only way to have it work is to follow this procedure (as pcap_open_live() does internally):

  1. Set snaplen with pcap_set_snaplen()
  2. Compile any filter, even "" empty string with pcap_compile()
  3. Set that BPF filter with pcap_setfilter()

Doing this in any other order will not work.

Solution: we need to implement a snaplen set operation, probably as a new IoControl code, and expose it via the existing PacketSetSnapLen() function. Then libpcap has to call PacketSetSnapLen() in the appropriate places, which it currently only does for DAG cards. This would be fine for them to do even with backwards compatibility in mind, since on non-DAG adapters, PacketSetSnapLen() has historically been a no-op.

@guyharris
Copy link

@guyharris guyharris commented Jul 2, 2020

pcap-bpf.c initially sets an "accept all" filter in the activate routine, so if no filter is explicitly set with pcap_setfilter(), there's still a filter that causes packets to be trimmed off at the snapshot length.

pcap-npf.c could do the same.

(And I should check to make sure pcap-linux.c does the same for memory-mapped capture, if that's the only way the snapshot length gets passed to the kernel.)

@dmiller-nmap
Copy link
Contributor Author

@dmiller-nmap dmiller-nmap commented Jul 2, 2020

@guyharris I thought of this, but I kept running into difficulties, primarily this one:

It doesn't appear there's anything preventing pcap_setfilter() from being called prior to pcap_activate(), which means the "accept all" filter would clobber the other one. On the other hand, I'm guessing doing this means that the call to PacketSetBpf() in pcap_setfilter_npf() is done when p->adapter == NULL, which would fail, leading to install_bpf_program() being called, and filtering being done in userland. I think this is not a code path that should be open.

@guyharris
Copy link

@guyharris guyharris commented Jul 3, 2020

@guyharris I thought of this, but I kept running into difficulties, primarily this one:

It doesn't appear there's anything preventing pcap_setfilter() from being called prior to pcap_activate(),

There is.

The setfilter_op pointer, which pcap_setfilter() calls through, is initially set to pcap_setfilter_not_initialized(), which just returns an error of "not activated". Only after the pcap_t is activated is it set to the module's "set filter" routine.

(Note that, if it could get to the module before the pcap_t is activated, that would be an issue for pcap-bpf.c as well.)

@dmiller-nmap
Copy link
Contributor Author

@dmiller-nmap dmiller-nmap commented Jul 3, 2020

We will cherry-pick the-tcpdump-group/libpcap@6c893c1 to nmap/libpcap's wpcap-1.9 branch for the next release and mark this issue resolved in our changelog. Thanks for the help!

@guyharris
Copy link

@guyharris guyharris commented Jul 3, 2020

We will cherry-pick the-tcpdump-group/libpcap@6c893c1 to nmap/libpcap's wpcap-1.9 branch for the next release and mark this issue resolved in our changelog. Thanks for the help!

That, and 141253c471119db0761d73c2fc095b82a2017eb3, are commits to a test program, to allow the test program not to set the snapshot length and not to set the filter; I made them in order to be able to test the change I'm proposing to pcap-npf.c. I'll test the latter change today and, if it works, check it in; that will be what you'll want to cherry-pick to get the bug fixed.

@guyharris
Copy link

@guyharris guyharris commented Jul 3, 2020

Oops, that commit checked in my "add a filter when activating" change before it was tested - it doesn't compile. It was intended to check in only a fix to the test program; I've backed out the pcap-npf.c change. I'll get it fixed and check it in later.

guyharris added a commit to the-tcpdump-group/libpcap that referenced this issue Jul 3, 2020
You need a filter in order to tell the kernel what the snapshot length
is.

This should handle GitHub issues #947 and nmap/npcap#201.
@guyharris
Copy link

@guyharris guyharris commented Jul 4, 2020

OK, that's the commit you want - the-tcpdump-group/libpcap@a0a9c27. (I had to hammer on libpcap a bit - AppVeyor must have installed a new version of VS 2019, because a bunch of switch-statement warnings started popping up and, as we're building with /WX, the build failed - because I wanted the build to succeed before I signed off on the commit.)

dmiller-nmap pushed a commit to nmap/libpcap that referenced this issue Jul 4, 2020
You need a filter in order to tell the kernel what the snapshot length
is.

This should handle GitHub issues the-tcpdump-group#947 and nmap/npcap#201.
@guyharris
Copy link

@guyharris guyharris commented Jul 11, 2020

This is fixed now, right?

@dmiller-nmap
Copy link
Contributor Author

@dmiller-nmap dmiller-nmap commented Jul 11, 2020

@dmiller-nmap
Copy link
Contributor Author

@dmiller-nmap dmiller-nmap commented Jul 13, 2020

This issue is fixed in Npcap 0.9995 by backporting an unreleased changed from upstream libpcap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants