Bug #451
opentcpdump: invalid source/destination address selection
0%
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
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