Discussion:
tcpdump: decode 802.11 QoS frames correctly
Stefan Sperling
2016-01-31 11:53:09 UTC
Permalink
This matters for frames which arrived in A-MPDUs.

Before:

12:35:07.726898 802.11: QoS data: 00:1e:52:f1:80:55 sap 00 > 58:94:6b:06:70:04 sap 06 I (s=85,r=85,C) len=82

After:

12:49:08.879003 802.11: QoS data: 10.197.84.33 > 10.0.1.3: icmp: echo reply

Index: print-802_11.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
retrieving revision 1.28
diff -u -p -r1.28 print-802_11.c
--- print-802_11.c 12 Jan 2016 09:28:10 -0000 1.28
+++ print-802_11.c 31 Jan 2016 11:48:06 -0000
@@ -156,11 +156,19 @@ ieee80211_data(struct ieee80211_frame *w
struct ieee80211_frame_addr4 *w4;
u_int datalen;
int data = !(wh->i_fc[0] & IEEE80211_FC0_SUBTYPE_NODATA);
+ int hasqos = ((wh->i_fc[0] &
+ (IEEE80211_FC0_TYPE_MASK | IEEE80211_FC0_SUBTYPE_QOS)) ==
+ (IEEE80211_FC0_TYPE_DATA | IEEE80211_FC0_SUBTYPE_QOS));
u_char *esrc = NULL, *edst = NULL;

TCHECK(*wh);
- t += sizeof(struct ieee80211_frame);
- datalen = len - sizeof(struct ieee80211_frame);
+ if (hasqos) {
+ t += sizeof(struct ieee80211_qosframe);
+ datalen = len - sizeof(struct ieee80211_qosframe);
+ } else {
+ t += sizeof(struct ieee80211_frame);
+ datalen = len - sizeof(struct ieee80211_frame);
+ }

switch (wh->i_fc[1] & IEEE80211_FC1_DIR_MASK) {
case IEEE80211_FC1_DIR_TODS:
Stefan Sperling
2016-01-31 12:36:17 UTC
Permalink
Post by Stefan Sperling
This matters for frames which arrived in A-MPDUs.
12:35:07.726898 802.11: QoS data: 00:1e:52:f1:80:55 sap 00 > 58:94:6b:06:70:04 sap 06 I (s=85,r=85,C) len=82
12:49:08.879003 802.11: QoS data: 10.197.84.33 > 10.0.1.3: icmp: echo reply
Missed another case further down.

Index: print-802_11.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
retrieving revision 1.28
diff -u -p -r1.28 print-802_11.c
--- print-802_11.c 12 Jan 2016 09:28:10 -0000 1.28
+++ print-802_11.c 31 Jan 2016 12:19:55 -0000
@@ -153,14 +153,21 @@ int
ieee80211_data(struct ieee80211_frame *wh, u_int len)
{
u_int8_t *t = (u_int8_t *)wh;
- struct ieee80211_frame_addr4 *w4;
u_int datalen;
int data = !(wh->i_fc[0] & IEEE80211_FC0_SUBTYPE_NODATA);
+ int hasqos = ((wh->i_fc[0] &
+ (IEEE80211_FC0_TYPE_MASK | IEEE80211_FC0_SUBTYPE_QOS)) ==
+ (IEEE80211_FC0_TYPE_DATA | IEEE80211_FC0_SUBTYPE_QOS));
u_char *esrc = NULL, *edst = NULL;

TCHECK(*wh);
- t += sizeof(struct ieee80211_frame);
- datalen = len - sizeof(struct ieee80211_frame);
+ if (hasqos) {
+ t += sizeof(struct ieee80211_qosframe);
+ datalen = len - sizeof(struct ieee80211_qosframe);
+ } else {
+ t += sizeof(struct ieee80211_frame);
+ datalen = len - sizeof(struct ieee80211_frame);
+ }

switch (wh->i_fc[1] & IEEE80211_FC1_DIR_MASK) {
case IEEE80211_FC1_DIR_TODS:
@@ -176,12 +183,25 @@ ieee80211_data(struct ieee80211_frame *w
edst = wh->i_addr1;
break;
case IEEE80211_FC1_DIR_DSTODS:
- w4 = (struct ieee80211_frame_addr4 *) wh;
- TCHECK(*w4);
- t = (u_int8_t *) (w4 + 1);
- datalen = len - sizeof(*w4);
- esrc = w4->i_addr4;
- edst = w4->i_addr3;
+ if (hasqos) {
+ struct ieee80211_qosframe_addr4 *w4;
+
+ w4 = (struct ieee80211_qosframe_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ } else {
+ struct ieee80211_frame_addr4 *w4;
+
+ w4 = (struct ieee80211_frame_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ }
break;
}
Mark Kettenis
2016-01-31 14:14:46 UTC
Permalink
Date: Sun, 31 Jan 2016 13:36:17 +0100
Post by Stefan Sperling
This matters for frames which arrived in A-MPDUs.
12:35:07.726898 802.11: QoS data: 00:1e:52:f1:80:55 sap 00 > 58:94:6b:06:70:04 sap 06 I (s=85,r=85,C) len=82
12:49:08.879003 802.11: QoS data: 10.197.84.33 > 10.0.1.3: icmp: echo reply
Missed another case further down.
I think you should add a TCHECK somewhere at the top for the qos frame
size otherwise this might look beyond the end of the buffer for
specially crafted frames.
Index: print-802_11.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
retrieving revision 1.28
diff -u -p -r1.28 print-802_11.c
--- print-802_11.c 12 Jan 2016 09:28:10 -0000 1.28
+++ print-802_11.c 31 Jan 2016 12:19:55 -0000
@@ -153,14 +153,21 @@ int
ieee80211_data(struct ieee80211_frame *wh, u_int len)
{
u_int8_t *t = (u_int8_t *)wh;
- struct ieee80211_frame_addr4 *w4;
u_int datalen;
int data = !(wh->i_fc[0] & IEEE80211_FC0_SUBTYPE_NODATA);
+ int hasqos = ((wh->i_fc[0] &
+ (IEEE80211_FC0_TYPE_MASK | IEEE80211_FC0_SUBTYPE_QOS)) ==
+ (IEEE80211_FC0_TYPE_DATA | IEEE80211_FC0_SUBTYPE_QOS));
u_char *esrc = NULL, *edst = NULL;
TCHECK(*wh);
- t += sizeof(struct ieee80211_frame);
- datalen = len - sizeof(struct ieee80211_frame);
+ if (hasqos) {
+ t += sizeof(struct ieee80211_qosframe);
+ datalen = len - sizeof(struct ieee80211_qosframe);
+ } else {
+ t += sizeof(struct ieee80211_frame);
+ datalen = len - sizeof(struct ieee80211_frame);
+ }
switch (wh->i_fc[1] & IEEE80211_FC1_DIR_MASK) {
@@ -176,12 +183,25 @@ ieee80211_data(struct ieee80211_frame *w
edst = wh->i_addr1;
break;
- w4 = (struct ieee80211_frame_addr4 *) wh;
- TCHECK(*w4);
- t = (u_int8_t *) (w4 + 1);
- datalen = len - sizeof(*w4);
- esrc = w4->i_addr4;
- edst = w4->i_addr3;
+ if (hasqos) {
+ struct ieee80211_qosframe_addr4 *w4;
+
+ w4 = (struct ieee80211_qosframe_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ } else {
+ struct ieee80211_frame_addr4 *w4;
+
+ w4 = (struct ieee80211_frame_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ }
break;
}
Stefan Sperling
2016-01-31 19:57:29 UTC
Permalink
Post by Mark Kettenis
I think you should add a TCHECK somewhere at the top for the qos frame
size otherwise this might look beyond the end of the buffer for
specially crafted frames.
Oh yes, indeed.

Index: print-802_11.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
retrieving revision 1.28
diff -u -p -r1.28 print-802_11.c
--- print-802_11.c 12 Jan 2016 09:28:10 -0000 1.28
+++ print-802_11.c 31 Jan 2016 19:56:07 -0000
@@ -153,14 +153,25 @@ int
ieee80211_data(struct ieee80211_frame *wh, u_int len)
{
u_int8_t *t = (u_int8_t *)wh;
- struct ieee80211_frame_addr4 *w4;
u_int datalen;
int data = !(wh->i_fc[0] & IEEE80211_FC0_SUBTYPE_NODATA);
+ int hasqos = ((wh->i_fc[0] &
+ (IEEE80211_FC0_TYPE_MASK | IEEE80211_FC0_SUBTYPE_QOS)) ==
+ (IEEE80211_FC0_TYPE_DATA | IEEE80211_FC0_SUBTYPE_QOS));
u_char *esrc = NULL, *edst = NULL;

- TCHECK(*wh);
- t += sizeof(struct ieee80211_frame);
- datalen = len - sizeof(struct ieee80211_frame);
+ if (hasqos) {
+ struct ieee80211_qosframe *wq;
+
+ wq = (struct ieee80211_qosframe *) wh;
+ TCHECK(*wq);
+ t += sizeof(*wq);
+ datalen = len - sizeof(*wq);
+ } else {
+ TCHECK(*wh);
+ t += sizeof(*wh);
+ datalen = len - sizeof(*wh);
+ }

switch (wh->i_fc[1] & IEEE80211_FC1_DIR_MASK) {
case IEEE80211_FC1_DIR_TODS:
@@ -176,12 +187,25 @@ ieee80211_data(struct ieee80211_frame *w
edst = wh->i_addr1;
break;
case IEEE80211_FC1_DIR_DSTODS:
- w4 = (struct ieee80211_frame_addr4 *) wh;
- TCHECK(*w4);
- t = (u_int8_t *) (w4 + 1);
- datalen = len - sizeof(*w4);
- esrc = w4->i_addr4;
- edst = w4->i_addr3;
+ if (hasqos) {
+ struct ieee80211_qosframe_addr4 *w4;
+
+ w4 = (struct ieee80211_qosframe_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ } else {
+ struct ieee80211_frame_addr4 *w4;
+
+ w4 = (struct ieee80211_frame_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ }
break;
}
Mark Kettenis
2016-01-31 21:29:54 UTC
Permalink
Date: Sun, 31 Jan 2016 20:57:29 +0100
Post by Mark Kettenis
I think you should add a TCHECK somewhere at the top for the qos frame
size otherwise this might look beyond the end of the buffer for
specially crafted frames.
Oh yes, indeed.
Index: print-802_11.c
===================================================================
RCS file: /cvs/src/usr.sbin/tcpdump/print-802_11.c,v
retrieving revision 1.28
diff -u -p -r1.28 print-802_11.c
--- print-802_11.c 12 Jan 2016 09:28:10 -0000 1.28
+++ print-802_11.c 31 Jan 2016 19:56:07 -0000
@@ -153,14 +153,25 @@ int
ieee80211_data(struct ieee80211_frame *wh, u_int len)
{
u_int8_t *t = (u_int8_t *)wh;
- struct ieee80211_frame_addr4 *w4;
u_int datalen;
int data = !(wh->i_fc[0] & IEEE80211_FC0_SUBTYPE_NODATA);
+ int hasqos = ((wh->i_fc[0] &
+ (IEEE80211_FC0_TYPE_MASK | IEEE80211_FC0_SUBTYPE_QOS)) ==
+ (IEEE80211_FC0_TYPE_DATA | IEEE80211_FC0_SUBTYPE_QOS));
u_char *esrc = NULL, *edst = NULL;
- TCHECK(*wh);
- t += sizeof(struct ieee80211_frame);
- datalen = len - sizeof(struct ieee80211_frame);
+ if (hasqos) {
+ struct ieee80211_qosframe *wq;
+
+ wq = (struct ieee80211_qosframe *) wh;
+ TCHECK(*wq);
+ t += sizeof(*wq);
+ datalen = len - sizeof(*wq);
+ } else {
+ TCHECK(*wh);
+ t += sizeof(*wh);
+ datalen = len - sizeof(*wh);
+ }
switch (wh->i_fc[1] & IEEE80211_FC1_DIR_MASK) {
@@ -176,12 +187,25 @@ ieee80211_data(struct ieee80211_frame *w
edst = wh->i_addr1;
break;
- w4 = (struct ieee80211_frame_addr4 *) wh;
- TCHECK(*w4);
- t = (u_int8_t *) (w4 + 1);
- datalen = len - sizeof(*w4);
- esrc = w4->i_addr4;
- edst = w4->i_addr3;
+ if (hasqos) {
+ struct ieee80211_qosframe_addr4 *w4;
+
+ w4 = (struct ieee80211_qosframe_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ } else {
+ struct ieee80211_frame_addr4 *w4;
+
+ w4 = (struct ieee80211_frame_addr4 *) wh;
+ TCHECK(*w4);
+ t = (u_int8_t *) (w4 + 1);
+ datalen = len - sizeof(*w4);
+ esrc = w4->i_addr4;
+ edst = w4->i_addr3;
+ }
break;
}
Loading...