Fix a few sign/verify issues

Fix use after free on sigtool verifydiff cleanup. Make it so downloadFile doesn't throw a warning if the server doesn't have the .sign file. Make it so clamav engine initialization checks the CVD_CERTS_PATH environment variable
Cisco-Talos · Dec 20, 2024 · 02c62b5 · 02c62b5
1 parent 019c862
commit 02c62b5
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 61 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/clamd/clamd.c b/clamd/clamd.c
@@ -592,29 +592,23 @@ int main(int argc, char **argv)
         }
 
         cvdcertsdir = optget(opts, "cvdcertsdir")->strarg;
-        if (NULL == cvdcertsdir) {
-            // Check if the CVD_CERTS_DIR environment variable is set
-            cvdcertsdir = getenv("CVD_CERTS_DIR");
-
-            // If not, use the default value
-            if (NULL == cvdcertsdir) {
-                cvdcertsdir = CERTSDIR;
+        if (NULL != cvdcertsdir) {
+            // Config option must override the engine defaults
+            // (which would've used the env var or hardcoded path)
+            if (LSTAT(cvdcertsdir, &statbuf) == -1) {
+                logg(LOGG_ERROR,
+                    "ClamAV CA certificates directory is missing: %s\n"
+                    "It should have been provided as a part of installation.",
+                    cvdcertsdir);
+                ret = 1;
+                break;
             }
-        }
 
-        if (LSTAT(cvdcertsdir, &statbuf) == -1) {
-            logg(LOGG_ERROR,
-                "ClamAV CA certificates directory is missing: %s\n"
-                "It should have been provided as a part of installation.",
-                cvdcertsdir);
-            ret = 1;
-            break;
-        }
-
-        if ((ret = cl_engine_set_str(engine, CL_ENGINE_CVDCERTSDIR, cvdcertsdir))) {
-            logg(LOGG_ERROR, "cli_engine_set_str(CL_ENGINE_CVDCERTSDIR) failed: %s\n", cl_strerror(ret));
-            ret = 1;
-            break;
+            if ((ret = cl_engine_set_str(engine, CL_ENGINE_CVDCERTSDIR, cvdcertsdir))) {
+                logg(LOGG_ERROR, "cli_engine_set_str(CL_ENGINE_CVDCERTSDIR) failed: %s\n", cl_strerror(ret));
+                ret = 1;
+                break;
+            }
         }
 
         cl_engine_set_clcb_hash(engine, hash_callback);

diff --git a/clamscan/manager.c b/clamscan/manager.c
@@ -1253,31 +1253,23 @@ int scanmanager(const struct optstruct *opts)
     }
 
     cvdcertsdir = optget(opts, "cvdcertsdir")->strarg;
-    if (NULL == cvdcertsdir) {
-        // Check if the CVD_CERTS_DIR environment variable is set
-        cvdcertsdir = getenv("CVD_CERTS_DIR");
-
-        // If not, use the default value
-        if (NULL == cvdcertsdir) {
-            cvdcertsdir = CERTSDIR;
+    if (NULL != cvdcertsdir) {
+        // Command line option must override the engine defaults
+        // (which would've used the env var or hardcoded path)
+        if (LSTAT(cvdcertsdir, &statbuf) == -1) {
+            logg(LOGG_ERROR,
+                "ClamAV CA certificates directory is missing: %s\n"
+                "It should have been provided as a part of installation.",
+                cvdcertsdir);
+            ret = 2;
+            goto done;
         }
-    }
 
-    if (LSTAT(cvdcertsdir, &statbuf) == -1) {
-        logg(LOGG_ERROR,
-             "ClamAV CA certificates directory is missing: %s\n"
-             "It should have been provided as a part of installation.",
-             cvdcertsdir);
-
-        ret = 2;
-        goto done;
-    }
-
-    if ((ret = cl_engine_set_str(engine, CL_ENGINE_CVDCERTSDIR, cvdcertsdir))) {
-        logg(LOGG_ERROR, "cli_engine_set_str(CL_ENGINE_CVDCERTSDIR) failed: %s\n", cl_strerror(ret));
-
-        ret = 2;
-        goto done;
+        if ((ret = cl_engine_set_str(engine, CL_ENGINE_CVDCERTSDIR, cvdcertsdir))) {
+            logg(LOGG_ERROR, "cli_engine_set_str(CL_ENGINE_CVDCERTSDIR) failed: %s\n", cl_strerror(ret));
+            ret = 2;
+            goto done;
+        }
     }
 
     if ((opt = optget(opts, "database"))->active) {

diff --git a/libclamav/others.c b/libclamav/others.c
@@ -448,6 +448,7 @@ struct cl_engine *cl_engine_new(void)
 {
     struct cl_engine *new;
     cli_intel_t *intel;
+    char *cvdcertsdir = NULL;
 
     new = (struct cl_engine *)calloc(1, sizeof(struct cl_engine));
     if (!new) {
@@ -599,7 +600,13 @@ struct cl_engine *cl_engine_new(void)
 
 #endif
 
-    new->certs_directory = CLI_MPOOL_STRDUP(new->mempool, CERTSDIR);
+    // Check if the CVD_CERTS_DIR environment variable is set
+    cvdcertsdir = getenv("CVD_CERTS_DIR");
+    if (NULL != cvdcertsdir) {
+        new->certs_directory = CLI_MPOOL_STRDUP(new->mempool, cvdcertsdir);
+    } else {
+        new->certs_directory = CLI_MPOOL_STRDUP(new->mempool, CERTSDIR);
+    }
 
     cli_dbgmsg("Initialized %s engine\n", cl_retver());
     return new;

diff --git a/libfreshclam/libfreshclam_internal.c b/libfreshclam/libfreshclam_internal.c
@@ -1166,11 +1166,23 @@ static fc_error_t remote_cvdhead(
     return status;
 }
 
+/**
+ * @brief Download a file from a remote server.
+ *
+ * @param url               URL of file to download.
+ * @param destfile          Local file to save downloaded file to.
+ * @param bAllowRedirect    Allow redirects.
+ * @param logerr            Log a failure as an error instead of a warning.
+ * @param quiet             Don't warn if we get a 404. Just a debug message.
+ * @param ifModifiedSince   If-Modified-Since time to use in request.
+ * @return fc_error_t       FC_SUCCESS if download successful.
+ */
 static fc_error_t downloadFile(
     const char *url,
     const char *destfile,
     int bAllowRedirect,
     int logerr,
+    int quiet,
     time_t ifModifiedSince)
 {
     fc_error_t ret;
@@ -1398,9 +1410,9 @@ static fc_error_t downloadFile(
         }
         case 404: {
             if (g_proxyServer)
-                logg(LOGG_WARNING, "downloadFile: file not found: %s (Proxy: %s:%u)\n", url, g_proxyServer, g_proxyPort);
+                logg(quiet ? LOGG_DEBUG : LOGG_WARNING, "downloadFile: file not found: %s (Proxy: %s:%u)\n", url, g_proxyServer, g_proxyPort);
             else
-                logg(LOGG_WARNING, "downloadFile: file not found: %s\n", url);
+                logg(quiet ? LOGG_DEBUG : LOGG_WARNING, "downloadFile: file not found: %s\n", url);
             status = FC_EFAILEDGET;
             break;
         }
@@ -1483,7 +1495,7 @@ static fc_error_t getcvd(
     url    = malloc(urlLen + 1);
     snprintf(url, urlLen + 1, "%s/%s", server, cvdfile);
 
-    ret = downloadFile(url, tmpfile, 1, logerr, ifModifiedSince);
+    ret = downloadFile(url, tmpfile, 1, logerr, 0, ifModifiedSince);
     if (ret == FC_UPTODATE) {
         logg(LOGG_INFO, "%s is up-to-date.\n", cvdfile);
         status = ret;
@@ -1520,7 +1532,7 @@ static fc_error_t getcvd(
     tmpsignfile    = malloc(tmpsignfileLen + 1);
     snprintf(tmpsignfile, tmpsignfileLen + 1, "%s" PATHSEP "%s", g_tempDirectory, sign_filename);
 
-    ret = downloadFile(sign_file_url, tmpsignfile, 1, logerr, 0);
+    ret = downloadFile(sign_file_url, tmpsignfile, 1, logerr, 1, 0);
     if (ret != FC_SUCCESS) {
         logg(LOGG_DEBUG, "No external .sign digital signature file for %s-%u\n", database, cvd->version);
         // It's not an error if the .sign file doesn't exist.
@@ -1723,7 +1735,7 @@ static fc_error_t downloadPatchAndApply(
     url    = malloc(urlLen + 1);
     snprintf(url, urlLen + 1, "%s/%s", server, patch);
 
-    if (FC_SUCCESS != (ret = downloadFile(url, patch, 1, logerr, 0))) {
+    if (FC_SUCCESS != (ret = downloadFile(url, patch, 1, logerr, 0, 0))) {
         if (ret == FC_EEMPTYFILE) {
             logg(LOGG_INFO, "Empty script %s, need to download entire database\n", patch);
         } else {
@@ -1742,7 +1754,7 @@ static fc_error_t downloadPatchAndApply(
     sign_url    = malloc(sign_urlLen + 1);
     snprintf(sign_url, sign_urlLen + 1, "%s/%s", server, patch_sign_file);
 
-    if (FC_SUCCESS != (ret = downloadFile(sign_url, patch_sign_file, 1, logerr, 0))) {
+    if (FC_SUCCESS != (ret = downloadFile(sign_url, patch_sign_file, 1, logerr, 1, 0))) {
         // No sign file is not an error.
         // Just means we'll have to fall back to the legacy sha256-based rsa method for verifying CDIFFs.
         logg(LOGG_DEBUG, "No external .sign digital signature file for %s\n", patch);
@@ -2779,7 +2791,7 @@ fc_error_t updatecustomdb(
 
         dbtime = (CLAMSTAT(databaseName, &statbuf) != -1) ? statbuf.st_mtime : 0;
 
-        ret = downloadFile(url, tmpfile, 1, logerr, dbtime);
+        ret = downloadFile(url, tmpfile, 1, logerr, 0, dbtime);
         if (ret == FC_UPTODATE) {
             logg(LOGG_INFO, "%s is up-to-date (version: custom database)\n", databaseName);
             goto up_to_date;

diff --git a/sigtool/sigtool.c b/sigtool/sigtool.c
@@ -2629,15 +2629,15 @@ static int verifydiff(const struct optstruct *opts, const char *diff, const char
     }
 
 done:
+    if (created_temp_dir) {
+        removeTempDir(opts, tempdir);
+    }
     if (NULL != tempdir) {
         free(tempdir);
     }
     if (NULL != cdiff_apply_error) {
         ffierror_free(cdiff_apply_error);
     }
-    if (created_temp_dir) {
-        removeTempDir(opts, tempdir);
-    }
 
     return ret;
 }

diff --git a/unit_tests/check_clamav.c b/unit_tests/check_clamav.c
@@ -424,12 +424,6 @@ START_TEST(test_cl_load)
     engine = cl_engine_new();
     ck_assert_msg(engine != NULL, "cl_engine_new failed");
 
-    cvdcertsdir = getenv("CVD_CERTS_DIR");
-    ck_assert_msg(cvdcertsdir != NULL, "CVD_CERTS_DIR not set");
-
-    ret = cl_engine_set_str(engine, CL_ENGINE_CVDCERTSDIR, cvdcertsdir);
-    ck_assert_msg(ret == CL_SUCCESS, "cl_engine_set_str failed: %s", cl_strerror(ret));
-
     /* load test cvd */
     testfile = SRCDIR PATHSEP "input" PATHSEP "freshclam_testfiles" PATHSEP "test-5.cvd";
     ret      = cl_load(testfile, engine, &sigs, CL_DB_STDOPT);