lib: parse_canframe: guard delimiter reads with length checks

As Alex J pointed out in comment
https://github.com/linux-can/can-utils/issues/632#issuecomment-5746692502
the delimiters to detect CAN CC/FD/XL frames were read from the input
data without checking the length of that input data. This could lead
to an out-of-bounds read and to unintended detection of incorrect content.

Add a length check before reading those delimiters and also add/change
some comments to clarify the reasons for some assignments.

Reported-by: Alex J <jipaionut@gmail.com>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

isotpsniffer.c

@@ -59,7 +59,6 @@
 #include <linux/can.h>
 #include <linux/can/isotp.h>
 #include <linux/sockios.h>
-#include <errno.h>
 
 #define NO_CAN_ID 0xFFFFFFFFU
 
@@ -67,8 +66,6 @@
 #define FORMAT_ASCII 2
 #define FORMAT_DEFAULT (FORMAT_ASCII | FORMAT_HEX)
 
-#define PDU_BUF_SIZE 4096
-
 void print_usage(char *prg)
 {
 	fprintf(stderr, "\nUsage: %s [options] <CAN interface>\n", prg);
@@ -82,7 +79,6 @@ void print_usage(char *prg)
 	fprintf(stderr, "         -f <format>  (1 = HEX, 2 = ASCII, 3 = HEX & ASCII - default: %d)\n", FORMAT_DEFAULT);
 	fprintf(stderr, "         -L           (set link layer options for CAN FD)\n");
 	fprintf(stderr, "         -h <len>     (head: print only first <len> bytes)\n");
-	fprintf(stderr, "         -i           (ignore syscall errors to receive malformed PDUs)\n");
 	fprintf(stderr, "\nCAN IDs and addresses are given and expected in hexadecimal values.\n");
 	fprintf(stderr, "\n");
 }
@@ -193,16 +189,15 @@ int main(int argc, char **argv)
 	int head = 0;
 	int timestamp = 0;
 	int format = FORMAT_DEFAULT;
-	int ignore_errors = 0;
 	canid_t src = NO_CAN_ID;
 	canid_t dst = NO_CAN_ID;
 	extern int optind, opterr, optopt;
 	static struct timeval tv, last_tv;
 
-	unsigned char buffer[PDU_BUF_SIZE];
+	unsigned char buffer[4096];
 	int nbytes;
 
-	while ((opt = getopt(argc, argv, "s:d:x:X:h:ct:f:L?i")) != -1) {
+	while ((opt = getopt(argc, argv, "s:d:x:X:h:ct:f:L?")) != -1) {
 		switch (opt) {
 		case 's':
 			src = strtoul(optarg, NULL, 16);
@@ -254,10 +249,6 @@ int main(int argc, char **argv)
 			}
 			break;
 
-		case 'i':
-			ignore_errors = 1;
-			break;
-
 		case '?':
 			print_usage(basename(argv[0]));
 			goto out;
@@ -376,39 +367,33 @@ int main(int argc, char **argv)
 		}
 
 		if (FD_ISSET(s, &rdfs)) {
-			nbytes = read(s, buffer, PDU_BUF_SIZE);
+			nbytes = read(s, buffer, 4096);
 			if (nbytes < 0) {
 				perror("read socket s");
 				r = 1;
-				if(!ignore_errors)
-					goto out;
-			}
-			if (nbytes > (PDU_BUF_SIZE - 1)) {
-				r = 1;
-				fprintf(stderr, "PDU length %d longer than PDU buffer: %s\n", nbytes, strerror(errno));
 				goto out;
 			}
-			if(nbytes > 0)
-				printbuf(buffer, nbytes, color?2:0, timestamp, format,
-					 &tv, &last_tv, dst, s, if_name, head);
+			if (nbytes > 4095) {
+				r = 1;
+				goto out;
+			}
+			printbuf(buffer, nbytes, color?2:0, timestamp, format,
+				 &tv, &last_tv, dst, s, if_name, head);
 		}
 
 		if (FD_ISSET(t, &rdfs)) {
-			nbytes = read(t, buffer, PDU_BUF_SIZE);
+			nbytes = read(t, buffer, 4096);
 			if (nbytes < 0) {
 				perror("read socket t");
 				r = 1;
-				if(!ignore_errors)
-					goto out;
-			}
-			if (nbytes > (PDU_BUF_SIZE - 1)) {
-				r = 1;
-				fprintf(stderr, "PDU length %d longer than PDU buffer: %s\n", nbytes, strerror(errno));
 				goto out;
 			}
-			if(nbytes > 0)
-				printbuf(buffer, nbytes, color?1:0, timestamp, format,
-					 &tv, &last_tv, src, t, if_name, head);
+			if (nbytes > 4095) {
+				r = 1;
+				goto out;
+			}
+			printbuf(buffer, nbytes, color?1:0, timestamp, format,
+				 &tv, &last_tv, src, t, if_name, head);
 		}
 	}
 

