Browse code

clamdscan: Fix --fdpass -m & ExcludePath crash

If you set an ExcludePath regex in clamd.conf and then perform a
ClamDScan scan with --fdpass --multiscan, it will segfault.
The same issue also affects --fdpass --multiscan scans when using
ExcludePath when scanning a patch that doesn't exist.

The issue is that the filepath isn't being passed along for the path
exclusion regex match, resulting in a NULL deref.

This commit also fixes a possible memory leak if by duplicating the path
for the handle_entry() call _after_ the callback() runs, in case ret
isn't CL_SUCCESS and the function exits without every using the entry
structure or free'ing the copied filename.

The above work temporarily caused a test failure in check_clamd and a
valgrind failure in clamd for the nonexistent file test due to a minor
memory leak. This made it apparent that there were a few other nearby
possible memory leaks.

This commit fixes the above plus cleans up the error handling in clamd's
the file tree walk functions.

Micah Snyder (micasnyd) authored on 2021/03/23 08:52:14
Showing 4 changed files
... ...
@@ -146,8 +146,8 @@ cl_error_t scan_callback(STATBUF *sb, char *filename, const char *msg, enum cli_
146 146
 
147 147
     if (NULL != filename) {
148 148
         if (CL_SUCCESS != cli_realpath((const char *)filename, &real_filename)) {
149
-            conn_reply_errno(scandata->conn, msg, "Failed to determine real path:");
150
-            logg("^Failed to determine real path for: %s\n", filename);
149
+            conn_reply_errno(scandata->conn, msg, "File path check failure:");
150
+            logg("^File path check failure for: %s\n", filename);
151 151
             logg("*Quarantine of the file may fail if file path contains symlinks.\n");
152 152
         } else {
153 153
             free(filename);
... ...
@@ -180,25 +180,30 @@ cl_error_t scan_callback(STATBUF *sb, char *filename, const char *msg, enum cli_
180 180
             else
181 181
                 logg("!Memory allocation failed during cli_ftw()\n");
182 182
             scandata->errors++;
183
+            free(filename);
183 184
             return CL_EMEM;
184 185
         case error_stat:
185
-            conn_reply_errno(scandata->conn, msg, "lstat() failed:");
186
-            logg("^lstat() failed on: %s\n", msg);
186
+            conn_reply_errno(scandata->conn, msg, "File path check failure:");
187
+            logg("^File path check failure on: %s\n", msg);
187 188
             scandata->errors++;
189
+            free(filename);
188 190
             return CL_SUCCESS;
189 191
         case warning_skipped_dir:
190
-            logg("^Directory recursion limit reached, skipping %s\n",
191
-                 msg);
192
+            logg("^Directory recursion limit reached, skipping %s\n", msg);
193
+            free(filename);
192 194
             return CL_SUCCESS;
193 195
         case warning_skipped_link:
194 196
             logg("$Skipping symlink: %s\n", msg);
197
+            free(filename);
195 198
             return CL_SUCCESS;
196 199
         case warning_skipped_special:
197 200
             if (msg == scandata->toplevel_path)
198 201
                 conn_reply(scandata->conn, msg, "Not supported file type", "ERROR");
199 202
             logg("*Not supported file type: %s\n", msg);
203
+            free(filename);
200 204
             return CL_SUCCESS;
201 205
         case visit_directory_toplev:
206
+            free(filename);
202 207
             return CL_SUCCESS;
203 208
         case visit_file:
204 209
             break;
... ...
@@ -951,7 +951,7 @@ typedef int (*cli_ftw_pathchk)(const char *path, struct cli_ftw_cbdata *data);
951 951
  * which one it is.
952 952
  * If it is a file, it simply calls the callback once, otherwise recurses.
953 953
  */
954
-int cli_ftw(char *base, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk);
954
+cl_error_t cli_ftw(char *base, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk);
955 955
 
956 956
 const char *cli_strerror(int errnum, char *buf, size_t len);
957 957
 
... ...
@@ -574,28 +574,46 @@ static int get_filetype(const char *fname, int flags, int need_stat,
574 574
     return stated;
575 575
 }
576 576
 
577
-static int handle_filetype(const char *fname, int flags,
578
-                           STATBUF *statbuf, int *stated, enum filetype *ft,
579
-                           cli_ftw_cb callback, struct cli_ftw_cbdata *data)
577
+static cl_error_t handle_filetype(const char *fname, int flags,
578
+                                  STATBUF *statbuf, int *stated, enum filetype *ft,
579
+                                  cli_ftw_cb callback, struct cli_ftw_cbdata *data)
580 580
 {
581
-    int ret;
581
+    cl_error_t status = CL_EMEM;
582 582
 
583 583
     *stated = get_filetype(fname, flags, flags & CLI_FTW_NEED_STAT, statbuf, ft);
584 584
 
585 585
     if (*stated == -1) {
586
-        /*  we failed a stat() or lstat() */
587
-        ret = callback(NULL, NULL, fname, error_stat, data);
588
-        if (ret != CL_SUCCESS)
589
-            return ret;
586
+        /* we failed a stat() or lstat() */
587
+        char *fname_copy = cli_strdup(fname);
588
+        if (NULL == fname_copy) {
589
+            goto done;
590
+        }
591
+
592
+        status = callback(NULL, fname_copy, fname, error_stat, data);
593
+        if (status != CL_SUCCESS) {
594
+            goto done;
595
+        }
590 596
         *ft = ft_unknown;
591 597
     } else if (*ft == ft_skipped_link || *ft == ft_skipped_special) {
592 598
         /* skipped filetype */
593
-        ret = callback(stated ? statbuf : NULL, NULL, fname,
594
-                       *ft == ft_skipped_link ? warning_skipped_link : warning_skipped_special, data);
595
-        if (ret != CL_SUCCESS)
596
-            return ret;
599
+        char *fname_copy = cli_strdup(fname);
600
+        if (NULL == fname_copy) {
601
+            goto done;
602
+        }
603
+
604
+        status = callback(stated ? statbuf : NULL,
605
+                          fname_copy,
606
+                          fname,
607
+                          *ft == ft_skipped_link ? warning_skipped_link : warning_skipped_special,
608
+                          data);
609
+        if (status != CL_SUCCESS)
610
+            goto done;
597 611
     }
598
-    return CL_SUCCESS;
612
+
613
+    status = CL_SUCCESS;
614
+
615
+done:
616
+    return status;
599 617
 }
600 618
 
601 619
 static int cli_ftw_dir(const char *dirname, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk);
... ...
@@ -608,13 +626,14 @@ static int handle_entry(struct dirent_data *entry, int flags, int maxdepth, cli_
608 608
     }
609 609
 }
610 610
 
611
-int cli_ftw(char *path, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk)
611
+cl_error_t cli_ftw(char *path, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk)
612 612
 {
613
+    cl_error_t status = CL_EMEM;
613 614
     STATBUF statbuf;
614 615
     enum filetype ft = ft_unknown;
615 616
     struct dirent_data entry;
616
-    int stated = 0;
617
-    int ret;
617
+    int stated      = 0;
618
+    char *path_copy = NULL;
618 619
 
619 620
     if (((flags & CLI_FTW_TRIM_SLASHES) || pathchk) && path[0] && path[1]) {
620 621
         char *pathend;
... ...
@@ -627,23 +646,49 @@ int cli_ftw(char *path, int flags, int maxdepth, cli_ftw_cb callback, struct cli
627 627
         while (pathend > path && pathend[-1] == *PATHSEP) --pathend;
628 628
         *pathend = '\0';
629 629
     }
630
-    if (pathchk && pathchk(path, data) == 1)
631
-        return CL_SUCCESS;
632
-    ret = handle_filetype(path, flags, &statbuf, &stated, &ft, callback, data);
633
-    if (ret != CL_SUCCESS)
634
-        return ret;
635
-    if (ft_skipped(ft))
636
-        return CL_SUCCESS;
637
-    entry.statbuf  = stated ? &statbuf : NULL;
638
-    entry.is_dir   = ft == ft_directory;
639
-    entry.filename = entry.is_dir ? NULL : strdup(path);
640
-    entry.dirname  = entry.is_dir ? path : NULL;
630
+
631
+    if (pathchk && pathchk(path, data) == 1) {
632
+        status = CL_SUCCESS;
633
+        goto done;
634
+    }
635
+
636
+    status = handle_filetype(path, flags, &statbuf, &stated, &ft, callback, data);
637
+    if (status != CL_SUCCESS) {
638
+        goto done;
639
+    }
640
+
641
+    if (ft_skipped(ft)) {
642
+        status = CL_SUCCESS;
643
+        goto done;
644
+    }
645
+
646
+    entry.statbuf = stated ? &statbuf : NULL;
647
+    entry.is_dir  = ft == ft_directory;
648
+
641 649
     if (entry.is_dir) {
642
-        ret = callback(entry.statbuf, NULL, path, visit_directory_toplev, data);
643
-        if (ret != CL_SUCCESS)
644
-            return ret;
650
+        path_copy = cli_strdup(path);
651
+        if (NULL == path_copy) {
652
+            goto done;
653
+        }
654
+
655
+        status = callback(entry.statbuf, path_copy, path, visit_directory_toplev, data);
656
+        if (status != CL_SUCCESS) {
657
+            goto done;
658
+        }
659
+    }
660
+
661
+    path_copy = cli_strdup(path);
662
+    if (NULL == path_copy) {
663
+        goto done;
645 664
     }
646
-    return handle_entry(&entry, flags, maxdepth, callback, data, pathchk);
665
+
666
+    entry.filename = entry.is_dir ? NULL : path_copy;
667
+    entry.dirname  = entry.is_dir ? path : NULL;
668
+
669
+    status = handle_entry(&entry, flags, maxdepth, callback, data, pathchk);
670
+
671
+done:
672
+    return status;
647 673
 }
648 674
 
649 675
 static int cli_ftw_dir(const char *dirname, int flags, int maxdepth, cli_ftw_cb callback, struct cli_ftw_cbdata *data, cli_ftw_pathchk pathchk)
... ...
@@ -121,7 +121,7 @@ static void conn_teardown(void)
121 121
 
122 122
 #define NONEXISTENT "/nonexistent\vfilename"
123 123
 
124
-#define NONEXISTENT_REPLY NONEXISTENT ": lstat() failed: No such file or directory. ERROR"
124
+#define NONEXISTENT_REPLY NONEXISTENT ": File path check failure: No such file or directory. ERROR"
125 125
 
126 126
 #define ACCDENIED BUILDDIR "/accdenied"
127 127
 #define ACCDENIED_REPLY ACCDENIED ": Access denied. ERROR"