From 9f697ce6a6e27b951e2353bd1835f6f3b35ca3ed Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Mon, 10 Apr 2023 20:26:23 -0700 Subject: [PATCH 1/7] Freshclam: Download whole CVD if failure applying CDIFF In the event that there is an issue with the CDIFF process, freshclam is treating it as thought no patch was downloaded. If freshclam fails to apply the patch because of an issue with the patch, or some bug in the CDIFF module, it should retry for the whole CVD. --- libfreshclam/libfreshclam_internal.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libfreshclam/libfreshclam_internal.c b/libfreshclam/libfreshclam_internal.c index 55b7c02c6e..2f7663643f 100644 --- a/libfreshclam/libfreshclam_internal.c +++ b/libfreshclam/libfreshclam_internal.c @@ -2343,6 +2343,7 @@ fc_error_t updatedb( if ( (FC_EEMPTYFILE == ret) || /* Request a new CVD if we got an empty CDIFF. */ + (FC_EFAILEDUPDATE == ret) || /* Request a new CVD if we failed to apply a CDIFF. */ (FC_SUCCESS != ret && ( /* Or if the incremental update failed: */ (0 == numPatchesReceived) && /* 1. Ask for the CVD if we didn't get any patches, */ (localVersion < remoteVersion - 1) /* 2. AND if we're more than 1 version out of date. */ From c294959e80d8da1ed60dd20e44d3d261c99ffc1b Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Wed, 12 Apr 2023 22:42:31 -0700 Subject: [PATCH 2/7] Tests: Add sigtool tests for correct CDIFF UNLINK functionality Also includes: - A sigtool test to verify that Rust log macros are working. - Changing the freshclam tests to use --no-dns so they run faster when DNS isn't working (e.g. no internet). --- unit_tests/freshclam_test.py | 116 ++++++++++++++++++++++++++++++----- unit_tests/sigtool_test.py | 78 +++++++++++++++++++++++ 2 files changed, 179 insertions(+), 15 deletions(-) diff --git a/unit_tests/freshclam_test.py b/unit_tests/freshclam_test.py index 62c0a6daf0..ae5b9b8a24 100644 --- a/unit_tests/freshclam_test.py +++ b/unit_tests/freshclam_test.py @@ -87,7 +87,7 @@ def test_freshclam_00_version(self): port=TC.mock_mirror_port, )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} -V'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} -V'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -133,7 +133,7 @@ def test_freshclam_01_file_copy(self): user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config}'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config}'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -174,7 +174,7 @@ def test_freshclam_02_http_403(self): port=TC.mock_mirror_port, user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=daily'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=daily'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -189,7 +189,7 @@ def test_freshclam_02_http_403(self): # verify stderr self.verify_output(output.err, expected=expected_stderr) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=daily'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=daily'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -229,7 +229,7 @@ def test_freshclam_03_http_403_daemonized(self): port=TC.mock_mirror_port, user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=daily --daemon -F'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=daily --daemon -F'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -269,7 +269,7 @@ def test_freshclam_04_http_429(self): port=TC.mock_mirror_port, user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=daily'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=daily'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -322,7 +322,7 @@ def test_freshclam_05_cdiff_update(self): port=TC.mock_mirror_port, user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -384,7 +384,7 @@ def test_freshclam_05_cdiff_update_UNC(self): port=TC.mock_mirror_port, user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -440,7 +440,7 @@ def test_freshclam_06_cdiff_partial_minus_1(self): port=TC.mock_mirror_port, user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -465,7 +465,7 @@ def test_freshclam_06_cdiff_partial_minus_1(self): # # Try again, we should be 1 behind which is tolerable and should not trigger a full CVD download # - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -526,7 +526,7 @@ def test_freshclam_07_cdiff_partial_minus_2(self): port=TC.mock_mirror_port, user=getpass.getuser(), )) - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -551,7 +551,7 @@ def test_freshclam_07_cdiff_partial_minus_2(self): # # Try again, we should be 2 behind which is NOT tolerable and SHOULD trigger a full CVD download # - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -617,7 +617,7 @@ def test_freshclam_07_no_cdiff_out_of_date_cvd(self): # # 1st attempt # - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -637,7 +637,7 @@ def test_freshclam_07_no_cdiff_out_of_date_cvd(self): # # 2nd attempt # - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -663,7 +663,7 @@ def test_freshclam_07_no_cdiff_out_of_date_cvd(self): # # 3rd attempt # - command = '{valgrind} {valgrind_args} {freshclam} --config-file={freshclam_config} --update-db=test'.format( + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config ) output = self.execute_command(command) @@ -685,6 +685,92 @@ def test_freshclam_07_no_cdiff_out_of_date_cvd(self): # verify stderr self.verify_output(output.err, unexpected=unexpected_results) + def test_freshclam_08_cdiff_update_twice(self): + self.step_name('Verify that freshclam can update from an older CVD to a newer with CDIFF patches, and then update from an older CLD to the latest with more CDIFF patches') + + # start with this CVD + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-1.cvd'), str(TC.path_db / 'test.cvd')) + + # advertise this CVD FIRST (by sending the header response to Range requests) + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-3.cvd'), str(TC.path_www / 'test.cvd.advertised')) + + # using these CDIFFs + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-2.cdiff'), str(TC.path_www)) + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-3.cdiff'), str(TC.path_www)) + + handler = partial(WebServerHandler_WWW, TC.path_www) + TC.mock_mirror = Process(target=mock_database_mirror, args=(handler, TC.mock_mirror_port)) + TC.mock_mirror.start() + + if TC.freshclam_config.exists(): + os.remove(str(TC.freshclam_config)) + + TC.freshclam_config.write_text(''' + DatabaseMirror http://localhost:{port} + DNSDatabaseInfo no + PidFile {freshclam_pid} + LogVerbose yes + LogFileMaxSize 0 + LogTime yes + DatabaseDirectory {path_db} + DatabaseOwner {user} + '''.format( + freshclam_pid=TC.freshclam_pid, + path_db=TC.path_db, + port=TC.mock_mirror_port, + user=getpass.getuser(), + )) + + # + # Now run the update for the first set up updates. + # + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( + valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config + ) + output = self.execute_command(command) + + assert output.ec == 0 # success + + expected_stdout = [ + 'test.cld updated', + ] + unexpected_results = [ + 'already up-to-date' + ] + + # verify stdout + self.verify_output(output.out, expected=expected_stdout, unexpected=unexpected_results) + + # verify stderr + self.verify_output(output.err, unexpected=unexpected_results) + + # advertise this newer CVD SECOND (by sending the header response to Range requests) + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-6.cvd'), str(TC.path_www / 'test.cvd.advertised')) + + # using these CDIFFs + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-4.cdiff'), str(TC.path_www)) + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-5.cdiff'), str(TC.path_www)) + shutil.copy(str(TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' /'test-6.cdiff'), str(TC.path_www)) + + # + # Now re-run the update for more updates. + # + command = '{valgrind} {valgrind_args} {freshclam} --no-dns --config-file={freshclam_config} --update-db=test'.format( + valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, freshclam=TC.freshclam, freshclam_config=TC.freshclam_config + ) + output = self.execute_command(command) + + assert output.ec == 0 # success + + expected_stdout = [ + 'test.cld updated', + ] + unexpected_results = [ + 'already up-to-date' + ] + + + def mock_database_mirror(handler, port=8001): ''' Process entry point for our HTTP Server to mock a database mirror. diff --git a/unit_tests/sigtool_test.py b/unit_tests/sigtool_test.py index 63a512537b..3da0aba362 100644 --- a/unit_tests/sigtool_test.py +++ b/unit_tests/sigtool_test.py @@ -58,11 +58,13 @@ def tearDownClass(cls): super(TC, cls).tearDownClass() def setUp(self): + TC.original_cwd = os.getcwd() super(TC, self).setUp() def tearDown(self): super(TC, self).tearDown() self.verify_valgrind_log() + os.chdir(TC.original_cwd) def test_sigtool_00_version(self): self.step_name('sigtool version test') @@ -79,3 +81,79 @@ def test_sigtool_00_version(self): 'ClamAV {}'.format(TC.version), ] self.verify_output(output.out, expected=expected_results) + + def test_sigtool_01_run_cdiff(self): + self.step_name('sigtool run cdiff test') + # In addition to testing that running a cdiff works, this also tests a regression. + # Applying test-3.cdiff was failing because UNLINK wasn't properly implemented (since 0.105.0). + # We didn't notice it because logging wasn't enabled, and leniency in our freshclam cdiff process + # allowed the test to pass without noticing the bug. + + self.log.warning('VG: {}'.format(os.getenv("VG"))) + + (TC.path_tmp / 'run_cdiff').mkdir() + os.chdir(TC.path_tmp / 'run_cdiff') + + command = '{valgrind} {valgrind_args} {sigtool} --unpack {cvd}'.format( + valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, sigtool=TC.sigtool, + cvd=TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' / 'test-1.cvd' + ) + output = self.execute_command(command) + + assert output.ec == 0 # success + + # Apply 1st cdiff. + + command = '{valgrind} {valgrind_args} {sigtool} --run-cdiff={cdiff}'.format( + valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, sigtool=TC.sigtool, + cdiff=TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' / 'test-2.cdiff' + ) + output = self.execute_command(command) + + # Apply 2nd cdiff. This one failed because the CLOSE operation should create a file + # if it didn't exist, and it was only appending but not creating. + + command = '{valgrind} {valgrind_args} {sigtool} --run-cdiff={cdiff}'.format( + valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, sigtool=TC.sigtool, + cdiff=TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' / 'test-3.cdiff' + ) + output = self.execute_command(command) + + assert output.ec == 0 # success + + def test_sigtool_02_rust_logs_messages_work(self): + self.step_name('sigtool test rust log macros work') + # In addition to testing that running a cdiff works, this also tests a regression. + # Applying test-3.cdiff was failing because UNLINK wasn't properly implemented (since 0.105.0). + # We didn't notice it because logging wasn't enabled, and leniency in our freshclam cdiff process + # allowed the test to pass without noticing the bug. + + self.log.warning('VG: {}'.format(os.getenv("VG"))) + + (TC.path_tmp / 'run_cdiff_log').mkdir() + os.chdir(TC.path_tmp / 'run_cdiff_log') + + command = '{valgrind} {valgrind_args} {sigtool} --unpack {cvd}'.format( + valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, sigtool=TC.sigtool, + cvd=TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' / 'test-1.cvd' + ) + output = self.execute_command(command) + + assert output.ec == 0 # success + + # Apply cdiffs in wrong order. Should fail and print a message. + + command = '{valgrind} {valgrind_args} {sigtool} --run-cdiff={cdiff}'.format( + valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, sigtool=TC.sigtool, + cdiff=TC.path_source / 'unit_tests' / 'input' / 'freshclam_testfiles' / 'test-3.cdiff' + ) + output = self.execute_command(command) + + assert output.ec == 1 # failure + + # Verify that the `error!()` message was printed to stderr. + # This didn't happen before when we didn't initialize the rust logging lib at the top of sigtool. + expected_results = [ + 'LibClamAV Error', + ] + self.verify_output(output.err, expected=expected_results) From dd7e4b565eb9ddf7246f7cf6eff2c2e5c469cf2f Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Wed, 12 Apr 2023 22:49:17 -0700 Subject: [PATCH 3/7] Fix freshclam and sigtool logging issues Some log statements using the old ^, !, and * logg-prefix where they were making use a ternary to determine the log level in the log statement. Also sigtool and freshclam weren't outputting error log messages using the Rust log macros e.g. `error!("...")`. --- libfreshclam/dns.c | 6 ++-- libfreshclam/libfreshclam.c | 7 +++++ libfreshclam/libfreshclam_internal.c | 43 ++++++++++++++-------------- sigtool/sigtool.c | 6 ++++ 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/libfreshclam/dns.c b/libfreshclam/dns.c index 9536dd3c62..8f63abe6ba 100644 --- a/libfreshclam/dns.c +++ b/libfreshclam/dns.c @@ -71,12 +71,12 @@ dnsquery(const char *domain, int qtype, unsigned int *ttl) if (qtype == T_TXT) qtype = T_ANY; if ((len = res_query(domain, C_IN, qtype, answer, PACKETSZ)) < 0) { - logg(LOGG_INFO, "%cCan't query %s\n", - (qtype == T_TXT || qtype == T_ANY) ? '^' : '*', domain); + logg((qtype == T_TXT || qtype == T_ANY) ? LOGG_WARNING : LOGG_DEBUG, "Can't query %s\n", + domain); return NULL; } #else - logg(LOGG_INFO, "%cCan't query %s\n", (qtype == T_TXT) ? '^' : '*', domain); + logg((qtype == T_TXT) ? LOGG_WARNING : LOGG_DEBUG, "Can't query %s\n", domain); return NULL; #endif } diff --git a/libfreshclam/libfreshclam.c b/libfreshclam/libfreshclam.c index 73a44fc9d9..5783f4764f 100644 --- a/libfreshclam/libfreshclam.c +++ b/libfreshclam/libfreshclam.c @@ -56,6 +56,7 @@ // libclamav #include "clamav.h" +#include "clamav_rust.h" #include "others.h" #include "regex_list.h" #include "str.h" @@ -132,6 +133,12 @@ fc_error_t fc_initialize(fc_config *fcConfig) return status; } + /* Rust logging initialization */ + if (!clrs_log_init()) { + cli_dbgmsg("Unexpected problem occurred while setting up rust logging... continuing without rust logging. \ + Please submit an issue to https://github.com/Cisco-Talos/clamav"); + } + /* Initilize libcurl */ curl_global_init(CURL_GLOBAL_ALL); diff --git a/libfreshclam/libfreshclam_internal.c b/libfreshclam/libfreshclam_internal.c index 2f7663643f..6b10c2621b 100644 --- a/libfreshclam/libfreshclam_internal.c +++ b/libfreshclam/libfreshclam_internal.c @@ -987,11 +987,11 @@ static fc_error_t remote_cvdhead( * show the more generic information from curl_easy_strerror instead. */ size_t len = strlen(errbuf); - logg(LOGG_INFO, "%cremote_cvdhead: Download failed (%d) ", logerr ? '!' : '^', curl_ret); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "remote_cvdhead: Download failed (%d) ", curl_ret); if (len) - logg(LOGG_INFO, "%c Message: %s%s", logerr ? '!' : '^', errbuf, ((errbuf[len - 1] != '\n') ? "\n" : "")); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, " Message: %s%s", errbuf, ((errbuf[len - 1] != '\n') ? "\n" : "")); else - logg(LOGG_INFO, "%c Message: %s\n", logerr ? '!' : '^', curl_easy_strerror(curl_ret)); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, " Message: %s\n", curl_easy_strerror(curl_ret)); status = FC_ECONNECTION; goto done; } @@ -1057,11 +1057,11 @@ static fc_error_t remote_cvdhead( } default: { if (g_proxyServer) - logg(LOGG_INFO, "%cremote_cvdhead: Unexpected response (%li) from %s (Proxy: %s:%u)\n", - logerr ? '!' : '^', http_code, server, g_proxyServer, g_proxyPort); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "remote_cvdhead: Unexpected response (%li) from %s (Proxy: %s:%u)\n", + http_code, server, g_proxyServer, g_proxyPort); else - logg(LOGG_INFO, "%cremote_cvdhead: Unexpected response (%li) from %s\n", - logerr ? '!' : '^', http_code, server); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "remote_cvdhead: Unexpected response (%li) from %s\n", + http_code, server); status = FC_EFAILEDGET; goto done; } @@ -1071,7 +1071,7 @@ static fc_error_t remote_cvdhead( * Identify start of CVD header in response body. */ if (receivedData.size < CVD_HEADER_SIZE) { - logg(LOGG_INFO, "%cremote_cvdhead: Malformed CVD header (too short)\n", logerr ? '!' : '^'); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "remote_cvdhead: Malformed CVD header (too short)\n"); status = FC_EFAILEDGET; goto done; } @@ -1087,7 +1087,7 @@ static fc_error_t remote_cvdhead( (receivedData.buffer && !*receivedData.buffer) || (receivedData.buffer && !isprint(receivedData.buffer[i]))) { - logg(LOGG_INFO, "%cremote_cvdhead: Malformed CVD header (bad chars)\n", logerr ? '!' : '^'); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "remote_cvdhead: Malformed CVD header (bad chars)\n"); status = FC_EFAILEDGET; goto done; } @@ -1098,7 +1098,7 @@ static fc_error_t remote_cvdhead( * Parse CVD info into CVD info struct. */ if (!(cvdhead = cl_cvdparse(head))) { - logg(LOGG_INFO, "%cremote_cvdhead: Malformed CVD header (can't parse)\n", logerr ? '!' : '^'); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "remote_cvdhead: Malformed CVD header (can't parse)\n"); status = FC_EFAILEDGET; goto done; } else { @@ -1289,17 +1289,18 @@ static fc_error_t downloadFile( * show the more generic information from curl_easy_strerror instead. */ size_t len = strlen(errbuf); - logg(LOGG_INFO, "%cDownload failed (%d) ", logerr ? '!' : '^', curl_ret); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "Download failed (%d) ", curl_ret); if (len) - logg(LOGG_INFO, "%c Message: %s%s", logerr ? '!' : '^', errbuf, ((errbuf[len - 1] != '\n') ? "\n" : "")); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, " Message: %s%s", errbuf, ((errbuf[len - 1] != '\n') ? "\n" : "")); else - logg(LOGG_INFO, "%c Message: %s\n", logerr ? '!' : '^', curl_easy_strerror(curl_ret)); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, " Message: %s\n", curl_easy_strerror(curl_ret)); status = FC_ECONNECTION; goto done; } /* Check HTTP code */ curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &http_code); + logg(LOGG_WARNING, " ******* RESULT %ld, SIZE: %zu ******* \n", http_code, receivedFile.size); switch (http_code) { case 200: case 206: { @@ -1363,11 +1364,11 @@ static fc_error_t downloadFile( } default: { if (g_proxyServer) - logg(LOGG_INFO, "%cdownloadFile: Unexpected response (%li) from %s (Proxy: %s:%u)\n", - logerr ? '!' : '^', http_code, url, g_proxyServer, g_proxyPort); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "downloadFile: Unexpected response (%li) from %s (Proxy: %s:%u)\n", + http_code, url, g_proxyServer, g_proxyPort); else - logg(LOGG_INFO, "%cdownloadFile: Unexpected response (%li) from %s\n", - logerr ? '!' : '^', http_code, url); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "downloadFile: Unexpected response (%li) from %s\n", + http_code, url); status = FC_EFAILEDGET; } } @@ -1426,7 +1427,7 @@ static fc_error_t getcvd( status = ret; goto done; } else if (ret > FC_UPTODATE) { - logg(LOGG_INFO, "%cCan't download %s from %s\n", logerr ? '!' : '^', cvdfile, url); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "Can't download %s from %s\n", cvdfile, url); status = ret; goto done; } @@ -1625,7 +1626,7 @@ static fc_error_t downloadPatch( if (ret == FC_EEMPTYFILE) { logg(LOGG_INFO, "Empty script %s, need to download entire database\n", patch); } else { - logg(LOGG_INFO, "%cdownloadPatch: Can't download %s from %s\n", logerr ? '!' : '^', patch, url); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "downloadPatch: Can't download %s from %s\n", patch, url); } status = ret; goto done; @@ -2389,7 +2390,7 @@ fc_error_t updatedb( size_t newLocalFilenameLen = 0; if (FC_SUCCESS != buildcld(tmpdir, database, tmpfile, g_bCompressLocalDatabase)) { logg(LOGG_ERROR, "updatedb: Incremental update failed. Failed to build CLD.\n"); - status = FC_EFAILEDUPDATE; + status = FC_EBADCVD; goto done; } @@ -2603,7 +2604,7 @@ fc_error_t updatecustomdb( logg(LOGG_INFO, "%s is up-to-date (version: custom database)\n", databaseName); goto up_to_date; } else if (ret > FC_UPTODATE) { - logg(LOGG_INFO, "%cCan't download %s from %s\n", logerr ? '!' : '^', databaseName, url); + logg(logerr ? LOGG_ERROR : LOGG_WARNING, "Can't download %s from %s\n", databaseName, url); status = ret; goto done; } diff --git a/sigtool/sigtool.c b/sigtool/sigtool.c index da73ecdac9..ee723d23c5 100644 --- a/sigtool/sigtool.c +++ b/sigtool/sigtool.c @@ -3457,6 +3457,12 @@ int main(int argc, char **argv) } ret = 1; + /* Rust logging initialization */ + if (!clrs_log_init()) { + cli_dbgmsg("Unexpected problem occurred while setting up rust logging... continuing without rust logging. \ + Please submit an issue to https://github.com/Cisco-Talos/clamav"); + } + opts = optparse(NULL, argc, argv, 1, OPT_SIGTOOL, 0, NULL); if (!opts) { mprintf(LOGG_ERROR, "Can't parse command line options\n"); From 78e49f5f582fbfecee0e2ebcf2969a4e598b3b4f Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Mon, 10 Apr 2023 18:25:43 -0700 Subject: [PATCH 4/7] Freshclam/Sigtool: Fix CDIFF Unlink operation Any cdiff or script using the UNLINK operation will fail to delete the file claiming "No DB open for action UNLINK". The UNLINK operation appears to be trying to delete a currently open database, when in fact it should ensure no database is open before deleting the local file given by the single "db_name" parameter. --- libclamav_rust/src/cdiff.rs | 46 +++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/libclamav_rust/src/cdiff.rs b/libclamav_rust/src/cdiff.rs index 57c0c12b59..3257476815 100644 --- a/libclamav_rust/src/cdiff.rs +++ b/libclamav_rust/src/cdiff.rs @@ -183,6 +183,9 @@ pub enum InputError { #[error("no final newline")] MissingNL, + + #[error("Database file is still open: {0}")] + DBStillOpen(String), } /// Errors encountered while processing @@ -286,6 +289,32 @@ impl<'a> DelOp<'a> { } } +#[derive(Debug)] +pub struct UnlinkOp<'a> { + db_name: &'a str, +} + +/// Method to parse the cdiff line describing an unlink operation +impl<'a> UnlinkOp<'a> { + pub fn new(data: &'a [u8]) -> Result { + let mut iter = data.split(|b| *b == b' '); + let db_name = str::from_utf8( + iter.next() + .ok_or(InputError::MissingParameter("UNLINK", "db_name"))?, + ) + .map_err(|_| InputError::NotUnicode("database name"))?; + + if !db_name + .chars() + .all(|x: char| x.is_alphanumeric() || x == '\\' || x == '/' || x == '.') + { + return Err(InputError::ForbiddenCharactersInDB(db_name.to_owned())); + } + + Ok(UnlinkOp { db_name }) + } +} + #[derive(Debug)] pub struct MoveOp<'a> { src: PathBuf, @@ -927,12 +956,16 @@ fn cmd_close(ctx: &mut Context) -> Result<(), InputError> { } /// Set up Context structure with data parsed from command unlink -fn cmd_unlink(ctx: &mut Context) -> Result<(), InputError> { +fn cmd_unlink(ctx: &mut Context, unlink_op: UnlinkOp) -> Result<(), InputError> { if let Some(open_db) = &ctx.open_db { - fs::remove_file(open_db).map_err(ProcessingError::from)?; - } else { - return Err(InputError::NoDBForAction("UNLINK")); + return Err(InputError::DBStillOpen(open_db.clone())); } + + // We checked that the db_name doesn't have any '/' or '\\' in it before + // adding to the UnlinkOp struct, so it's safe to say the path is just a local file and + // won't accidentally delete something in a different directory. + fs::remove_file(unlink_op.db_name).map_err(ProcessingError::from)?; + Ok(()) } @@ -960,7 +993,10 @@ fn process_line(ctx: &mut Context, line: &[u8]) -> Result<(), InputError> { cmd_move(ctx, move_op) } b"CLOSE" => cmd_close(ctx), - b"UNLINK" => cmd_unlink(ctx), + b"UNLINK" => { + let unlink_op = UnlinkOp::new(remainder.unwrap())?; + cmd_unlink(ctx, unlink_op) + } _ => Err(InputError::UnknownCommand( String::from_utf8(cmd.to_owned()).unwrap(), )), From 1bf9ea5b1ec89bee8c989faf29c637c7c5e4835e Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Mon, 10 Apr 2023 19:37:46 -0700 Subject: [PATCH 5/7] Fix Rust Clippy linter complaints --- libclamav_rust/build.rs | 6 +++--- libclamav_rust/src/cdiff.rs | 10 +++++----- libclamav_rust/src/fuzzy_hash.rs | 2 +- libclamav_rust/src/logging.rs | 6 +++++- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/libclamav_rust/build.rs b/libclamav_rust/build.rs index e2f90a50cb..9f16bb98b3 100644 --- a/libclamav_rust/build.rs +++ b/libclamav_rust/build.rs @@ -182,7 +182,7 @@ fn detect_clamav_build() -> Result<(), &'static str> { // LLVM is optional, and don't have a path to each library like we do with the other libs. let llvm_libs = env::var("LLVM_LIBS").unwrap_or("".into()); - if llvm_libs != "" { + if !llvm_libs.is_empty() { match env::var("LLVM_DIRS") { Err(env::VarError::NotPresent) => eprintln!("LLVM_DIRS not set"), Err(env::VarError::NotUnicode(_)) => return Err("environment value not unicode"), @@ -199,7 +199,7 @@ fn detect_clamav_build() -> Result<(), &'static str> { llvm_libs .split(',') - .for_each(|filepath_str| match parse_lib_path(&filepath_str) { + .for_each(|filepath_str| match parse_lib_path(filepath_str) { Ok(parsed_path) => { println!("cargo:rustc-link-search={}", parsed_path.dir); eprintln!(" - requesting that rustc link {:?}", &parsed_path.libname); @@ -278,7 +278,7 @@ struct ParsedLibraryPath { // Parse a library path, returning the portion expected after the `-l`, and the // directory containing the library -fn parse_lib_path<'a>(path: &'a str) -> Result { +fn parse_lib_path(path: &str) -> Result { let path = PathBuf::from(path); let file_name = path .file_name() diff --git a/libclamav_rust/src/cdiff.rs b/libclamav_rust/src/cdiff.rs index 3257476815..e7547cebbe 100644 --- a/libclamav_rust/src/cdiff.rs +++ b/libclamav_rust/src/cdiff.rs @@ -477,7 +477,7 @@ pub fn script2cdiff(script_file_name: &str, builder: &str, server: &str) -> Resu .map_err(|e| CdiffError::FileCreate(cdiff_file_name.to_owned(), e))?; // Open the original script file for reading - let script_file: File = File::open(&script_file_name) + let script_file: File = File::open(script_file_name) .map_err(|e| CdiffError::FileOpen(script_file_name.to_owned(), e))?; // Get file length @@ -599,7 +599,7 @@ pub fn cdiff_apply(file: &mut File, mode: ApplyMode) -> Result<(), CdiffError> { let dsig = read_dsig(file)?; debug!("cdiff_apply() - final dsig length is {}", dsig.len()); if is_debug_enabled() { - print_file_data(dsig.clone(), dsig.len() as usize); + print_file_data(dsig.clone(), dsig.len()); } // Get file length @@ -1022,7 +1022,7 @@ where 0 => break, n_read => { decompressed_bytes = decompressed_bytes + n_read + 1; - match linebuf.get(0) { + match linebuf.first() { // Skip comment lines Some(b'#') => continue, _ => process_line(ctx, &linebuf).map_err(|e| CdiffError::Input(line_no, e))?, @@ -1071,7 +1071,7 @@ fn read_dsig(file: &mut File) -> Result, SignatureError> { // as the offset in the file that the header ends. fn read_size(file: &mut File) -> Result<(u32, usize), HeaderError> { // Seek to beginning of file. - file.seek(SeekFrom::Start(0))?; + file.rewind()?; // File should always start with "ClamAV-Diff". let prefix = b"ClamAV-Diff"; @@ -1116,7 +1116,7 @@ fn get_hash(file: &mut File, len: usize) -> Result<[u8; 32], CdiffError> { let mut hasher = Sha256::new(); // Seek to beginning of file - file.seek(SeekFrom::Start(0))?; + file.rewind()?; let mut sum: usize = 0; diff --git a/libclamav_rust/src/fuzzy_hash.rs b/libclamav_rust/src/fuzzy_hash.rs index fd4dd5aed3..8ac89e9fc2 100644 --- a/libclamav_rust/src/fuzzy_hash.rs +++ b/libclamav_rust/src/fuzzy_hash.rs @@ -123,7 +123,7 @@ pub struct FuzzyHashMeta { /// Initialize the hashmap #[no_mangle] pub extern "C" fn fuzzy_hashmap_new() -> sys::fuzzyhashmap_t { - Box::into_raw(Box::new(FuzzyHashMap::default())) as sys::fuzzyhashmap_t + Box::into_raw(Box::::default()) as sys::fuzzyhashmap_t } /// Free the hashmap diff --git a/libclamav_rust/src/logging.rs b/libclamav_rust/src/logging.rs index 7e70cf8eaf..7c0af7b863 100644 --- a/libclamav_rust/src/logging.rs +++ b/libclamav_rust/src/logging.rs @@ -88,8 +88,12 @@ mod tests { /// API exported for C code to log to standard error using Rust. /// This would be be an alternative to fputs, and reliably prints /// non-ASCII UTF8 characters on Windows, where fputs does not. +/// +/// # Safety +/// +/// This function dereferences the c_buff raw pointer. Pointer must be valid. #[no_mangle] -pub extern "C" fn clrs_eprint(c_buf: *const c_char) -> () { +pub unsafe extern "C" fn clrs_eprint(c_buf: *const c_char) { if c_buf.is_null() { return; } From e7118ec18ce55f7fdc0a1d25eb55a77d71c8a5f8 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Wed, 12 Apr 2023 23:47:44 -0700 Subject: [PATCH 6/7] Freshclam, Sigtool: Fix bug creating new files in CLD with CDIFF The CLOSE command is failing to create a file when appending changes if the file does not already exist. This prevents adding new files to a database with a CDIFF and caused failures applying the test-3.cdiff file in the freshclam feature tests. Also improved the error message to show which command, specifically, is failing (not just the line number). --- libclamav_rust/src/cdiff.rs | 62 ++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 12 deletions(-) diff --git a/libclamav_rust/src/cdiff.rs b/libclamav_rust/src/cdiff.rs index e7547cebbe..dc369906c7 100644 --- a/libclamav_rust/src/cdiff.rs +++ b/libclamav_rust/src/cdiff.rs @@ -25,8 +25,8 @@ use std::{ io::{prelude::*, BufReader, BufWriter, Read, Seek, SeekFrom, Write}, iter::*, os::raw::c_char, - path::PathBuf, - str, + path::{Path, PathBuf}, + str::{self, FromStr}, }; use crate::sys; @@ -97,8 +97,12 @@ pub enum CdiffError { /// /// This error *may* wrap a processing error if the command has side effects /// (e.g., MOVE or CLOSE) - #[error("{1} on line {0}")] - Input(usize, InputError), + #[error("{err} on line {line}: {operation}")] + Input { + line: usize, + err: InputError, + operation: String, + }, /// An error encountered while handling a particular CDiff command #[error("processing {1} command on line {2}: {0}")] @@ -155,14 +159,20 @@ pub enum InputError { #[error("No DB open for action {0}")] NoDBForAction(&'static str), + #[error("Invalid DB \"{0}\" open for action {1}")] + InvalidDBForAction(String, &'static str), + #[error("File {0} not closed before opening {1}")] NotClosedBeforeOpening(String, String), #[error("{0} not Unicode")] NotUnicode(&'static str), - #[error("Forbidden characters found in database name {0}")] - ForbiddenCharactersInDB(String), + #[error("Invalid database name {0}. Characters must be alphanumeric or '.'")] + InvalidDBNameForbiddenCharacters(String), + + #[error("Invalid database name {0}. Must not specify parent directory.")] + InvalidDBNameNoParentDirectory(String), #[error("{0} missing for {1}")] MissingParameter(&'static str, &'static str), @@ -306,9 +316,20 @@ impl<'a> UnlinkOp<'a> { if !db_name .chars() - .all(|x: char| x.is_alphanumeric() || x == '\\' || x == '/' || x == '.') + .all(|x: char| x.is_alphanumeric() || x == '.') { - return Err(InputError::ForbiddenCharactersInDB(db_name.to_owned())); + // DB Name contains invalid characters. + return Err(InputError::InvalidDBNameForbiddenCharacters( + db_name.to_owned(), + )); + } + + let db_path = PathBuf::from_str(db_name).unwrap(); + if db_path.parent() != Some(Path::new("")) { + // DB Name must be not include a parent directory. + return Err(InputError::InvalidDBNameNoParentDirectory( + db_name.to_owned(), + )); } Ok(UnlinkOp { db_name }) @@ -673,10 +694,22 @@ fn cmd_open(ctx: &mut Context, db_name: Option<&[u8]>) -> Result<(), InputError> if !db_name .chars() - .all(|x: char| x.is_alphanumeric() || x == '\\' || x == '/' || x == '.') + .all(|x: char| x.is_alphanumeric() || x == '.') { - return Err(InputError::ForbiddenCharactersInDB(db_name.to_owned())); + // DB Name contains invalid characters. + return Err(InputError::InvalidDBNameForbiddenCharacters( + db_name.to_owned(), + )); + } + + let db_path = PathBuf::from_str(db_name).unwrap(); + if db_path.parent() != Some(Path::new("")) { + // DB Name must be not include a parent directory. + return Err(InputError::InvalidDBNameNoParentDirectory( + db_name.to_owned(), + )); } + ctx.open_db = Some(db_name.to_owned()); Ok(()) @@ -941,6 +974,7 @@ fn cmd_close(ctx: &mut Context) -> Result<(), InputError> { // Test for lines to add if !ctx.additions.is_empty() { let mut db_file = OpenOptions::new() + .create(true) .append(true) .open(&open_db) .map_err(ProcessingError::from)?; @@ -998,7 +1032,7 @@ fn process_line(ctx: &mut Context, line: &[u8]) -> Result<(), InputError> { cmd_unlink(ctx, unlink_op) } _ => Err(InputError::UnknownCommand( - String::from_utf8(cmd.to_owned()).unwrap(), + String::from_utf8_lossy(&cmd).to_string(), )), } } @@ -1025,7 +1059,11 @@ where match linebuf.first() { // Skip comment lines Some(b'#') => continue, - _ => process_line(ctx, &linebuf).map_err(|e| CdiffError::Input(line_no, e))?, + _ => process_line(ctx, &linebuf).map_err(|e| CdiffError::Input { + line: line_no, + err: e, + operation: String::from_utf8_lossy(&linebuf).to_string(), + })?, } } } From eb53e11d194faecd672cec13d93c8249156bdd51 Mon Sep 17 00:00:00 2001 From: Micah Snyder Date: Fri, 14 Apr 2023 19:15:19 -0700 Subject: [PATCH 7/7] Tests: python 3.5 compatibility fixes --- unit_tests/sigtool_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unit_tests/sigtool_test.py b/unit_tests/sigtool_test.py index 3da0aba362..73c9ba4159 100644 --- a/unit_tests/sigtool_test.py +++ b/unit_tests/sigtool_test.py @@ -92,7 +92,7 @@ def test_sigtool_01_run_cdiff(self): self.log.warning('VG: {}'.format(os.getenv("VG"))) (TC.path_tmp / 'run_cdiff').mkdir() - os.chdir(TC.path_tmp / 'run_cdiff') + os.chdir(str(TC.path_tmp / 'run_cdiff')) command = '{valgrind} {valgrind_args} {sigtool} --unpack {cvd}'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, sigtool=TC.sigtool, @@ -131,7 +131,7 @@ def test_sigtool_02_rust_logs_messages_work(self): self.log.warning('VG: {}'.format(os.getenv("VG"))) (TC.path_tmp / 'run_cdiff_log').mkdir() - os.chdir(TC.path_tmp / 'run_cdiff_log') + os.chdir(str(TC.path_tmp / 'run_cdiff_log')) command = '{valgrind} {valgrind_args} {sigtool} --unpack {cvd}'.format( valgrind=TC.valgrind, valgrind_args=TC.valgrind_args, sigtool=TC.sigtool,