Clam 1594 fix allmatch remove checks (part 2) by val-ms · Pull Request #684 · Cisco-Talos/clamav

val-ms · 2022-08-24T18:42:36Z

This is the final part of the alllmatch overhaul:

Removes checks for SCAN_ALLMATCHES
Makes sure to stop scanning when scan return code is != CL_SUCCESS, instead of only when CL_VIRUS or CL_BREAK or whatever - to make sure we stop scanning when exceeding scan limits.
Some code cleanup and some swap from use of CL_CLEAN -> CL_SUCCESS to try to differentiate the purpose of cl_error_t from the scan verdict. The cl_error_t is to indicate if something succeeded or failed, and if it failed most likely we should abort the scan with very few exceptions. Note most file parsing errors should not be fatal to the scan, so catching non-fatal errors or even just debug-logging them and moving on is the way to go.
Adds some additional tests and restructure the clamscan tests into separate files because clamscan_test.py was getting way too long. Also fixed the clamscan .ign2 test.
Makes sure that magic_scan() only returns errors that should halt the scan, and makes sure that all calls to magic_scan() propagate errors reported by magic_scan(). Many used to only propagate the CL_VIRUS return, but that would ignore critical errors like CL_EMEM and CL_ETIMEOUT. In addition, the functions that originate CL_VIRUS and CL_ETIMOUT now also set a ctx->abort_scan flag which is checked in magic_scan() to ensure that scans terminate even if the return code was somehow lost.

For code review, It may be best to review it commit-by-commit in the https://github.com/Cisco-Talos/clamav/pull/684/commits page. I tried to keep each commit on separate files, or at least separate issues so that none is incomplete on its own.

val-ms · 2022-08-25T16:43:37Z

Hmm uh oh, merge conflicts. Not sure how this happened. I will try to fix it.

My force push a moment ago was because the .idb icon database test files didn't get added in the pe-allmatch test commit because they are apparently ignored by the .gitignore file (conflicting file extension) and it caused test failures.

val-ms · 2022-08-25T17:40:39Z

Merge conflict fixed. There wasn't a real merge conflict, just the hashes for the previous commits were different because GitHub's rebase+merge isn't a fast-forward merge when a rebase isn't required. It still does the rebase, which alters the hash even though it accomplishes nothing. Well done, GitHub, well done.

lgtm-com · 2022-08-25T17:44:13Z

This pull request introduces 2 alerts when merging 9dc4dc6 into edb399f - view on LGTM.com

new alerts:

2 for Static array access may cause overflow

val-ms · 2022-08-25T17:53:42Z

Oops, the test.exe was apparently also missing because of the .gitignore. Added that and did another force-push.

val-ms · 2022-08-25T17:59:10Z

Another force-push, fixed the issues identified by the lgtm-com app. It was memcpy'ing to the wrong hash array (md5 instead of sha1 or sha256).

And fix issue where calls to magic_scan may not propagate critical errors.

Remove `det` checks (like `virus_found`) that was a part of the old allmatch implementation.

And fix issue where magic_scan() critical error may be ignored.

I found mixed types and multiple bugs in the hashtable/map/set code, and very little documentation. The most documentation available is the bytecode compiler users manual. Although I also found one discrepancy there with the return value for the BC API map_remove function that calls cli_map_removekey() and so put in an issue with the compiler project for the documentation. Most notably is that this hashtab.c had a lot of functions returning negative enum values instead of returning the enums and then having the caller evaluate the return code to return a negative/0/1 result. This commit fixes all of that, and adds in a bunch of documentation to explain the purpose and behavior of each function and structure provided by hashtab.c/.h. Specific bugs that I know I fixed outside of code quality improvements: - cli_hashset_toarray() was returning CL_ENULLARG / CL_EMEM on failure, when the caller is expecting a ssize_t to indicate how big of an array is allocated. It now returns -1 on failure. I also found that an attempt was made to have the same API that takes a mempool pointer even if mempool is disabled. I preserved that, but made it so the macro is in all-caps so it's more obvious what is going on.

And add a test that makes sure it only alerts once when allmatch disabled.

