[BlueZ 0/2] adv_monitor: Fix buffer overflow caused by integer overflow

[BlueZ 0/2] adv_monitor: Fix buffer overflow caused by integer overflow

[Bastien Nocera]

Those issues were originally reported as security issues, but we not
deemed worthy of a security advisory as they impacted an experimental
feature.

Bastien Nocera (2):
  adv_monitor: Fix buffer overflow caused by integer overflow
  adv_monitor: Ignore additional patterns past what's supported

 src/adv_monitor.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

-- 
2.55.0

[BlueZ 1/2] adv_monitor: Fix buffer overflow caused by integer overflow

[Bastien Nocera]

In src/adv_monitor.c, merged_pattern_send_add_pattern(), and with a
large merged_pattern->patterns list, it's possible to overflow cp_len,
an 8-bit integer:
pattern_count = queue_length(merged_pattern->patterns);
cp_len = sizeof(*cp) + pattern_count * sizeof(struct mgmt_adv_pattern);

Eight patterns require 273 bytes of command storage, but assigning
that result to uint8_t cp_len wraps it to 17 before allocation.

The loop that follows does not use the truncated size; it still copies
all eight 34-byte struct mgmt_adv_pattern records into the heap object.
The mismatch between the wrapped allocation size and the full copy
volume produces a large, deterministic heap overwrite.

Fix this in 2 ways in the function itself:
1) increase the size of cp_len, as 8 patterns would overflow it, and some
   typical Bluetooth devices can support 16 patterns
2) Store the result of the multiplication in a 64-bit integer before
   checking whether it's bigger than our 16-bit cp_len

A similar bug exists in merged_pattern_send_add_pattern_rssi().

