Interface down message and de-duplicated loopback packets #54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zeek/zeek-dev contains AF_Packet builtin. Use zeek/zeek:5.0.7 for now. Going forward we may want to remove the Dockerfile again, or add something complex that builds Zeek from scratch, or accept that it'll be hard to test this as an external plugin and developing in-tree is easier.
...not sure why they are marked inline.
Technically, socket() can return 0, so shouldn't use it as an indication of a non existent / closed socket. I'm not 100% sure about the Close() contract here: If something goes haywire with a packet source Zeek calls FatalError without calling Close() nor properly destructing the PktSrc. Oh yikes.
When using af_packet with an interface that was not up, the following
non-informative error was reported:
$ /opt/zeek-5.2/bin/zeek -i af_packet::replay
fatal error: problem with interface af_packet::replay (Invalid argument)
With this change, the error now includes information about the
interface being down:
$ ZEEK_PLUGIN_PATH=$(pwd)/build zeek -Ci af_packet::replay
fatal error: problem with interface af_packet::replay (interface is down)
Also removes some code duplication and prepares for loopback
special casing.
Fixes #51
awelzel
force-pushed
the
topic/awelzel/maintenance-usability-and-duplicated-loopback-packets
branch
from
bf4206c to
588d4aa
Compare
...we'll see them as incoming again and only pass them up to Zeek once this way. libpcap is doing it similarly, though supporting V2 and V3. Fixes #53
awelzel
force-pushed
the
topic/awelzel/maintenance-usability-and-duplicated-loopback-packets
branch
from
bbab7d5 to
08719aa
Compare
J-Gras
reviewed
Apr 3, 2023
Comment on lines
+295
to
+315
| // If this is a loopback interface and we're seeing an outgoing | ||
| // packet, drop it as it'll show up as incoming as well. Previously | ||
| // this would've caused Zeek to see all packets twice on loopback | ||
| // interfaces when compared to the normal libpcap based souce. | ||
| // | ||
| // The tpacket3_hdr is directly followed by a sockaddr_ll structure | ||
| // from where we can get the OUTGOING information about the packet. | ||
| // | ||
| // https://github.com/the-tcpdump-group/libpcap/blob/244080f5f9d4f17340041d1f5a3efd278ff08d7b/pcap-linux.c#L1173-L1181 | ||
| if ( is_loopback ) | ||
| { | ||
| const struct sockaddr_ll *sll = (struct sockaddr_ll*) | ||
| ((uint8_t *)packet + TPACKET_ALIGN(sizeof(struct tpacket3_hdr))); | ||
|
|
||
| if ( sll->sll_pkttype == PACKET_OUTGOING ) | ||
| { | ||
| DoneWithPacket(); | ||
| continue; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I can see the point made here that it doesn't seem very elegant to introduce all this for a quite exotic edge case. Throwing a warning on startup seems to be a good idea. Let's go with the same behaviour Suricata ends up with.
Comment on lines
+155
to
+157
| // std::fprintf(stderr, "AF_Packet: ifindex=%d ifr_flags=%0x is_loopback=%d\n", | ||
| // interface_ifindex, interface_flags, is_loopback); | ||
|
|
There was a problem hiding this comment.
Suggested change
| // std::fprintf(stderr, "AF_Packet: ifindex=%d ifr_flags=%0x is_loopback=%d\n", | |
| // interface_ifindex, interface_flags, is_loopback); |
| @@ -106,10 +130,10 @@ void AF_PacketSource::Open() | |||
| Opened(props); | |||
| } | |||
|
|
|||
| inline bool AF_PacketSource::BindInterface() | |||
| // Get ifindex and ifr_flags props.path. | |||
There was a problem hiding this comment.
Suggested change
| // Get ifindex and ifr_flags props.path. | |
| // Get interface index and flags. |
| @@ -57,6 +64,23 @@ void AF_PacketSource::Open() | |||
| return; | |||
| } | |||
|
|
|||
| // QueryInterface will log an appropriate error and return false. | |||
There was a problem hiding this comment.
Suggested change
| // QueryInterface will log an appropriate error and return false. |
Nit: I am not a fan of the name QueryInterface as the method comes with side-effects. Returning the values seems cleaner to me. If we skip the loopback part, the members for caching can be removed completely.
There was a problem hiding this comment.
If we skip the loopback part, the members for caching can be removed completely.
Yeah, seemed nice to not duplicate the ioctl for getting the index, but sure can let go.
I'll re-spin without the loopback change and just keep the issue open.
|
Closing to re-spin without the loopback changes. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #51 and #53 .