hcidump: fix buffer overflow

PATCH Applied Cho, Yu-Chen 2 participants 8 messages Oct 31, 2018 – Nov 6, 2018 Bluetooth drivers

Share & tools

Cho, Yu-Chen

author

2018-10-31 08:15 UTC #0 lore raw

cover hcidump: fix buffer overflow

Bugs were simple buffer overflow that was actually discovered already couple years ago by op7ic:https://www.spinics.net/lists/linux-bluetooth/msg68892.html Caused by missing boundary checks before accessing.

Cho, Yu-Chen (3):
hcidump: fixed AMP Assoc dump heap-over-flow hcidump:fixed hci frame dump stack-buffer-overflow
hcidump: Fix set_ext_ctrl() global buffer overflow

 tools/parser/amp.c   | 65 +++++++++++++++++++++++---------------------
 tools/parser/hci.c   |  3 ++
 tools/parser/l2cap.c |  2 +-
 3 files changed, 38 insertions(+), 32 deletions(-)

Signature · 1 line

2.19.1

Cho, Yu-Chen

author

2018-10-31 08:15 UTC #1 lore raw

PATCH 1/3 hcidump: fixed AMP Assoc dump heap-over-flow

amp_assoc_dump() didn't check the length of amp assoc struct of Type-Length-Value (TLV) triplets, and the Connected Chan List (number of triplets) is also need to check, or there are wrong length for the number of triplets.

Signed-off-by Cho, Yu-Chen <[email protected]>

---
 tools/parser/amp.c | 65 ++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

tools/parser/amp.c +34 ~~-31~~

index 158ca4a75..bd7f91000 100644
@@ -33,7 +33,7 @@ static void amp_dump_chanlist(int level, struct amp_tlv *tlv, char *prefix)
 	struct amp_country_triplet *triplet;
 	int i, num;
 
-	num = (tlv->len - sizeof(*chan_list)) / sizeof(*triplet);
+	num = sizeof(*chan_list->triplets) / sizeof(*chan_list->triplets[0]);
 
 	printf("%s (number of triplets %d)\n", prefix, num);

@@ -80,47 +80,50 @@ void amp_assoc_dump(int level, uint8_t *assoc, uint16_t len)
 
 		p_indent(level+1, 0);
 
-		switch (tlv->type) {
-		case A2MP_MAC_ADDR_TYPE:
-			if (tlvlen != 6)
-				break;
-			printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
+		if (tlvlen !=0) {
+			switch (tlv->type) {
+			case A2MP_MAC_ADDR_TYPE:
+				if (tlvlen != 6)
+					break;
+				printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
 					tlv->val[0], tlv->val[1], tlv->val[2],
 					tlv->val[3], tlv->val[4], tlv->val[5]);
-			break;
-
-		case A2MP_PREF_CHANLIST_TYPE:
-			amp_dump_chanlist(level, tlv, "Preferred Chan List");
-			break;
+				break;
 
-		case A2MP_CONNECTED_CHAN:
-			amp_dump_chanlist(level, tlv, "Connected Chan List");
-			break;
+			case A2MP_PREF_CHANLIST_TYPE:
+				amp_dump_chanlist(level, tlv, "Preferred Chan List");
+				break;
 
-		case A2MP_PAL_CAP_TYPE:
-			if (tlvlen != 4)
+			case A2MP_CONNECTED_CHAN:
+				amp_dump_chanlist(level, tlv, "Connected Chan List");
 				break;
-			printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
+
+			case A2MP_PAL_CAP_TYPE:
+				if (tlvlen != 4)
+					break;
+				printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
 					tlv->val[0], tlv->val[1], tlv->val[2],
 					tlv->val[3]);
-			break;
-
-		case A2MP_PAL_VER_INFO:
-			if (tlvlen != 5)
 				break;
-			ver = (struct amp_pal_ver *) tlv->val;
-			printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",
+
+			case A2MP_PAL_VER_INFO:
+				if (tlvlen != 5)
+					break;
+				ver = (struct amp_pal_ver *) tlv->val;
+				printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",
 					ver->ver, btohs(ver->company_id),
 					btohs(ver->sub_ver));
-			break;
+				break;
 
-		default:
-			printf("Unrecognized type %d\n", tlv->type);
-			break;
-		}
+			default:
+				printf("Unrecognized type %d\n", tlv->type);
+				break;
+			}
 
