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>

canlogserver.c

@@ -177,7 +177,8 @@ int main(int argc, char **argv)
 	sigset_t sigset;
 	fd_set rdfs;
 	int s[MAXDEV];
-	int socki, accsocket;
+	int socki;
+	int accsocket = -1;
 	canid_t mask[MAXDEV] = {0};
 	canid_t value[MAXDEV] = {0};
 	int inv_filter[MAXDEV] = {0};
@@ -286,7 +287,7 @@ int main(int argc, char **argv)
 	inaddr.sin_addr.s_addr = htonl(INADDR_ANY);
 	inaddr.sin_port = htons(port);
 
-	while(bind(socki, (struct sockaddr*)&inaddr, sizeof(inaddr)) < 0) {
+	while(running && bind(socki, (struct sockaddr*)&inaddr, sizeof(inaddr)) < 0) {
 		struct timespec f = {
 			.tv_nsec = 100 * 1000 * 1000,
 		};
@@ -295,18 +296,28 @@ int main(int argc, char **argv)
 		nanosleep(&f, NULL);
 	}
 
+	/*
+	 * Check if loop exited due to signal (during nanosleep) rather than
+	 * successful bind.
+	 */
+	if (!running) {
+		close(socki);
+		return 128 + signal_num;
+	}
+
 	if (listen(socki, 3) != 0) {
 		perror("listen");
 		exit(1);
 	}
 
-	while(1) {
+	while(running) {
 		accsocket = accept(socki, (struct sockaddr*)&clientaddr, &sin_size);
 		if (accsocket > 0) {
 			//printf("accepted\n");
 			if (!fork())
 				break;
 			close(accsocket);
+			accsocket = -1;
 		}
 		else if (errno != EINTR) {
 			perror("accept");
@@ -314,6 +325,17 @@ int main(int argc, char **argv)
 		}
 	}
 
+	/*
+	 * Check if loop exited due to signal (accept returned EINTR) rather
+	 * than successful fork
+	 */
+	if (!running) {
+		if (accsocket > 0)
+			close(accsocket);
+		close(socki);
+		return 128 + signal_num;
+	}
+
 	for (i=0; i<currmax; i++) {
 
 		pr_debug("open %d '%s' m%08X v%08X i%d e%d.\n",

j1939_timedate/j1939_timedate_srv.c

@@ -41,16 +41,22 @@ static void gmtime_to_j1939_pgn_65254_td(struct j1939_time_date_packet *tdp)
 	utc_tm = gmtime_r(&now, &utc_tm_buf);
 	local_tm = localtime_r(&now, &local_tm_buf);
 
-	/* Calculate the offsets */
+	if (local_tm) {
-	hour_offset = local_tm->tm_hour - utc_tm->tm_hour;
+		/* Calculate the offsets */
-	minute_offset = local_tm->tm_min - utc_tm->tm_min;
+		hour_offset = local_tm->tm_hour - utc_tm->tm_hour;
+		minute_offset = local_tm->tm_min - utc_tm->tm_min;
 
-	/* Handle date rollover */
+		/* Handle date rollover */
-	if (local_tm->tm_mday != utc_tm->tm_mday) {
+		if (local_tm->tm_mday != utc_tm->tm_mday) {
-		if (local_tm->tm_hour < 12)
+			if (local_tm->tm_hour < 12)
-			hour_offset += 24;  /* past midnight */
+				hour_offset += 24;  /* past midnight */
-		else
+			else
-			hour_offset -= 24;  /* before midnight */
+				hour_offset -= 24;  /* before midnight */
+		}
+	} else {
+		/* The local time offsets cannot be determined at the moment */
+		hour_offset = 0xF9;
+		minute_offset = 0xFF;
 	}		
 
 	/*

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)
+	if (len >= 4 && cs[3] == CANID_DELIM) { /* 3 digits SFF CAN ID */
-		return 0;
 
-	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++;