Adding --add-payload-offset flag (macos only) [and 1.48 updates]

[thepacketgeek]

Follow-up for #129: Not all "macos" Point-to-point (E.g. utun) interfaces use a payload offset, according to libpnet. This PR removes the offset for VPN interfaces by default and adds an --add-payload-offset flag to allow the offset when desired.

• Moves payload_offset calculation to Sniffer::new to evaluate network interface and cfg! conditions once
• Targets "macos" only (since payload_offset calculation in Sniffer only affects cfg!(target_os = "macos")
  • This does make the code a little messy (especially in the tests), but it wouldn't make sense to have this option for non-macos targets. I'm open to suggestions around dealing with this handling

[jconley]

@thepacketgeek @imsnif the logic here seems to be based off a libpnet example, but the logic seems to differ: https://github.com/libpnet/libpnet/blob/master/examples/packetdump.rs#L255
for reference, my local app-implemented vpn utun2 is not loopback. flags=8051<UP,POINTOPOINT,MULTICAST>

[thepacketgeek]

Correct, utun interfaces are not loopback, but loopback interfaces do have a payload offset.

In my testing, some utun interfaces have a payload offset (which is why #129 works) but not all. For example, when using bandwhich to monitor a Wireguard utun interface, this PR's --no-payload-offset is required to properly parse packets.

Looking at this code, it seems the offset should only apply to loopback interfaces, so I'm really not sure why packet parsing is different for some utun interfaces.

[imsnif]

Hey @thepacketgeek - could you help me understand what's the current status here? What do we know, what aren't we sure about, which types of interfaces need it, which don't, what will break, etc? It'll help me make a decision about how to include this change. Thanks!

[thepacketgeek]

Hey @imsnif , sorry for the delay. I latest update is here. I suspect that with the libpnet change utun/P2P interfaces no longer need an offset (only loopback interfaces on mac do), which greatly simplifies this PR (no cfg(taget = "macos") needed). However, I can't find VPN clients for utun interfaces to test with that still need an offset, all clients I can test with work fine without an offset.

So I was hoping someone that has a client that previously needed the offset to work test out the patch. I can update this PR with the simplified change, but would need external help to test it out on more interfaces.

[imsnif]

Aha. I sadly don't have access to one either. Do you think we can go with that assumption and provide some workaround flag in case we're either wrong or half right in one way or another? :)

[thepacketgeek]

I think that's a great idea :) I'll revert back to the version with a flag, but make the default only add offsets for loopbacks, but with a flag to add it for utun interfaces.

[imsnif]

Hey @thepacketgeek - looking good. I re-ran the failing test because it's sometimes a little flaky. It's okay now (sorry about that :) ).

Is this strictly a mac behaviour? If we're adding this as a manual flag, would it be wrong to have the flag be cross-platform and not mac-only?

[thepacketgeek]

It does appear to be macos only: https://github.com/libpnet/libpnet/blob/07e73e2c60dcec7a3655ea79876058037a57fff6/examples/packetdump.rs#L252,L264

I suspect adding a payload offset for any combination other than macos & loopback/utun would end up breaking packet parsing for anything other than macos?

[thepacketgeek]

@imsnif I agree that the change is greatly simplified without the #[cfg(target = 'macos')] for the payload flag deafult. Would that make more sense?

I was finally able to confirm that the payload flag is still required for some VPN interfaces (like Cisco AnyConnect). I really wish there was a way to deterministically know :\

[imsnif]

Sure. Let's add it as an option. Would definitely be better to deterministically know, but this way we can at least have a built-in workaround.

[thepacketgeek]

--add-payload-offset is an option for macos targets only (otherwise it's set to false). This should default to working on utun interfaces on mac and allow users the option to add the payload if necessary.

[thepacketgeek]

Oh shoot, I got caught in 1.48 clippy changes. I'll make another PR to update those lints and rebase this PR on top

[imsnif]

Actually, if you could do it in this PR that would be helpful for me :)

My apologies - I don't have a lot of time for bandwhich these days as I'm sure you noticed. I'd be happy to try and make time for this patch in the next few days though.

[thepacketgeek]

@imsnif I'm running into another gift of 1.48, an issue with the LruCache used in trust-dns (happens on main, this is not new to this PR):

MacOS:
thread 'resolver' panicked at 'attempted to leave type `linked_hash_map::Node<proto::op::Query, dns_lru::LruValue>` uninitialized, which is invalid', /.rustup/toolchains/st    -apple-darwin/lib/rustlib/src/rust/library/core/src/mem/mod.rs:658:9

Ubuntu:
thread 'resolver' panicked at 'attempted to leave type `linked_hash_map::Node<proto::op::Query, dns_lru::LruValue>` uninitialized, which is invalid', /home/mat/.rustup/toolchains/stable-x86_64/...e/src/mem/mod.rs:658:9

This only happens when DNS resolution is enabled, so -n hides it. I'm trying to track this issue down as it will need to be fixed in trust-dns.

I suggest waiting for a release

[thepacketgeek]

Alright, sorry for all the noise. Seems this issue has already been resolved in lru-cache/linked_hash_map

(I believe by the change that no longer requires maybe-uninit)

This PR should be ready to go finally :)

[dndll]

Alright, sorry for all the noise. Seems this issue has already been resolved in lru-cache/linked_hash_map

(I believe by the change that no longer requires maybe-uninit)

This PR should be ready to go finally :)

Thanks for this, i came here from the reference PR which may help the maintainers of bandwich.
lru-cache updated the dependency in place so it meant i had to monkey patch the lockfile to get the right version.