Add new tests to verify correct behaviour combining: - a "potentially unwanted" indicator - a regular alert (strong indicator) The test file is a zip that: 1. first has a malformed PNG file used to trigger the heuristic alert when using `--alert-broken-media`. 2. second has the clam.exe program used to trigger the normal alert. Included are tests with this file for a number of situations: 1. Test the heuristic alert must not alert because neither allmatch is specified, nor --heuristic-scan-precedence. 2. Test the heuristic alert must alert because we don't use the sig for the clam.exe file. 3. Test the heuristic alert must alert first because --heuristic-scan-precedence is enabled. We won't see the other alert because it's not allmatch mode. 4. Test we use --allmatch but we don't use --heuristic-scan-precedence. That means the NDB sig should alert first, even though the heuristic is encountered first. Note the verify_output() uses STRICT_ORDER. 5. Test we use --allmatch AND we use --heuristic-scan-precedence. That means the heuristic is encountered first and should be treated equally, so it should alert first. Note the verify_output() uses STRICT_ORDER.

When heuristic-precendence is enabled, add PUA as Strong instead of as PotentiallyUnwanted. This way, they will be treated equally and reported in order in allmatch mode.

When using --heuristic-precendence, the heuristic alerts in the image parsers much be reported as regular matches, which means that when not in all-match mode, the scan should abort. Right now, those matches aren't being recorded so scanning continues in archives containing more than just the malformed image file. This fix returns the "CL_VIRUS" status code when heuristic-precendence is enabled and allmatch is not enabled.

Also removed message saying 'Infected with ...'. It's not really infected, and with allmatch it'll only print the last match anyways. And fix issue where magic_scan() critical error may be ignored.

When potentially unwanted indicators are found, they are not reported right away, except in heuristic-precedence mode. At the end of the scan, we then report all of the heuristic/PUA alerts. In my initial overhaul of the allmatch mode, I introduced an issue where it is never reporting more than 1 heuristic/PUA alert. This commit fixes that by reporting all potentially-unwanted indicators at the end. Note that instead of using `cli_virus_found_cb()` which would report the top layer file descriptor as well in the callback, I'm reporting `-1` for the file descriptor instead, because we don't know when the alert was added. If it was at a deeper layer in the file, we would not longer have access to it at the end of the scan. Also removed excessive debug statements '... infected with ...' that use `cli_get_last_virus()` because it doesn't really reflect all of the matches in allmatch mode, because the match would've been reported previously (so it is excessive), and because I don't like the word 'infected'. :)

The bytecode source files largely use `int` instead of the appropriate `cl_errot_t` for clamav status codes, as well for boolean variables. This hides warnings that would indicate bugs, and makes it harder to read the code. I haven't gone as in depth as with some other code cleanups. This largely just replaces function interfactes and ret variables that use `int` with `cl_error_t`. I also swapped a couple of `int`s to `bool`s. While doing so I found that the `cli_bytecode_context_setpdf()` function was incorrectly placed in the `bytecode_api.c` file instead of the next to similar functions (`cli_bytecode_context_setpe`, etc.) in bytecode.c. It's not an API function, so I moved it to the correct location. I also eliminated a couple of compiler warnings: - LLVM's CFG.h header emits a warning about a multi-line comment, so that crops up with using LLVM for the bytecode runtime. I disabled the warning through CMake. - C doesn't like using the `inline` keyword on cli_dbgmsg in the declaration in `bytecode2llvm.c` because we're compiling the bytecode runtimes as a separate object file from the rest of libclamav. It doesn't appear to be a functional issue, but I swapped that file over to use `cli_dbgmsg_no_inline()` instead, just in case. I would hope link-time-optimization will inline it anyways.

Based on changes observed in the commits: - 08fef61 - 1aa8768 ... I believe that the iptr variable was intended to be used and was accidentally not used. In addition, adding the `ULL` suffix in the second location to match the first, because it appears to have been accidentally omited.

And fix issue where critical errors in magic_scan done by pdf_scan_contents() may be ignored.

And fix issue where call to magic_scan may not propagate critical errors.

A copy-paste error in the markdown support for building the macOS- installer caused an issue where markdown module may be accidentally used instead of pytest. Strangely, this still worked okay up until we converted the clamscan tests into a directory of tests, at which point the `python3 -m markdown` invocation was upset that it was trying to execute a directory instead of a .py file. This commit removes the line that overwrites the python test command with using the markdown module.

It appears the result from the _x509_to_pem() function was accidentally hardcoded to a `0` (success)

This does not change the ABI because enums are ints. Though it may add warnings.

Fix issue where critical errors from magic_scan may not be propagated. Fix issue where file name and descriptor may be leaked if keeptmp is enabled.

And fix issue where magic_scan() critical error may be ignored.

And fixed a couple of conditions where critical scan errors may be ignored.

