[PATCH 0/8] iw: parse measurement pilot and fix scan bugs

[PATCH 0/8] iw: parse measurement pilot and fix scan bugs

[Markus Theil]

Besides adding a parser for the measurement pilot element, this
series fixes several bugs found while fuzzing the scan code of iw.

Markus Theil (8):
  iw: scan: parse measurement pilot element
  iw: scan: fix buffer over-read in print_ies()
  iw: scan: fix buffer over-read in operation class parsing
  iw: scan: fix buffer over-read in parsing roaming consortium
  iw: scan: fix buffer over-read in print_wifi_wps
  iw: scan: fix buffer over-read in print_p2p
  iw: scan: fix undefined behaviour in rm capa print
  iw: scan: fix undefined behaviour in print_vht_capa()

 scan.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 58 insertions(+), 13 deletions(-)

--
2.25.0

[PATCH 1/8] iw: scan: parse measurement pilot element

[Markus Theil]

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/scan.c b/scan.c
index 50a4147..a6cb3bb 100644
--- a/scan.c
+++ b/scan.c
@@ -1530,6 +1530,47 @@ static void print_supp_op_classes(const uint8_t type, uint8_t len,
 		}
 }
 
+static void print_measurement_pilot_tx(const uint8_t type, uint8_t len,
+				       const uint8_t *data,
+				       const struct print_ies_data *ie_buffer)
+{
+	printf("\n");
+	printf("\t\t * interval: %d TUs\n", data[0]);
+
+	if(len <= 1)
+		return;
+
+	uint8_t *p = (uint8_t *) data + 1;
+	uint8_t len_remaining = len - 1;
+	
+	while (len_remaining >=5) {
+		uint8_t subelement_id = *p;
+		++p;
+		uint8_t len = *p;
+		++p;
+
+		len_remaining -= 2;
+
+		/* 802.11-2016 only allows vendor specific elements */
+		if (subelement_id != 221) {
+			printf("\t\t * <Invalid subelement ID %d>\n", subelement_id);
+			return;
+		}
+
+		printf("\t\t * vendor specific: OUI %.2x:%.2x:%.2x, data:",
+			p[0], p[1], p[2]);
+		len_remaining -= 3;
+
+		if (len > len_remaining)
+			printf(" <Parse error, element too short>\n");
+			return;
+
+		while (p < p + len)
+			printf(" %.2x", *p);
+		printf("\n");
+	}
+}
+
 static void print_obss_scan_params(const uint8_t type, uint8_t len,
 				   const uint8_t *data,
 				   const struct print_ies_data *ie_buffer)
