[PATCH] monitor: clamp max_len in print_packet

[PATCH] monitor: clamp max_len in print_packet

[Chad Spensky]

print_packet() derives the snprintf() bound for the packet text from the
terminal width and never bounds it against the space left in line[]:

	char line[LINE_MAX], ts_str[96], pid_str[140];
	int col = num_columns();
	...
	int max_len = col - len - extra_len - ts_len - 3;

	if (max_len <= 0) {
		extra = NULL;
		max_len = col - len - ts_len - 3;
	}

	n = snprintf(line + pos, max_len + 1, "%s%s",
					label ? ": " : "", text);

max_len can leave range in both directions, and either aborts under
_FORTIFY_SOURCE with "*** buffer overflow detected ***: terminated":

 - The existing max_len <= 0 recovery drops extra and recomputes, but if
   the prefix alone exceeds the column budget the result is still
   negative, and max_len + 1 then underflows when converted to size_t.
   len includes the "comm[pid]: " prefix built from struct ucred, which
   is only present when reading from the monitor socket, so this is
   reachable at ordinary widths. On a host with a large kernel.pid_max
   the pid is 7 digits, so a long process name plus a long label is
   enough and btmon dies mid-capture. Replaying the same traffic from a
   btsnoop file never reproduces it, because there is no ucred and hence
   no prefix.

 - col larger than sizeof(line) makes max_len + 1 exceed the remaining
   buffer. LINE_MAX raised the bar but did not remove it.

A negative max_len is also used to index line[] when truncating the
text, writing before the start of the buffer, so this is an
out-of-bounds write and not only an abort.

The check only triggers at _FORTIFY_SOURCE=3; at =2 the runtime pos
offset defeats __builtin_object_size and the check is elided, which is
why this is mostly seen on distributions defaulting to =3.

Clamp max_len to the space remaining in line[], and only write the
ellipsis when there is room for it.

Reproduced and verified by building the current logic and the patched
logic with -O2 -D_FORTIFY_SOURCE=3 and replaying a capture at a range of
terminal widths:

	cols   before   after
	  10   abort    ok
	  20   abort    ok
	  40   abort    ok
	  80   ok       ok
	 200   ok       ok
	2000   ok       ok
	3000   abort    ok
	5000   abort    ok

Reported-at: https://github.com/bluez/bluez/issues/1104
---
 monitor/packet.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/monitor/packet.c b/monitor/packet.c
index 0d3b23cc3..df6c0a819 100644
--- a/monitor/packet.c
+++ b/monitor/packet.c
@@ -603,13 +603,26 @@ static void print_packet(struct timeval *tv, struct ucred *cred, char ident,
 			max_len = col - len - ts_len - 3;
 		}
 
+		/* col comes from the terminal and len includes the optional
+		 * "comm[pid]: " prefix, so max_len can still be negative
+		 * here, or larger than the space left in line[]. Both
+		 * overflow the snprintf below, and a negative value also
+		 * indexes before line[].
+		 */
+		if (max_len > (int) sizeof(line) - pos - 1)
+			max_len = (int) sizeof(line) - pos - 1;
+		if (max_len < 0)
+			max_len = 0;
+
 		n = snprintf(line + pos, max_len + 1, "%s%s",
 						label ? ": " : "", text);
 		if (n > max_len) {
-			line[pos + max_len - 1] = '.';
-			line[pos + max_len - 2] = '.';
-			if (line[pos + max_len - 3] == ' ')
-				line[pos + max_len - 3] = '.';
+			if (max_len >= 3) {
+				line[pos + max_len - 1] = '.';
+				line[pos + max_len - 2] = '.';
+				if (line[pos + max_len - 3] == ' ')
+					line[pos + max_len - 3] = '.';
+			}
 
 			n = max_len;
 		}
-- 
2.43.0

RE: monitor: clamp max_len in print_packet

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2833 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=1146334

---Test result---

Test Summary:
CheckPatch                    PASS      0.49 seconds
GitLint                       FAIL      0.34 seconds
BuildEll                      PASS      20.71 seconds
BluezMake                     PASS      583.87 seconds
MakeCheck                     PASS      0.98 seconds
MakeDistcheck                 PASS      156.41 seconds
CheckValgrind                 PASS      150.41 seconds
CheckSmatch                   WARNING   298.13 seconds
bluezmakeextell               PASS      96.55 seconds
IncrementalBuild              PASS      563.82 seconds
ScanBuild                     PASS      905.21 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
monitor: clamp max_len in print_packet

6: B3 Line contains hard tab characters (\t): "	char line[LINE_MAX], ts_str[96], pid_str[140];"
7: B3 Line contains hard tab characters (\t): "	int col = num_columns();"
8: B3 Line contains hard tab characters (\t): "	..."
9: B3 Line contains hard tab characters (\t): "	int max_len = col - len - extra_len - ts_len - 3;"
11: B3 Line contains hard tab characters (\t): "	if (max_len <= 0) {"
12: B3 Line contains hard tab characters (\t): "		extra = NULL;"
13: B3 Line contains hard tab characters (\t): "		max_len = col - len - ts_len - 3;"
14: B3 Line contains hard tab characters (\t): "	}"
16: B3 Line contains hard tab characters (\t): "	n = snprintf(line + pos, max_len + 1, "%s%s","
17: B3 Line contains hard tab characters (\t): "					label ? ": " : "", text);"
51: B3 Line contains hard tab characters (\t): "	cols   before   after"
52: B3 Line contains hard tab characters (\t): "	  10   abort    ok"
53: B3 Line contains hard tab characters (\t): "	  20   abort    ok"
54: B3 Line contains hard tab characters (\t): "	  40   abort    ok"
55: B3 Line contains hard tab characters (\t): "	  80   ok       ok"
56: B3 Line contains hard tab characters (\t): "	 200   ok       ok"
57: B3 Line contains hard tab characters (\t): "	2000   ok       ok"
58: B3 Line contains hard tab characters (\t): "	3000   abort    ok"
59: B3 Line contains hard tab characters (\t): "	5000   abort    ok"
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
monitor/packet.c:2015:26: warning: Variable length array is used.monitor/packet.c: note: in included file:monitor/bt.h:3924:52: warning: array of flexible structuresmonitor/bt.h:3912:40: warning: array of flexible structures


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

---
Regards,
Linux Bluetooth