Also fixed a number of conditions where magic_scan() critical errors may be ignored. To ensure that the scan truly aborts for signature matches (not in allmatch mode) and for timeouts, the `ctx->abort` option is now set in these two conditions, and checked in several spots in magic_scan(). Additionally, I've consolidated some of the "scan must halt" type of checks (mostly large switch statements) into a function so that we can use the exact same logic in a few places in magic_scan(). I've also fixed a few minor warnings and code format issues.

Simple pattern matches that don't use fancier features like wildcards use the Boyer-Moore pattern matcher. Unfortunately, relative offsets such as PE section bounding is not fully implemented for our Boyer-Moore matcher. Specifically, the minimum offset is set but not the maximum, which would result in matching outside of the given section. But in addition, the `bm_shift` mechanism used in `cli_bm_scanbuff()` doesn't work with these relative offsets and will never even attempt to do the match. This commit fixes the issue by detecting the 'S' in the `offset` variable at signature load time and assigning the pattern to the Aho-Corasick matcher, where everything appears to work as intended.

The header parsing / executable metadata collecting functions for the PE, ELF, and Mach-O file types were using `int` for the return type. Mostly they were returning 0 for success and -1, -2, -3, or -4 for failure. But in some cases they were returning cl_error_t enum values for failure. Regardless, the function using them was treating 0 as success and non-zero as failure, which it stored as -1 ... every time. This commit switches them all to use cl_error_t. I am continuing to storeo the final result as 0 / -1 in the `peinfo` struct, but outside of that everything has been made consistent. While I was working on that, I got a tad side tracked. I noticed that the target type isn't an enum, or even a set of #defines. So I made an enum and then changed the code that uses target types to use the enum. I also removed the `target` parameter from a number of functions that don't actually use it at all. Some recursion was masking the fact that it was an unused parameter which is why there was no warning about it.

val-ms · 2022-08-29T17:56:53Z

I believe that the allmatch overhaul is complete with this PR (now).

We can probably start regression testing with this branch, although I do intend to do a rebase to squash one of the commits from this PR into one from the feature/fix-allmatch branch before doing another PR from feature/fix-allmatch over to main.

lgtm-com · 2022-08-29T18:17:50Z

This pull request fixes 1 alert when merging cd30f36 into edb399f - view on LGTM.com

fixed alerts:

1 for Empty branch of conditional

…code Fixes copy-paste error that caused a failures in the internal test suite when PCRE matching.

ragusaa · 2022-08-29T21:47:59Z

libclamav/asn1.c

                            ret = cli_append_virus(ctx, crt->name ? crt->name : "(unnamed CRB rule)");