@@ -1652,6 +1693,7 @@ static const struct ie_print ieprinters[] = {
 	[45] = { "HT capabilities", print_ht_capa, 26, 26, BIT(PRINT_SCAN), },
 	[47] = { "ERP D4.0", print_erp, 1, 255, BIT(PRINT_SCAN), },
 	[59] = { "Supported operating classes", print_supp_op_classes, 1, 255, BIT(PRINT_SCAN), },
+	[66] = { "Measurement Pilot Transmission", print_measurement_pilot_tx, 1, 255, BIT(PRINT_SCAN), },
 	[74] = { "Overlapping BSS scan params", print_obss_scan_params, 14, 255, BIT(PRINT_SCAN), },
 	[61] = { "HT operation", print_ht_op, 22, 22, BIT(PRINT_SCAN), },
 	[62] = { "Secondary Channel Offset", print_secchan_offs, 1, 1, BIT(PRINT_SCAN), },
-- 
2.25.0

[PATCH 2/8] iw: scan: fix buffer over-read in print_ies()

[Markus Theil]

This patch correctly checks, if enough data bytes for parsing IEs are
present (-2 in check for type and length). Furthermore, it adds a
nullptr and length check to ease future fuzzing.

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scan.c b/scan.c
index a6cb3bb..14138ca 100644
--- a/scan.c
+++ b/scan.c
@@ -2181,7 +2181,10 @@ void print_ies(unsigned char *ie, int ielen, bool unknown,
 		.ie = ie,
 		.ielen = ielen };
 
-	while (ielen >= 2 && ielen >= ie[1]) {
+	if (ie == NULL || ielen < 0)
+		return;
+
+	while (ielen >= 2 && ielen - 2 >= ie[1]) {
 		if (ie[0] < ARRAY_SIZE(ieprinters) &&
 		    ieprinters[ie[0]].name &&
 		    ieprinters[ie[0]].flags & BIT(ptype)) {
-- 
2.25.0

[PATCH 3/8] iw: scan: fix buffer over-read in operation class parsing

[Markus Theil]

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scan.c b/scan.c
index 14138ca..2d11f81 100644
--- a/scan.c
+++ b/scan.c
@@ -1507,7 +1507,7 @@ static void print_supp_op_classes(const uint8_t type, uint8_t len,
 	
 	printf("\n");
 	printf("\t\t * current operating class: %d\n", *p);
-	while (p++ < next_data) {
+	while (++p < next_data) {
 		if (*p == 130) {
 			one_hundred_thirty_delimiter = 1;
 			break;
@@ -1519,11 +1519,11 @@ static void print_supp_op_classes(const uint8_t type, uint8_t len,
 		printf("\t\t * operating class: %d\n", *p);
 	}
 	if (one_hundred_thirty_delimiter)
-		while (p++ < next_data) {
+		while (++p < next_data) {
 			printf("\t\t * current operating class extension: %d\n", *p);
 		}
 	if (zero_delimiter)
-		while (p++ < next_data - 1) {
+		while (++p < next_data - 1) {
 			printf("\t\t * operating class duple: %d %d\n", p[0], p[1]);
 			if (*p == 0)
 				break;
-- 
2.25.0

[PATCH 4/8] iw: scan: fix buffer over-read in parsing roaming consortium

[Markus Theil]

ANQP and OI length are always present, increase min. length by 2.

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scan.c b/scan.c
index 2d11f81..9a02363 100644
--- a/scan.c
+++ b/scan.c
@@ -1707,7 +1707,7 @@ static const struct ie_print ieprinters[] = {
 	[127] = { "Extended capabilities", print_capabilities, 0, 255, BIT(PRINT_SCAN), },
 	[107] = { "802.11u Interworking", print_interworking, 0, 255, BIT(PRINT_SCAN), },
 	[108] = { "802.11u Advertisement", print_11u_advert, 0, 255, BIT(PRINT_SCAN), },
-	[111] = { "802.11u Roaming Consortium", print_11u_rcon, 0, 255, BIT(PRINT_SCAN), },
+	[111] = { "802.11u Roaming Consortium", print_11u_rcon, 2, 255, BIT(PRINT_SCAN), },
 	[195] = { "Transmit Power Envelope", print_tx_power_envelope, 2, 5, BIT(PRINT_SCAN), },
 };
 
-- 
2.25.0

[PATCH 5/8] iw: scan: fix buffer over-read in print_wifi_wps

[Markus Theil]

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scan.c b/scan.c
index 9a02363..f57925d 100644
--- a/scan.c
+++ b/scan.c
@@ -1818,7 +1818,7 @@ static void print_wifi_wps(const uint8_t type, uint8_t len, const uint8_t *data,
 	while (len >= 4) {
 		subtype = (data[0] << 8) + data[1];
 		sublen = (data[2] << 8) + data[3];
-		if (sublen > len)
+		if (sublen > len - 4)
 			break;
 
 		switch (subtype) {
-- 
2.25.0

[PATCH 6/8] iw: scan: fix buffer over-read in print_p2p

[Markus Theil]

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scan.c b/scan.c
index f57925d..b3e7baa 100644
--- a/scan.c
+++ b/scan.c
@@ -2036,7 +2036,7 @@ static inline void print_p2p(const uint8_t type, uint8_t len,
 		case 0x12: /* invitation flags */
 		case 0xdd: /* vendor specific */
 		default: {
-			const __u8 *subdata = data + 4;
+			const __u8 *subdata = data + 3;
 			__u16 tmplen = sublen;
 
 			tab_on_first(&first);
-- 
2.25.0

[PATCH 7/8] iw: scan: fix undefined behaviour in rm capa print

[Markus Theil]

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scan.c b/scan.c
index b3e7baa..e2a620a 100644
--- a/scan.c
+++ b/scan.c
@@ -586,10 +586,10 @@ static void print_rm_enabled_capabilities(const uint8_t type, uint8_t len,
 			    const uint8_t *data,
 			    const struct print_ies_data *ie_buffer)
 {
-	__u64 capa = data[0] |
-		     data[1] << 8 |
-		     data[2] << 16 |
-		     data[3] << 24 |
+	__u64 capa = ((__u64) data[0]) |
+		     ((__u64) data[1]) << 8 |
+		     ((__u64) data[2]) << 16 |
+		     ((__u64) data[3]) << 24 |
 		     ((__u64) data[4]) << 32;
 
 	printf("\n");
-- 
2.25.0

[PATCH 8/8] iw: scan: fix undefined behaviour in print_vht_capa()

[Markus Theil]

Signed-off-by: Markus Theil <markus.theil@tu-ilmenau.de>
---
 scan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scan.c b/scan.c
index e2a620a..8f8d8ba 100644
--- a/scan.c
+++ b/scan.c
@@ -1473,8 +1473,8 @@ static void print_vht_capa(const uint8_t type, uint8_t len, const uint8_t *data,
 			   const struct print_ies_data *ie_buffer)
 {
 	printf("\n");
-	print_vht_info(data[0] | (data[1] << 8) |
-		       (data[2] << 16) | (data[3] << 24),
+	print_vht_info((__u32) data[0] | ((__u32)data[1] << 8) |
+		       ((__u32)data[2] << 16) | ((__u32)data[3] << 24),
 		       data + 4);
 }
 
-- 
2.25.0

Re: [PATCH 0/8] iw: parse measurement pilot and fix scan bugs

[Johannes Berg]

On Sun, 2020-02-09 at 17:58 +0100, Markus Theil wrote:
> this
> series fixes several bugs found while fuzzing the scan code of iw.

Nice, can you describe the setup you used for this?

johannes

Re: [PATCH 0/8] iw: parse measurement pilot and fix scan bugs

[Markus Theil]

On 2/10/20 9:11 AM, Johannes Berg wrote:
> On Sun, 2020-02-09 at 17:58 +0100, Markus Theil wrote:
>> this
>> series fixes several bugs found while fuzzing the scan code of iw.
> Nice, can you describe the setup you used for this?
>
> johannes
>
I used clang with its sanitizers (-fsanitize=address,fuzzer,undefined).
A file named fuzz_scan.c is used to call print_ies() with random input.
Some beacon frame TLVs were used as seed corpus for libfuzzer. I can also
post my small patches doing this, but the current integration into the Makefile can
be called "ad-hoc" at best :).

Markus

fuzz_scan.c:

#include "iw.h"

int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
	print_ies((unsigned char *)data, size, size % 2, PRINT_SCAN);
	return 0;
}

Re: [PATCH 0/8] iw: parse measurement pilot and fix scan bugs

[Johannes Berg]

On Mon, 2020-02-10 at 09:22 +0100, Markus Theil wrote:
> I can also
> post my small patches doing this, but the current integration into the Makefile can
> be called "ad-hoc" at best :).

Please do, I can always fix up things but then I have some incentive ;-)

johannes