lib.c

@@ -170,21 +170,23 @@ int parse_canframe(char *cs, union cfu *cu)
 
 	memset(cu, 0, sizeof(*cu)); /* init CAN CC/FD/XL frame, e.g. LEN = 0 */
 
-	if (len < 4)
-		return 0;
+	if (len >= 4 && cs[3] == CANID_DELIM) { /* 3 digits SFF CAN ID */
 
-	if (cs[3] == CANID_DELIM) { /* 3 digits SFF */
+		idx = 4; /* start of frame data */
 
-		idx = 4;
+		/* get 3 digits SFF CAN ID value */
 		for (i = 0; i < 3; i++) {
 			if ((tmp = asc2nibble(cs[i])) > 0x0F)
 				return 0;
 			cu->cc.can_id |= tmp << (2 - i) * 4;
 		}
 
-	} else if (cs[5] == CANID_DELIM) { /* 5 digits CAN XL VCID/PRIO*/
+	} else if (len >= 21 && cs[5] == CANID_DELIM && cs[20] == CANID_DELIM) {
+		/* 5 digits CAN XL VCID/PRIO - but also check for 2nd '#' here */
 
-		idx = 6;
+		idx = 6; /* start of CAN XL frame extra content (AF, SDT, etc) */
+
+		/* get 5 digits CAN XL VCID/PRIO */
 		for (i = 0; i < 5; i++) {
 			if ((tmp = asc2nibble(cs[i])) > 0x0F)
 				return 0;
@@ -196,9 +198,11 @@ int parse_canframe(char *cs, union cfu *cu)
 		cu->xl.prio &= CANXL_PRIO_MASK;
 		cu->xl.prio |= tmp;
 
-	} else if (cs[8] == CANID_DELIM) { /* 8 digits EFF */
+	} else if (len >= 9 && cs[8] == CANID_DELIM) { /* 8 digits EFF CAN ID */
 
-		idx = 9;
+		idx = 9; /* start of frame data */
+
+		/* get 8 digits EFF CAN ID value */
 		for (i = 0; i < 8; i++) {
 			if ((tmp = asc2nibble(cs[i])) > 0x0F)
 				return 0;
@@ -239,11 +243,12 @@ int parse_canframe(char *cs, union cfu *cu)
 		cu->fd.flags |= CANFD_FDF; /* dual-use */
 		idx += 2;
 
-	} else if (cs[idx + 14] == CANID_DELIM) { /* CAN XL frame '#80:00:11223344#' */
+	} else if (idx == 6) { /* CAN XL frame extra content '#80:00:11223344#' */
 		maxdlen = CANXL_MAX_DLEN;
 		mtu = CANXL_MTU;
-		data = cu->xl.data; /* fill CAN XL data */
+		data = cu->xl.data; /* overwrite pointer to CAN XL data */
 
+		/* get CAN XL frame extra content */
 		if ((cs[idx + 2] != XL_HDR_DELIM) || (cs[idx + 5] != XL_HDR_DELIM))
 			return 0;
 
@@ -277,6 +282,7 @@ int parse_canframe(char *cs, union cfu *cu)
 		idx++; /* skip CANID_DELIM */
 	}
 
+	/* copy CAN frame data content */
 	for (i = 0, dlen = 0; i < maxdlen; i++) {
 		if (cs[idx] == DATA_SEPERATOR) /* skip (optional) separator */
 			idx++;