Reported-by: @ax-nnlabs (for merged_pattern_send_add_pattern())
Reported-by: Aisle Research (for merged_pattern_send_add_pattern_rssi())
---
 src/adv_monitor.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 87fa824b35d8..7f437a0e7c8f 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -1106,12 +1106,15 @@ static bool merged_pattern_send_add_pattern(
 			struct adv_monitor_merged_pattern *merged_pattern)
 {
 	struct mgmt_cp_add_adv_monitor *cp = NULL;
-	uint8_t pattern_count, cp_len;
+	uint64_t cp_len;
+	uint8_t pattern_count;
 	const struct queue_entry *e;
 	bool success = true;
 
 	pattern_count = queue_length(merged_pattern->patterns);
 	cp_len = sizeof(*cp) + pattern_count * sizeof(struct mgmt_adv_pattern);
+	if (cp_len > UINT16_MAX)
+		return false;
 
 	cp = malloc0(cp_len);
 	if (!cp)
@@ -1126,8 +1129,9 @@ static bool merged_pattern_send_add_pattern(
 
 	if (!mgmt_send(merged_pattern->manager->mgmt,
 			MGMT_OP_ADD_ADV_PATTERNS_MONITOR,
-			merged_pattern->manager->adapter_id, cp_len, cp,
-			add_adv_patterns_monitor_cb, merged_pattern, NULL)) {
+			merged_pattern->manager->adapter_id, (uint16_t) cp_len,
+			cp, add_adv_patterns_monitor_cb, merged_pattern,
+			NULL)) {
 		error("Unable to send Add Adv Patterns Monitor command");
 		success = false;
 	}
@@ -1141,12 +1145,15 @@ static bool merged_pattern_send_add_pattern_rssi(
 			struct adv_monitor_merged_pattern *merged_pattern)
 {
 	struct mgmt_cp_add_adv_patterns_monitor_rssi *cp = NULL;
-	uint8_t pattern_count, cp_len;
+	uint64_t cp_len;
+	uint8_t pattern_count;
 	const struct queue_entry *e;
 	bool success = true;
 
 	pattern_count = queue_length(merged_pattern->patterns);
 	cp_len = sizeof(*cp) + pattern_count * sizeof(struct mgmt_adv_pattern);
+	if (cp_len > UINT16_MAX)
+		return false;
 
 	cp = malloc0(cp_len);
 	if (!cp)
@@ -1169,8 +1176,9 @@ static bool merged_pattern_send_add_pattern_rssi(
 
 	if (!mgmt_send(merged_pattern->manager->mgmt,
 			MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI,
-			merged_pattern->manager->adapter_id, cp_len, cp,
-			add_adv_patterns_monitor_cb, merged_pattern, NULL)) {
+			merged_pattern->manager->adapter_id, (uint16_t) cp_len,
+			cp, add_adv_patterns_monitor_cb, merged_pattern,
+			NULL)) {
 		error("Unable to send Add Adv Patterns Monitor RSSI command");
 		success = false;
 	}
-- 
2.55.0

[BlueZ 2/2] adv_monitor: Ignore additional patterns past what's supported

[Bastien Nocera]

Ignore patterns beyond what the adapter supports.

Suggested-by: Aisle Research
---
 src/adv_monitor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/adv_monitor.c b/src/adv_monitor.c
index 7f437a0e7c8f..c7de01f72a26 100644
--- a/src/adv_monitor.c
+++ b/src/adv_monitor.c
@@ -889,6 +889,7 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
 {
 	DBusMessageIter array, array_iter;
 	uint16_t adapter_id = monitor->app->manager->adapter_id;
+	uint8_t max_num_patterns = monitor->app->manager->max_num_patterns;
 
 	if (!g_dbus_proxy_get_property(monitor->proxy, "Patterns", &array)) {
 		btd_error(adapter_id,
@@ -950,6 +951,10 @@ static bool parse_patterns(struct adv_monitor *monitor, const char *path)
 
 		queue_push_tail(monitor->merged_pattern->patterns, pattern);
 
+		if (queue_length(monitor->merged_pattern->patterns) >=
+			max_num_patterns)
+			break;
+
 		dbus_message_iter_next(&array_iter);
 	}
 
-- 
2.55.0

RE: adv_monitor: Fix buffer overflow caused by integer overflow

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3364 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1145281

---Test result---

Test Summary:
CheckPatch                    FAIL      0.85 seconds
GitLint                       PASS      0.45 seconds
BuildEll                      PASS      20.54 seconds
BluezMake                     PASS      601.93 seconds
CheckSmatch                   WARNING   325.33 seconds
bluezmakeextell               PASS      102.25 seconds
IncrementalBuild              PASS      609.37 seconds
ScanBuild                     PASS      989.74 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,1/2] adv_monitor: Fix buffer overflow caused by integer overflow
ERROR:BAD_SIGN_OFF: Unrecognized email address: '@ax-nnlabs (for merged_pattern_send_add_pattern())'
#76: 
Reported-by: @ax-nnlabs (for merged_pattern_send_add_pattern())

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Aisle Research (for merged_pattern_send_add_pattern_rssi())'
#77: 
Reported-by: Aisle Research (for merged_pattern_send_add_pattern_rssi())

/github/workspace/src/patch/14747617.patch total: 2 errors, 0 warnings, 54 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14747617.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[BlueZ,2/2] adv_monitor: Ignore additional patterns past what's supported
ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Aisle Research'
#56: 
Suggested-by: Aisle Research

/github/workspace/src/patch/14747616.patch total: 1 errors, 0 warnings, 17 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14747616.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/adv_monitor.c: note: in included file:./src/shared/mgmt.h:95:25: error: redefinition of unsigned int enum mgmt_io_capabilitysrc/adv_monitor.c: note: in included file:./src/shared/mgmt.h:95:25: error: redefinition of unsigned int enum mgmt_io_capabilitysrc/adv_monitor.c: note: in included file:./src/shared/mgmt.h:95:25: error: redefinition of unsigned int enum mgmt_io_capabilitysrc/adv_monitor.c: note: in included file:./src/shared/mgmt.h:95:25: error: redefinition of unsigned int enum mgmt_io_capability


https://github.com/bluez/bluez/pull/2398

---
Regards,
Linux Bluetooth