-                            if ((ret == CL_VIRUS) && !SCAN_ALLMATCHES) {
+                            if (ret == CL_VIRUS) {
                                crtmgr_free(&newcerts);


There are a couple places in this function with this particular free. Do you think it's worth it to move this to the finish block so that it is only done in one place?

Sure can do.

I'm afraid of doing the clean up on the RTF parser, though. 😅

lgtm-com · 2022-08-29T21:58:15Z

This pull request fixes 1 alert when merging e99a45d into edb399f - view on LGTM.com

fixed alerts:

1 for Empty branch of conditional

Fix bug introduced with caching when relocating feature. And document reason for disabling cache with metadata JSON enabled

lgtm-com · 2022-08-29T22:42:31Z

This pull request fixes 1 alert when merging aa37f20 into edb399f - view on LGTM.com

fixed alerts:

1 for Empty branch of conditional

lgtm-com · 2022-08-30T06:15:20Z

This pull request fixes 1 alert when merging 078aa9d into edb399f - view on LGTM.com

fixed alerts:

1 for Empty branch of conditional

val-ms · 2022-09-07T20:36:11Z

Merged into the feature branch for extended testing and cleanup. Additional review to continue in #687

val-ms requested a review from ragusaa August 24, 2022 18:42

val-ms changed the base branch from main to feature/fix-allmatch August 24, 2022 18:42

val-ms force-pushed the CLAM-1594-fix-allmatch-remove-checks branch from 76fe3ec to faa255d Compare August 25, 2022 16:36

Allmatch: Remove individual checks for bytecode matches

49f8d7b

val-ms force-pushed the CLAM-1594-fix-allmatch-remove-checks branch from faa255d to 9dc4dc6 Compare August 25, 2022 17:15

val-ms force-pushed the CLAM-1594-fix-allmatch-remove-checks branch from 9dc4dc6 to 2957497 Compare August 25, 2022 17:53

val-ms force-pushed the CLAM-1594-fix-allmatch-remove-checks branch from 2957497 to f8821c5 Compare August 25, 2022 17:58

val-ms added 18 commits August 29, 2022 10:45

Allmatch: Remove individual checks for 7zip matches

b53ee4e

And fix issue where calls to magic_scan may not propagate critical errors.

Disk partition parsers: Remove all-match checks + code cleanup

634c0f8

Install Shield: Remove all-match checks + code cleanup

a4c4ae8

AutoIT: Remove all-match checks + code cleanup

7ede72e

Remove `det` checks (like `virus_found`) that was a part of the old allmatch implementation.

CPIO: Remove all-match checks + code cleanup

614b812

And fix issue where magic_scan() critical error may be ignored.

ISO9660: Remove all-match checks + code cleanup

fd21b07

HWP3, ASN1, blob: Remove all-match checks

524de1a

Tests: Add NDB & LDB to allmatch test

0704d39

And add a test that makes sure it only alerts once when allmatch disabled.

Squashme into: Allmatch-mode overhaul, part 1: append_virus

bc8b535

When heuristic-precendence is enabled, add PUA as Strong instead of as PotentiallyUnwanted. This way, they will be treated equally and reported in order in allmatch mode.

PDF: Remove all-match checks

4dddeaf

XAR: Remove allmatch checks

3edd326

Also removed message saying 'Infected with ...'. It's not really infected, and with allmatch it'll only print the last match anyways. And fix issue where magic_scan() critical error may be ignored.

Code cleanup: be explicit about what if-conditions are checking

0305378

val-ms added 16 commits August 29, 2022 10:48

PCRE: Remove allmatch check + minor code cleanup

3ea22c8

MSXML: Remove allmatch checks + minor code cleanup

e7a9300

OLE2: Remove allmatch checks + minor code cleanup

5a474bf

PDF: remove allmatch checks

9cf949f

And fix issue where critical errors in magic_scan done by pdf_scan_contents() may be ignored.

PE: Remove allmatch checks + minor code cleanup

4b7efd9

And fix issue where call to magic_scan may not propagate critical errors.

Fix certificate load x509 -> PEM result check

7e0d459

It appears the result from the _x509_to_pem() function was accidentally hardcoded to a `0` (success)

Correct parameter type in cl_strerror() public API

b72e97d

This does not change the ABI because enums are ints. Though it may add warnings.

Fix error handling in Mach-O and ELF bytecode unpacker hooks

80d5bb3

Fix issue where critical errors from magic_scan may not be propagated. Fix issue where file name and descriptor may be leaked if keeptmp is enabled.

TAR: Remove allmatch checks

7b8492a

And fix issue where magic_scan() critical error may be ignored.

ZIP: Remove allmatch checks

b6aecb4

And fix issue where magic_scan() critical error may be ignored.

Excel (XLM, VBA): Remove allmatch checks + some code cleanup

cf341ec

And fixed a couple of conditions where critical scan errors may be ignored.

Phishcheck: remove allmatch check

346698a

val-ms force-pushed the CLAM-1594-fix-allmatch-remove-checks branch from 44fb9a0 to cd30f36 Compare August 29, 2022 17:50

Squashme into Matcher: Remove allmatch checks and significantly tidy …

e99a45d

…code Fixes copy-paste error that caused a failures in the internal test suite when PCRE matching.

ragusaa reviewed Aug 29, 2022

View reviewed changes

Squashme into Allmatch-mode overhaul, part 1: append_virus

aa37f20

Fix bug introduced with caching when relocating feature. And document reason for disabling cache with metadata JSON enabled

val-ms added 2 commits August 29, 2022 22:48

Squashme into RTF commit. Fix bug introduced that leaks FD.

ca08d0e

Squashme into Autoit commit. Fix bug introduced that leaks FD, tempfile.

078aa9d

val-ms merged commit 9243b2e into Cisco-Talos:feature/fix-allmatch Sep 7, 2022

val-ms deleted the CLAM-1594-fix-allmatch-remove-checks branch September 7, 2022 20:35

Conversation

val-ms commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

val-ms commented Aug 25, 2022

Uh oh!

val-ms commented Aug 25, 2022

Uh oh!

lgtm-com bot commented Aug 25, 2022

Uh oh!

val-ms commented Aug 25, 2022

Uh oh!

val-ms commented Aug 25, 2022

Uh oh!

val-ms commented Aug 29, 2022

Uh oh!

lgtm-com bot commented Aug 29, 2022

Uh oh!

ragusaa Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

val-ms Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Aug 29, 2022

Uh oh!

lgtm-com bot commented Aug 29, 2022

Uh oh!

lgtm-com bot commented Aug 30, 2022

Uh oh!

val-ms commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

val-ms commented Aug 24, 2022 •

edited

Loading