Missing Sanity Checks/Null Pointer Dereference (CWE-476)/Memory Leaks in rootcheck

[dogbert2]

Hello All,

In reviewing calls to malloc() in the latest version of rootcheck, I found

numerous instances where return values for malloc are not checked for a value
of NULL, indicating failure. Additionally, calls to memset() and strncpy()
are made immediately afterwards, but if the return value from malloc() is
NULL, these calls will result in a segmentation violation/fault.

Additionally, previously allocated memory is also not released in the
event that malloc() fails.

The three patch files below should address/correct these issues:

--- GeoIP.c.orig    2016-02-26 12:39:26.334682615 -0800
+++ GeoIP.c 2016-02-26 12:43:29.717615334 -0800
@@ -775,6 +775,8 @@
 {
     if (NULL == GeoIPDBFileName) {
         GeoIPDBFileName = malloc(sizeof(char *) * NUM_DB_TYPES);
+   if (GeoIPDBFileName == NULL)
+       return;
         memset(GeoIPDBFileName, 0, sizeof(char *) * NUM_DB_TYPES);

         GeoIPDBFileName[GEOIP_COUNTRY_EDITION] = _GeoIP_full_path_to(
@@ -2387,6 +2389,9 @@
         } else {
             len = sizeof(char) * (strlen(buf) + 1);
             org_buf = malloc(len);
+       if (org_buf == NULL) {
+       return NULL;
+       }
             strncpy(org_buf, buf, len);
         }
     } else {
@@ -2396,6 +2401,9 @@
         } else {
             len = sizeof(char) * (strlen(buf_pointer) + 1);
             org_buf = malloc(len);
+       if (org_buf == NULL) {
+       return NULL;
+       }
             strncpy(org_buf, buf_pointer, len);
         }
     }
@@ -2447,6 +2455,9 @@
         } else {
             len = sizeof(char) * (strlen(buf) + 1);
             org_buf = malloc(len);
+       if (org_buf == NULL) {
+       return NULL;
+       }
             strncpy(org_buf, buf, len);
         }
     } else {
@@ -2456,6 +2467,9 @@
         } else {
             len = sizeof(char) * (strlen(buf_pointer) + 1);
             org_buf = malloc(len);
+       if (org_buf == NULL) {
+       return NULL;
+       }
             strncpy(org_buf, buf_pointer, len);
         }
     }
@@ -2470,6 +2484,9 @@
     int num_chars_written, i;

     ret_str = malloc(sizeof(char) * 16);
+    if (ret_str == NULL) {
+   return NULL;
+    }
     cur_str = ret_str;

     for (i = 0; i < 4; i++) {
@@ -2506,6 +2523,9 @@
     }

     ret = malloc(sizeof(char *) * 2);
+    if (ret == NULL) {
+   return NULL;
+    }

     ipnum = GeoIP_addr_to_num(addr);
     target_value = _GeoIP_seek_record_gl(gi, ipnum, gl);

=======================================================================

--- GeoIPCity.c.orig    2016-02-26 12:32:11.939473218 -0800
+++ GeoIPCity.c 2016-02-26 12:38:16.486805309 -0800
@@ -77,6 +77,9 @@
     }

     record = malloc(sizeof(GeoIPRecord));
+    if (record == NULL) {
+   return NULL;
+    }
     memset(record, 0, sizeof(GeoIPRecord));
     record->charset = gi->charset;

@@ -86,6 +89,10 @@
     if (gi->cache == NULL) {
         begin_record_buf = record_buf = malloc(
                                sizeof(unsigned char) * FULL_RECORD_LENGTH);
+   if (record_buf == NULL) {
+       free(record);
+       return NULL;
+   }
         bytes_read = pread(fileno(
                                gi->GeoIPDatabase), record_buf,
                            FULL_RECORD_LENGTH, record_pointer);
@@ -117,6 +124,11 @@
     }
     if (str_length > 0) {
         record->region = malloc(str_length + 1);
+   if (record->region == NULL) {
+       free(record_buf);
+       free(record);
+       return NULL;
+   }
         strncpy(record->region, (char *)record_buf, str_length + 1);
     }
     record_buf += str_length + 1;
@@ -131,6 +143,12 @@
             record->city = _GeoIP_iso_8859_1__utf8((const char *)record_buf);
         }else {
             record->city = malloc(str_length + 1);
+       if (record->city == NULL) {
+       free(record->region);
+       free(record->buf);
+       free(record);
+       return NULL;
+       }
             strncpy(record->city, (const char *)record_buf, str_length + 1);
         }
     }
@@ -143,6 +161,13 @@
     }
     if (str_length > 0) {
         record->postal_code = malloc(str_length + 1);
+   if (record->postal_code == NULL) {
+       free(record->city);
+       free(record->region);
+       free(record->buf);
+       free(record);
+       return NULL;
+   }
         strncpy(record->postal_code, (char *)record_buf, str_length + 1);
     }
     record_buf += (str_length + 1);


=======================================================================

--- lists_list.c.orig   2016-02-26 12:29:22.196899810 -0800
+++ lists_list.c    2016-02-26 12:30:14.805586103 -0800
@@ -209,6 +209,8 @@
             vpos = cdb_datapos(&lrule->db->cdb);
             vlen = cdb_datalen(&lrule->db->cdb);
             val = malloc(vlen);
+       if (val == NULL)
+       return 0;
             cdb_read(&lrule->db->cdb, val, vlen, vpos);
             result = OSMatch_Execute(val, vlen, lrule->matcher);
             free(val);

=======================================================================

[dcid]

That's a hard one to review from here. Mind sending a PR (pull request) instead?

thanks,

[ddpbsd]

Was a PR ever submitted for this? @dcid Will those issues possibly go away with the integration of your geoip work?

[dogbert2]

Beats me, that issue has been sitting there a LONG time, I would have
expected some changes/commits to be made.

Bill

On Fri, Apr 1, 2016 at 7:50 AM, Dan Parriott notifications@github.com
wrote:

Was a PR ever submitted for this? @dcid https://github.com/dcid Will
those issues possibly go away with the integration of your geoip work?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#752 (comment)

[ddpbsd]

Ok. I can't even find some of these files in our tree. I'm closing the issue, we'll have to take a look at the malloc calls later (I found the one in lists_list.c, will submit a PR).