-		len -= tlvlen + sizeof(*tlv);
-		assoc += tlvlen + sizeof(*tlv);
-		tlv = (struct amp_tlv *) assoc;
+			len -= tlvlen + sizeof(*tlv);
+			assoc += tlvlen + sizeof(*tlv);
+			tlv = (struct amp_tlv *) assoc;
+		} else
+			break;
 	}
 }

Signature · 1 line

2.19.1

Luiz Augusto von Dentz

maintainer

2018-11-02 10:54 UTC #2 lore raw

Re: [PATCH 1/3 BlueZ] hcidump: fixed AMP Assoc dump heap-over-flow

Hi,

Quoted 7 lines · Yu-Chen

amp_assoc_dump() didn't check the length of amp assoc struct of Type-Length-Value (TLV) triplets, and the Connected Chan List (number of triplets) is also need to check, or there are wrong length for the number of triplets.
Signed-off-by Cho, Yu-Chen <[email protected]>

Please remove the Signed-off-by, we don't use that in userspace.

Quoted 99 lines

---
 tools/parser/amp.c | 65 ++++++++++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

tools/parser/amp.c +34 ~~-32~~

index 158ca4a75..bd7f91000 100644
@@ -33,7 +33,7 @@ static void amp_dump_chanlist(int level, struct amp_tlv *tlv, char *prefix)
        struct amp_country_triplet *triplet;
        int i, num;
 
-       num = (tlv->len - sizeof(*chan_list)) / sizeof(*triplet);
+       num = sizeof(*chan_list->triplets) / sizeof(*chan_list->triplets[0]);
 
        printf("%s (number of triplets %d)\n", prefix, num);

@@ -80,47 +80,50 @@ void amp_assoc_dump(int level, uint8_t *assoc, uint16_t len)
 
                p_indent(level+1, 0);
 
-               switch (tlv->type) {
-               case A2MP_MAC_ADDR_TYPE:
-                       if (tlvlen != 6)
-                               break;
-                       printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
+               if (tlvlen !=0) {
+                       switch (tlv->type) {
+                       case A2MP_MAC_ADDR_TYPE:
+                               if (tlvlen != 6)
+                                       break;
+                               printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
                                        tlv->val[0], tlv->val[1], tlv->val[2],
                                        tlv->val[3], tlv->val[4], tlv->val[5]);
-                       break;
-
-               case A2MP_PREF_CHANLIST_TYPE:
-                       amp_dump_chanlist(level, tlv, "Preferred Chan List");
-                       break;
+                               break;
 
-               case A2MP_CONNECTED_CHAN:
-                       amp_dump_chanlist(level, tlv, "Connected Chan List");
-                       break;
+                       case A2MP_PREF_CHANLIST_TYPE:
+                               amp_dump_chanlist(level, tlv, "Preferred Chan List");
+                               break;
 
-               case A2MP_PAL_CAP_TYPE:
-                       if (tlvlen != 4)
+                       case A2MP_CONNECTED_CHAN:
+                               amp_dump_chanlist(level, tlv, "Connected Chan List");
                                break;
-                       printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
+
+                       case A2MP_PAL_CAP_TYPE:
+                               if (tlvlen != 4)
+                                       break;
+                               printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
                                        tlv->val[0], tlv->val[1], tlv->val[2],
                                        tlv->val[3]);
-                       break;
-
-               case A2MP_PAL_VER_INFO:
-                       if (tlvlen != 5)
                                break;
-                       ver = (struct amp_pal_ver *) tlv->val;
-                       printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",
+
+                       case A2MP_PAL_VER_INFO:
+                               if (tlvlen != 5)
+                                       break;
+                               ver = (struct amp_pal_ver *) tlv->val;
+                               printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",
                                        ver->ver, btohs(ver->company_id),
                                        btohs(ver->sub_ver));
-                       break;
+                               break;
 
-               default:
-                       printf("Unrecognized type %d\n", tlv->type);
-                       break;
-               }
+                       default:
+                               printf("Unrecognized type %d\n", tlv->type);
+                               break;
+                       }
 
-               len -= tlvlen + sizeof(*tlv);
-               assoc += tlvlen + sizeof(*tlv);
-               tlv = (struct amp_tlv *) assoc;
+                       len -= tlvlen + sizeof(*tlv);
+                       assoc += tlvlen + sizeof(*tlv);
+                       tlv = (struct amp_tlv *) assoc;
+               } else
+                       break;
        }
 }
