JulianSchmid / etherparse

A rust library for parsing ethernet & ethernet using protocols.
Apache License 2.0
285 stars 54 forks source link

Null/loopback header is incorrectly interpreted as `Ethernet2Header` #78

Open GyulyVGC opened 6 months ago

GyulyVGC commented 6 months ago

Hi @JulianSchmid, as you may know I'm using etherparse in Sniffnet.

Some users complained about the impossibility to see traffic when monitoring their VPN TUN interfaces, and today I tried it myself and found out that the problem resides in the fact that etherparse doesn't correctly identify the traffic as null/loopback, but it still identifies it as ethernet.

This is how a sample packet looks like in Wireshark: Screenshot 2023-12-22 at 12 18 21

As you can see, it's tagged as Null/loopback.

However, etherparse still categorise this as ethernet. The consequences are multiple:

In particular I double checked and found out that etherparse says that ip and transport header are None, and as you can see from the screenshot below MAC addresses are assigned to the first bytes of the packet, even if those bytes are not supposed to be MAC address but part of the Null/loopback header and part of the IP header.

Screenshot 2023-12-22 at 12 22 53

I think the solution would just require to correctly parse the loopback header and changing the offset to automatically parse the following ip and transport headers.

GyulyVGC commented 6 months ago

Alternatively, if you don't want to add full support for loopback header, it could be worth it to just try parsing ip header with the loopback offset in case parsing it via the Ethernet offset resolves to None (as a fallback strategy).

JulianSchmid commented 6 months ago

Hi @GyulyVGC ,

Thanks making me aware.

So what would be needed are some method to start parsing from the null/loopback? E.g.:

that would parse the Null/Loopback ( https://wiki.wireshark.org/NullLoopback.md ) header and continue downwards?

Greets Julian

GyulyVGC commented 6 months ago

Thanks for your prompt answer.

That would surely solve the problem, but I'm not sure it's the best solution. I was thinking to parse it starting from_ethernet_slice (or any other method that allows to start form the very first bytes).

The ideal solution would require etherparse to notice that the link header is None, adjusting the offset to retrieve ip and transport headers.

In this way users wouldn't have the concern of calling a different method for each possible case, without considering that we cannot even know a priori if we should call one method with respect to another in some circumstances, i.e. when to call from_null vs when to call from_ethernet_slice?

JulianSchmid commented 6 months ago

Writing something that tries detects if a given slice contains an ethernet II header or a null/loopback header is possible but not really robust. There is nothing in either headers that easily clearly identifies them. The only thing I could think of is to try to see if the byte position of the "ether type" field of an Ethernet II header

image

contains a known ether type value and if this not the case to check if there is a known "address family"/"protocol family" value present in any of the known null/loopback formats. But that falls apart if a to etherparse unknown ether_type is received.

I also had a look at the Wireshark implementation and they also rely on the "link_type" coming from either the interface or the "data link type" present in a pcap file.

But I could offer you that I write a small PR for Sniffnet to check the link type in https://github.com/GyulyVGC/sniffnet/blob/5274187832a9859c684742265d60c1e204bc68d3/src/secondary_threads/parse_packets.rs#L48 by checking for the link type of the capture:

Something along the lines of:

pub fn parse_packets(
    current_capture_id: &Arc<Mutex<usize>>,
    device: &MyDevice,
    mut cap: Capture<Active>,
    filters: &Filters,
    info_traffic_mutex: &Arc<Mutex<InfoTraffic>>,
    country_mmdb_reader: &Arc<MmdbReader>,
    asn_mmdb_reader: &Arc<MmdbReader>,
) {
    let capture_id = *current_capture_id.lock().unwrap();
    let link_type = cap.get_datalink();

    loop {
        match cap.next_packet() {
            Err(_) => {
                if *current_capture_id.lock().unwrap() != capture_id {
                    return;
                }
                continue;
            }
            Ok(packet) => {
                if *current_capture_id.lock().unwrap() != capture_id {
                    return;
                }
                let slice = match link_type {
                    Linktype::ETHERNET => PacketHeaders::from_ethernet_slice(&packet),
                    Linktype::NULL | Linktype::LOOP => PacketHeaders::from_null_slice(&packet),
                    Linktype::IPV4 | Linktype::IPV6 => PacketHeaders::from_ip_slice(&packet),
                    _ => {
                        return;
                    }
                };
                match slice {
GyulyVGC commented 6 months ago

Yeah as you said is not robust at all actually... the "ether type" field could potentially contain a valid value even if it's not an ethernet packet.

Relying on the link_type seems much more meaningful actually.

It'd be amazing if you could do that and I thank you very much for the proposal.

GyulyVGC commented 6 months ago

Hey @JulianSchmid

I've crafted a little PR myself, it'd be awesome if you could review it.

The TOFU mechanism I'm using is not so robust, so I'm open to other ideas to improve it.

GyulyVGC commented 6 months ago

https://github.com/GyulyVGC/sniffnet/pull/421

JulianSchmid commented 6 months ago

Hey @JulianSchmid

I've crafted a little PR myself, it'd be awesome if you could review it.

The TOFU mechanism I'm using is not so robust, so I'm open to other ideas to improve it.

👍 I left you a review in https://github.com/GyulyVGC/sniffnet/pull/421

GyulyVGC commented 6 months ago

Thank you so much 🙏