From: Sebastien GODARD <sysstat@users.noreply.github.com>
Date: Sat, 6 Feb 2021 08:02:27 +0000 (+0100)
Subject: Check untrusted values before use
X-Git-Tag: v12.5.3~7
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f9bc451cfb775d975063b889407e3074cfa817fc;p=sysstat

Check untrusted values before use

Values read from a daily data file need to be checked before being used.
Fix Coverity CID #29717 #366268 #366269

Signed-off-by: Sebastien GODARD <sysstat@users.noreply.github.com>
---

diff --git a/sa.h b/sa.h
index 7bd7472..6107971 100644
--- a/sa.h
+++ b/sa.h
@@ -681,6 +681,8 @@ struct file_activity {
 #define FILE_ACTIVITY_UL_NR	0	/* Nr of unsigned long in file_activity structure */
 #define FILE_ACTIVITY_U_NR	9	/* Nr of [unsigned] int in file_activity structure */
 
+#define MAX_ITEM_STRUCT_SIZE	1024	/* Used for sanity check */
+
 /*
  * Description of an extra structure.
  * The composition of this structure should not change in time.
@@ -764,7 +766,7 @@ struct record_header {
 	 */
 	unsigned char record_type;
 	/*
-	 * Timestamp: Hour (0-23), minute (0-59) and second (0-59).
+	 * Timestamp: Hour (0-23), minute (0-59) and second (0-60).
 	 * Used to determine TRUE time.
 	 * Hour value depends in fact on timezone (TZ variable) value.
 	 */
diff --git a/sa_common.c b/sa_common.c
index c1a8d15..8d1eb17 100644
--- a/sa_common.c
+++ b/sa_common.c
@@ -1493,6 +1493,17 @@ int read_record_hdr(int ifd, void *buffer, struct record_header *record_hdr,
 			       record_hdr->hour, record_hdr->minute, record_hdr->second);
 		}
 
+		/* Sanity checks */
+		if ((record_hdr->record_type <= 0) || (record_hdr->record_type > R_EXTRA_MAX) ||
+		    (record_hdr->hour > 23) || (record_hdr->minute > 59) || (record_hdr->second > 60)) {
+#ifdef DEBUG
+			fprintf(stderr, "%s: record_type=%d HH:MM:SS=%02d:%02d:%02d\n",
+				__FUNCTION__, record_hdr->record_type,
+				record_hdr->hour, record_hdr->minute, record_hdr->second);
+#endif
+			return 2;
+		}
+
 		/*
 		 * Skip unknown extra structures if present.
 		 * This will be done later for R_COMMENT and R_RESTART records, as extra structures
@@ -1974,7 +1985,8 @@ void check_file_actlst(int *ifd, char *dfile, struct activity *act[], uint64_t f
 
 		/*
 		 * Every activity, known or unknown, should have
-		 * at least one item and sub-item, and a positive size value.
+		 * at least one item and sub-item, and a size value in
+		 * a defined range.
 		 * Also check that the number of items and sub-items
 		 * doesn't exceed a max value. This is necessary
 		 * because we will use @nr and @nr2 to
@@ -1985,10 +1997,10 @@ void check_file_actlst(int *ifd, char *dfile, struct activity *act[], uint64_t f
 		 */
 		if ((fal->nr < 1) || (fal->nr2 < 1) ||
 		    (fal->nr > NR_MAX) || (fal->nr2 > NR2_MAX) ||
-		    (fal->size <= 0)) {
+		    (fal->size <= 0) || (fal->size > MAX_ITEM_STRUCT_SIZE)) {
 #ifdef DEBUG
-			fprintf(stderr, "%s: id=%d nr=%d nr2=%d\n",
-				__FUNCTION__, fal->id, fal->nr, fal->nr2);
+			fprintf(stderr, "%s: id=%d nr=%d nr2=%d size=%d\n",
+				__FUNCTION__, fal->id, fal->nr, fal->nr2, fal->size);
 #endif
 			goto format_error;
 		}
diff --git a/sa_conv.c b/sa_conv.c
index db1fade..5575bd1 100644
--- a/sa_conv.c
+++ b/sa_conv.c
@@ -338,8 +338,8 @@ int upgrade_header_section(char dfile[], int fd, int stdfd, struct activity *act
 				a_cpu = TRUE;
 			}
 
-			/* Size of an activity cannot be zero */
-			if (!ofal->size)
+			/* Sanity checks */
+			if (!ofal->size || (ofal->size > MAX_ITEM_STRUCT_SIZE))
 				goto invalid_header;
 
 			/* Size of activity in file is larger than up-to-date activity size */