Project

General

Profile

Actions

Bug #451

open

tcpdump: invalid source/destination address selection

Added by Sven Eckelmann 5 days ago. Updated 5 days ago.

Status:
New
Priority:
Normal
Target version:
-
Start date:
07/05/2026
Due date:
% Done:

0%

Estimated time:

Description

This is the selection code for the source/destination address selection which @Marek Lindner introduced in a8926828bcf3 ("batctl: add raw wifi packet decapsulation support"):

    shost = wifi_hdr->addr2;
    if (fc & IEEE80211_FCTL_FROMDS)
        shost = wifi_hdr->addr3;
    else if (fc & IEEE80211_FCTL_TODS)
        shost = wifi_hdr->addr4;

    dhost = wifi_hdr->addr1;
    if (fc & IEEE80211_FCTL_TODS)
        dhost = wifi_hdr->addr3;

here is how wireshark 4.4.15 is doing it:

      uint32_t src_offset, dst_offset, da_offset, sa_offset, ta_offset = 10, bssid_offset;
      addr_type = FCF_ADDR_SELECTOR(fcf);
      if ((option_flags & IEEE80211_COMMON_OPT_NORMAL_QOS) && DATA_FRAME_IS_QOS(frame_type_subtype)) {
        if (!phdr->no_a_msdus && !DATA_FRAME_IS_NULL(frame_type_subtype)) {
          is_amsdu = QOS_AMSDU_PRESENT(qos_control);
        }
      }

      /* In order to show src/dst address we must always do the following */
      switch (addr_type)
      {

        case DATA_ADDR_T1:
          da_offset = 4;
          sa_offset = 10;
          bssid_offset = 16;
          dst_offset = da_offset;
          src_offset = sa_offset;
          break;

        case DATA_ADDR_T2:
          da_offset = 4;
          sa_offset = !is_amsdu ? 16 : 0;
          bssid_offset = 10;
          dst_offset = da_offset;
          src_offset = sa_offset;
          break;

        case DATA_ADDR_T3:
          da_offset = !is_amsdu ? 16 : 0;
          sa_offset = 10;
          bssid_offset = 4;
          dst_offset = da_offset;
          src_offset = sa_offset;
          break;

        case DATA_ADDR_T4:
          if (!is_amsdu) {
            da_offset = 16;
            sa_offset = 24;
            bssid_offset = 0;
            dst_offset = da_offset;
            src_offset = sa_offset;
          } else {
            da_offset = 0;
            sa_offset = 0;
            // The second BSSID (Addr4) is handled below.
            bssid_offset = 16;
            dst_offset = 4;     // RA
            src_offset = 10;    // TA
          }
          break;

        default:
          /* All four cases are covered above. */
          DISSECTOR_ASSERT_NOT_REACHED();
      }

Which of course looks a lot more complex but would be roughly in batctl's terms:

      if (fc & IEEE80211_STYPE_QOS_DATA) {
        if (!phdr->no_a_msdus && !DATA_FRAME_IS_NULL(frame_type_subtype)) {
          is_amsdu = QOS_AMSDU_PRESENT(qos_control);
        }
      }

      /* In order to show src/dst address we must always do the following */
      switch (fc & (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS))
      {

        case 0:
          dhost = wifi_hdr->addr1;
          shost = wifi_hdr->addr2;
          break;

        case IEEE80211_FCTL_FROMDS:
          dhost = wifi_hdr->addr1;
          shost = !is_amsdu ? wifi_hdr->addr3 : NULL;
          break;

        case IEEE80211_FCTL_TODS:
          dhost = !is_amsdu ? wifi_hdr->addr3 : NULL;
          shost = wifi_hdr->addr2;
          break;

        case (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS):
          if (!is_amsdu) {
            dhost = wifi_hdr->addr3;
            shost = wifi_hdr->addr4;
          } else {
            dhost = wifi_hdr->addr1;
            shost = wifi_hdr->addr2;
          }
          break;
      }

Even when ignoring the AMSDUs, you can see that the shost part is quite wrong for the IEEE80211_FCTL_TODS case. Instead of being addr2, it ends up with addr4. The shortened wireshark code would be:

      switch (fc & (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS))
      {
        case 0:
          shost = wifi_hdr->addr2;
          break;

        case IEEE80211_FCTL_FROMDS:
          shost = wifi_hdr->addr3;
          break;

        case IEEE80211_FCTL_TODS:
          shost = wifi_hdr->addr2;
          break;

        case (IEEE80211_FCTL_FROMDS | IEEE80211_FCTL_TODS):
          shost = wifi_hdr->addr4;
          break;
      }

which can be shortened (too look more like the batctl code) and applied via:

diff --git a/tcpdump.c b/tcpdump.c
index 762231c6ad556729ad2b06e8b64ea1d0556b5293..fc9a97fc8e3a5937dcdd7ba4ff0c1c0f8c8629c4 100644
--- a/tcpdump.c
+++ b/tcpdump.c
@@ -1369,10 +1369,10 @@ static void parse_wifi_hdr(unsigned char *packet_buff, ssize_t buff_len,
         return;

     shost = wifi_hdr->addr2;
-    if (fc & IEEE80211_FCTL_FROMDS)
-        shost = wifi_hdr->addr3;
-    else if (fc & IEEE80211_FCTL_TODS)
+    if ((fc & IEEE80211_FCTL_FROMDS) && (fc & IEEE80211_FCTL_TODS))
         shost = wifi_hdr->addr4;
+    else if (fc & IEEE80211_FCTL_FROMDS)
+        shost = wifi_hdr->addr3;

     dhost = wifi_hdr->addr1;
     if (fc & IEEE80211_FCTL_TODS)

Of course, this would not yet handle the AMSDU cases

Actions #1

Updated by Sven Eckelmann 5 days ago

  • Description updated (diff)
Actions #2

Updated by Sven Eckelmann 5 days ago

I have now submitted a patch for the basic shost problem: https://patchwork.open-mesh.org/project/b.a.t.m.a.n./patch/20260705-bugfixes-tcpdump-v1-9-c37f6f82eed1@narfation.org/

But I haven't touched the AMSDU part

Actions

Also available in: Atom PDF