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 7bd747222915fae496ebc7d614d93be27e706964..610797186340478c030c8ad457cfda2138d73b5f 100644 (file)
--- 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 c1a8d154a0a86f15ac8d8fa6544eb2608a92ce39..8d1eb17b0294fe9a5a9c4b5a9e15693912d409cb 100644 (file)
--- 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 db1fade01920f6ff995c11e39f42408f2202d729..5575bd1f42736aa149c5d252ed417d8629bc6b54 100644 (file)
--- 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 */