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>

canplayer-bisect

@@ -1,205 +0,0 @@
-#!/bin/bash
-
-if [ -z "${CANPLAYER}" ]; then
-	CANPLAYER="canplayer"
-fi
-
-die() {
-	echo "$*" > /dev/stderr
-	exit 1
-}
-
-usage() {
-	echo "canplayer-bisect <start|stop|clean|good|yes|bad|no|again|where|undo> <logfile> <canplayer options>"
-}
-
-is_ready() {
-	if [ ! -d .canplayer-bisect ]; then
-		usage
-		exit 1
-	fi
-
-	return 0
-}
-
-setup() {
-	is_ready
-
-	LOGFILE=$(cat .canplayer-bisect/logfile |head -n 1)
-
-	SAVED_LEN="$(cat .canplayer-bisect/len|tail -n 1)"
-	LEN="$(wc -l ${LOGFILE} | awk '{ print $1 }')"
-
-	if [ "$LEN" != "$SAVED_LEN" ]; then
-		die "logfile has changed size. restart"
-	fi
-
-	CANPLAYER_ARGS=$(cat .canplayer-bisect/args |head -n 1)
-
-	HEAD="$(cat .canplayer-bisect/head |tail -n 1)"
-	TAIL="$(cat .canplayer-bisect/tail |tail -n 1)"
-}
-
-back() {
-	HEAD="$(cat .canplayer-bisect/head |tail -n 2 |head -n1)"
-	TAIL="$(cat .canplayer-bisect/tail |tail -n 2 |head -n1)"
-}
-
-do_undo() {
-	sed -i '$ d' .canplayer-bisect/head
-	sed -i '$ d' .canplayer-bisect/tail
-}
-
-teardown() {
-	mkdir -p .canplayer-bisect
-	echo $LEN > .canplayer-bisect/len
-	echo $LOGFILE > .canplayer-bisect/logfile
-	echo $CANPLAYER_ARGS > .canplayer-bisect/args
-
-	echo $HEAD >> .canplayer-bisect/head
-	echo $TAIL >> .canplayer-bisect/tail
-}
-
-show() {
-	cat $LOGFILE | sed -n ${HEAD},${TAIL}p
-}
-
-play() {
-	#we *could* pipe directly to canplayer, but then the user can't add -l i to CANPLAYER_ARGS to hunt for packets using looped playback
-	the_show="$(mktemp)"
-	trap "rm -rf \"${the_show}\"" EXIT
-
-	show > "${the_show}"
-	"${CANPLAYER}" ${CANPLAYER_ARGS} -I "${the_show}"
-}
-
-do_show() {
-	setup
-
-	show
-}
-
-check_heads_n_tails() {
-	if [ $HEAD -eq $TAIL ]; then
-		do_stop
-	fi
-}
-
-do_good() {
-	setup
-
-	check_heads_n_tails
-
-	if [ $(( $HEAD + 1 )) -eq $TAIL ]; then
-		TAIL=$HEAD
-	else
-		TAIL=$(( ( $TAIL - $HEAD ) / 2 + $HEAD - 1 ))
-	fi
-
-	teardown
-	play
-}
-
-do_bad() {
-	setup
-
-	check_heads_n_tails
-
-	back
-	if [ $(( $HEAD + 1 )) -eq $TAIL ]; then
-		HEAD=$TAIL
-	else
-		HEAD=$(( ( $TAIL - $HEAD ) / 2 + $HEAD ))
-	fi
-
-	teardown
-	play
-}
-
-do_again() {
-	setup
-	play
-}
-
-do_start() {
-	do_clean
-
-	LEN="$(wc -l ${LOGFILE} | awk '{ print $1 }')"
-
-	HEAD=1
-	TAIL=$LEN
-
-	echo "assuming logfile contains the packets you seek... bisecting to first half"
-
-	teardown
-	play
-}
-
-do_where() {
-	setup
-	echo "between $HEAD and $TAIL (+$(( $TAIL - $HEAD ))) of $LOGFILE"
-}
-
-do_stop() {
-	setup
-
-	if [ "$COMMAND" == "no" ]; then
-		echo "failed to find what you were looking for"
-		exit 1
-	else
-		echo "the packets you seek are:"
-		do_where
-		exit 0
-	fi
-}
-
-do_clean() {
-	rm -rf .canplayer-bisect
-}
-
-if [ -z "$1" ]; then
-	usage
-	exit 1
-fi
-COMMAND=$1
-
-if [ ! -d .canplayer-bisect ] && [ ! -z "$2" ] && [ ! -e "$2" ]; then
-	usage
-	exit 1
-fi
-LOGFILE="$2"
-
-shift
-shift
-CANPLAYER_ARGS="$*"
-
-case "$COMMAND" in
-	start)
-		do_start
-	;;
-	stop)
-		do_stop
-	;;
-	clean)
-		do_clean
-	;;
-	good|yes)
-		do_good
-	;;
-	bad|no)
-		do_bad
-	;;
-	again)
-		do_again
-	;;
-	where)
-		do_where
-	;;
-	undo)
-		do_undo
-	;;
-	show)
-		do_show
-	;;
-esac
-

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++;