--

Quoted 1 line

2.19.1

Please fix these:

Applying: hcidump: fixed AMP Assoc dump heap-over-flow WARNING:LONG_LINE_STRING: line over 80 characters #28: FILE: tools/parser/amp.c:88:
+ printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",

WARNING:LONG_LINE_STRING: line over 80 characters
#42: FILE: tools/parser/amp.c:94:
+ amp_dump_chanlist(level, tlv, "Preferred Chan List");

WARNING:LONG_LINE_STRING: line over 80 characters
#48: FILE: tools/parser/amp.c:98:
+ amp_dump_chanlist(level, tlv, "Connected Chan List");

WARNING:LONG_LINE_STRING: line over 80 characters
#70: FILE: tools/parser/amp.c:113:
+ printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",

Signature · 1 line

Luiz Augusto von Dentz

Cho, Yu-Chen

author

2018-11-06 06:57 UTC #3 lore raw

PATCH 1/3 hcidump: fixed AMP Assoc dump heap-over-flow

Signed-off-by Cho, Yu-Chen <[email protected]>

---
 tools/parser/amp.c | 69 +++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 31 deletions(-)

tools/parser/amp.c +38 ~~-31~~

index 158ca4a75..420099c90 100644
@@ -33,7 +33,7 @@ static void amp_dump_chanlist(int level, struct amp_tlv *tlv, char *prefix)
 	struct amp_country_triplet *triplet;
 	int i, num;
 
-	num = (tlv->len - sizeof(*chan_list)) / sizeof(*triplet);
+	num = sizeof(*chan_list->triplets) / sizeof(*chan_list->triplets[0]);
 
 	printf("%s (number of triplets %d)\n", prefix, num);

@@ -80,47 +80,54 @@ void amp_assoc_dump(int level, uint8_t *assoc, uint16_t len)
 
 		p_indent(level+1, 0);
 
-		switch (tlv->type) {
-		case A2MP_MAC_ADDR_TYPE:
-			if (tlvlen != 6)
-				break;
-			printf("MAC: %2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
+		if (tlvlen != 0) {
+			switch (tlv->type) {
+			case A2MP_MAC_ADDR_TYPE:
+				if (tlvlen != 6)
+					break;
+				printf("MAC: ");
+				printf("%2.2X:%2.2X:%2.2X:%2.2X:%2.2X:%2.2X\n",
 					tlv->val[0], tlv->val[1], tlv->val[2],
 					tlv->val[3], tlv->val[4], tlv->val[5]);
-			break;
-
-		case A2MP_PREF_CHANLIST_TYPE:
-			amp_dump_chanlist(level, tlv, "Preferred Chan List");
-			break;
+				break;
 
-		case A2MP_CONNECTED_CHAN:
-			amp_dump_chanlist(level, tlv, "Connected Chan List");
-			break;
+			case A2MP_PREF_CHANLIST_TYPE:
+				amp_dump_chanlist(level, tlv,
+						   "Preferred Chan List");
+				break;
 
-		case A2MP_PAL_CAP_TYPE:
-			if (tlvlen != 4)
+			case A2MP_CONNECTED_CHAN:
+				amp_dump_chanlist(level, tlv,
+						   "Connected Chan List");
 				break;
-			printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
+
+			case A2MP_PAL_CAP_TYPE:
+				if (tlvlen != 4)
+					break;
+				printf("PAL CAP: %2.2x %2.2x %2.2x %2.2x\n",
 					tlv->val[0], tlv->val[1], tlv->val[2],
 					tlv->val[3]);
-			break;
-
-		case A2MP_PAL_VER_INFO:
-			if (tlvlen != 5)
 				break;
-			ver = (struct amp_pal_ver *) tlv->val;
-			printf("PAL VER: %2.2x Comp ID: %4.4x SubVer: %4.4x\n",
+
+			case A2MP_PAL_VER_INFO:
+				if (tlvlen != 5)
+					break;
+				ver = (struct amp_pal_ver *) tlv->val;
+				printf("PAL VER: ");
+				printf("%2.2x Comp ID: %4.4x SubVer: %4.4x\n",
 					ver->ver, btohs(ver->company_id),
 					btohs(ver->sub_ver));
-			break;
+				break;
 
-		default:
-			printf("Unrecognized type %d\n", tlv->type);
-			break;
-		}
+			default:
+				printf("Unrecognized type %d\n", tlv->type);
+				break;
+			}
 
-		len -= tlvlen + sizeof(*tlv);
-		assoc += tlvlen + sizeof(*tlv);
-		tlv = (struct amp_tlv *) assoc;
+			len -= tlvlen + sizeof(*tlv);
+			assoc += tlvlen + sizeof(*tlv);
+			tlv = (struct amp_tlv *) assoc;
+		} else
+			break;
 	}
 }

Signature · 1 line

2.19.1

Cho, Yu-Chen

author

2018-10-31 08:15 UTC #4 lore raw

PATCH 2/3 hcidump:fixed hci frame dump stack-buffer-overflow

In reply to Cho, Yu-Chen

hci_dump() didn't check the length of frame, and it would be a stack-buffer-overflow error.

Signed-off-by Cho, Yu-Chen <[email protected]>

---
 tools/parser/hci.c | 3 +++
 1 file changed, 3 insertions(+)

tools/parser/hci.c +3 -0

index 8c7bd2581..4e6c36040 100644
@@ -4107,6 +4107,9 @@ void hci_dump(int level, struct frame *frm)
 
 	frm->ptr++; frm->len--;
 
+	if (frm->len == 0)
+		return;
+
 	switch (type) {
 	case HCI_COMMAND_PKT:
 		command_dump(level, frm);

Signature · 1 line

2.19.1

Luiz Augusto von Dentz

maintainer Applied

2018-11-02 11:12 UTC #5 lore raw

Re: [PATCH 2/3 BlueZ] hcidump:fixed hci frame dump stack-buffer-overflow

Hi,

Quoted 24 lines · Yu-Chen

hci_dump() didn't check the length of frame, and it would be a stack-buffer-overflow error.

Signed-off-by Cho, Yu-Chen <[email protected]>

---
 tools/parser/hci.c | 3 +++
 1 file changed, 3 insertions(+)

tools/parser/hci.c +3 -1

index 8c7bd2581..4e6c36040 100644
@@ -4107,6 +4107,9 @@ void hci_dump(int level, struct frame *frm)
 
        frm->ptr++; frm->len--;
 
+       if (frm->len == 0)
+               return;
+
        switch (type) {
        case HCI_COMMAND_PKT:
                command_dump(level, frm);
--

Quoted 1 line

2.19.1

Applied.

Signature · 1 line

Luiz Augusto von Dentz

Cho, Yu-Chen

author

2018-10-31 08:15 UTC #6 lore raw

PATCH 3/3 hcidump: Fix set_ext_ctrl() global buffer overflow

In reply to Cho, Yu-Chen

Fix set_ext_ctrl() global buffer overflow.

Signed-off-by Cho, Yu-Chen <[email protected]>

---
 tools/parser/l2cap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/parser/l2cap.c +1 -1

index a05796482..5daefcbaa 100644
@@ -56,7 +56,7 @@ typedef struct {
 	uint8_t mode;
 	uint8_t ext_ctrl;
 } cid_info;
-#define CID_TABLE_SIZE 20
+#define CID_TABLE_SIZE 32
 
 static cid_info cid_table[2][CID_TABLE_SIZE];

Signature · 1 line

2.19.1

Luiz Augusto von Dentz

maintainer Applied

2018-11-02 11:12 UTC #7 lore raw

Re: [PATCH 3/3 BlueZ] hcidump: Fix set_ext_ctrl() global buffer overflow

Hi,

Quoted 22 lines · Yu-Chen

Fix set_ext_ctrl() global buffer overflow.

Signed-off-by Cho, Yu-Chen <[email protected]>

---
 tools/parser/l2cap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tools/parser/l2cap.c +1 -2

index a05796482..5daefcbaa 100644
@@ -56,7 +56,7 @@ typedef struct {
        uint8_t mode;
        uint8_t ext_ctrl;
 } cid_info;
-#define CID_TABLE_SIZE 20
+#define CID_TABLE_SIZE 32
 
 static cid_info cid_table[2][CID_TABLE_SIZE];
 
--

Quoted 1 line

2.19.1

Applied, thanks.

Signature · 1 line

Luiz Augusto von Dentz