Notably the commit adds a heuristic alert when VBA is extracted using
the new VBA extraction code and similarly adds "HasMacros":true to the
JSON scan properties.
In addition, a change was added to the cli_sanitize_filepath() function
so it converts posix pathseps to Windows pathseps on Windows and also
outputs a sanitized basename pointer (optional) which is used when
generating a temporary filename so that using a prefix with pathseps in
it won't cause file creation failures (observed with --leave-temps where
original filenames are incorporated into temporarily filenames).
Included soem error handling improvements for cli_vba_scandir() to
better track alert and macro detections.
Downgraded utf8 conversion error messages to debug messages because they
are too verbose in files with invalid filenames (observed in some
malware).
Changed the xlm macro and vba project temp filenames to include
"xlm_macros" and "vba_project" prefix, to make it easier to find them.
Relocated XLM and VBA temp files from the top-level tmp directory to the
current sub_tmpdir, so tempfiles for a given scan are more organized.
Fix an infinite loop in the new XLM macro parser.
Fix error handling, resource cleanup in OLE2 parser.
Fix issues tracking detected "viruses" in VBA & OLE2 parsers affecting
non-allmatch (regular) scan mode, wherein multiple viruses may be found
but each record lost and the overall detection comes up clean.
Also silence switch() fall-through warning for WORD/PPT/XL/HWP (OOXML)
file type fall-throughs to the ZIP parser (because they are zips).
Also silence switch() fall-through warning when handling the limits-
exceeded error types, checking for the limits-exceeded heuristic, and
continuing on to bail out with a clean verdict.
Changes cli_checkfp_virus to a recursive function which checks all
parent fmaps in the context for false positives
Simplifies params needed for cli_checkfp_virus to use the current digest
and fmap length that resides within the fmap struct itself
Add missing ping_clamd() declaration in client.h
Fix check for ping option to first check if ping option is NULL before
strdup'ing and checking if the alloc failed.
Fix format string for uint64_t print.
Correctly assign name pointer to stack buffer in cpio parser.
Remove vestigial variables from insert_list() function matcher-ac.c,
left over from before the load-time optimizations completely
restructured everything.
Silence warnings about unused parameters in progress bar callback
function.
Valgrind reports uninitialized `tag` stack buffer being used. While this
appears to be a false positive, it can't hurt to initialize this and
similar buffers in this function.
Fixes bound checks in recently rewritten VBA parser code (i.e. issue
does not affect prior versions).
Also improves VBA terminator header parsing to better match the spec,
per recommendation by Jonas Zaddach.
The clamonacc command doesn't present a `--debug` flag, but according
to your blog https://blog.clamav.net/2019/09/understanding-and-transitioning-to.html
the correct flag should be `--verbose`:
"[...]This is akin to clamd’s or clamscan’s --debug option, but isn’t
quite so noisy as either of those. By default, clamonacc does not
print any output after daemonizing, so you will have to pair this
option with --log or --foreground to use it.[...]"
- Updated `SUMHEAD` string macro with corrected alignment of `DBVER`
and `DBTIME` labels to accommodate current (wider) ClamAV version
numbers.
- Tweaked printing of primary thread counts and header values so
they fit in 80-column windows (as best as possible; the `h` suffix
of the `DBTIME` hour value doesn't fit, but at least the value
does). Previously, an 83-column window was required for the
information to fit.
- Make another line truncate at screen edge instead of 2 cols short.
- Make print_con_info truncate at screen edge and properly handle
messages with a newline.
- Use strrchr in print_con_info; handle wrapped lines in task list.
- Improve alignment of task list header labels.
- Fix null host/port bug; curses_inited should be initially 0.
- Fix compiler warning; fixes to line wrap patch; correct ENGINE
field width; a little tidy-up.
- Fix warnings; cleanup mem stats; version parsing fix; formatting tweaks
- Use newterm() and delscreen() for cleaner exit.
Also relocated codepage table from msdoc.h to entconv.h
Also adds new macros for codepages to reduce use of magic numbers when
referencing code pages elsewhere in libclamav.
294429: Negative check for fd_out occurs after a call to fdopen where
the value must not be negative. Coverity interprets this as a high
severity issue, even though it really isn't. Removing the needless check
should silence the false positive.
Somewhere between bison versions 3.5 and 3.6.4, having multiple
characters in a single-quoted string went from a warning to an error.
This commit corrects that by changing a literal to string,
Removed all autotools generates files. Autotools (autoconf, automake,
libtool, pkg-config, m4) will be required from now on for builds from
git clones.
Added autogen.sh to be run before ./configure.
Significant update to main .gitignore file.
Removed extraneous .gitignore files. A Git repository only needs one
.gitignore file.
Overflow check "(value >> 32) * 10 < INT32_MAX" may not work in
certain conditions, e.g. value is 0xcccccccdbcdc9cc
Note: This fixes oss-fuzz bug 16117.
290424 Missing break in switch - In hash_match: Missing break
statement between cases in switch statement
290414 Resource leak - In cli_scanishield_msi: Leak of memory or
pointers to system resources. Memory leak in a fail case
288197 Resource leak - In decrypt_any: Leak of memory or pointers
to system resources. Memory leak in a fail case
290426 Resource leak - In cli_magic_scan: Leak of memory or pointers
to system resources. Leaked a file prefix when running with
--save-temps
192923 Resource leak - In cli_scanrar: Leak of memory or pointers to
system resources. Leaked a file descriptor if a virus was found in
a RAR file comment
225146 Resource leak - In cli_scanegg: Leak of memory or pointers
to system resources. Leaked a file descriptor if unable to write
a comment file to disk
290425 Resource leak - In scan_common: Leak of memory or pointers
to system resources. Memory leaks in various fail cases.
Also changes cli_scanrar to write out the file comment only if
--leave-temps is specified and scan the buffer (like what is done
in cli_scanegg) instead of writing the file out, scanning that,
and then deleting the file if --leave-temps is not specified.
The unit tests stopped working when correcting an issue with a
switch statement that determined what type of signature had matched
on a Google SafeBrowsing GDB rule. Looking into the unit tests, it
looks like the code had always assumed that the test cases would be
detected by a malware test rule in unit_tests/input/daily.gdb, but
now some of the tests get matched on the phishing test rule.
I updated the test logic to be more clear, and added tests for both
cases now.
Fix some memory leaks in libclamav/scanners.c
Fixes a possible stack buffer overflow introduced in 0.103 development
when we added optional names to file maps (fmaps). The CPIO parser uses
a stack buffer to store the name (if present). If no name present, then
the stack buffer was passed unitialized to the fmap scanning function
which could cause an overflow.
This fix both initializes the buffer and uses a pointer so the scan
function gets NULL instead of a buffer in the event that a name isn't
present as that's the intended way to use the API, rather than passing
an empty string name buffer.
Fixes a stack overflow that resulted in stack corruption and general
mayhem. This bugfix only applies to the 0.103 dev branch.
The issue was caused by buffering formatted XLM macro content to a small
buffer without regard for possible overflow. Instead of buffering
manually, use of snprintf and later cli_writen were replaced with direct
calls to fwrite / fprintf / fputc.
The ARJ parser fix from 0.102.3 was insufficient and still allowed for some overflow. This commit passes correct buffer sizes into text_normalize functions.
Add notices to man pages and help strings cautioning against running
bytecode signatures from untrusted sources.
Also adds missing BytecodeUnsigned option to clamd.conf.sample files.
A malicious user could replace a scan target's directory with a symlink
to another path to trick clamscan, clamdscan, or clamonacc into removing
or moving a different file (eg. a critical system file). The issue would
affect users that use the `--move` or `--remove` options for clamscan,
clamdscan, and clamonacc.
This patch gets the real path for the scan target before the scan,
and if the file alerts and the --move or --remove quarantine features
are used, it mitigates the symlink attack by traversing the path one
directory at a time until reaching the leaf directory where the scan
target file resides before unlinking (or renaming) the file directly.
This commit applies a similar tactic used in the previous commit for
Windows builds, using the Win32 Native API to traverse a path and delete
or move files by handle rather than by file path.
I had some trouble using SetFileInformationByHandle to rename a file by
handle, so for Windows instead it will copy the file to the new location
and then use the safe unlink technique to remove the old file. If the
symlink attack occurs, the unlink will fail, and the system will not be
damaged.
For more information about AV quarantine attacks using links, see the
[RACK911 Lab's report](https://www.rack911labs.com/research/exploiting-almost-every-antivirus-software)
dboptions should be an unsigned int.
Julius Plenz accidentally missing from NEWS.md.
Fail out if cl_engine_settings_apply() fails, because the only possible
cause of failure is a malloc issue.
Add clamd config option to force blocking clamd database reload to
conserve RAM. Users may set `ConcurrentDatabaseReload no` in their
clamd.conf config file to force a blocking reload.
The blocking mode will still perform the reload in a new thread, but
will first free the current database, wait for scans targeting that
database to complete, and then load the new database in the new thread
and wait (`pthread_join()`) on that thread. Once loaded, any pending
scans will continue. This is effectively the same behavior as how
clamd reloads worked before the multi-threaded database reload feature
was added.
Offload the DB load to a separate thread and only replace the current
engine instance afterwards.
While reload is pending:
- existing scan requests use the old db (this is unchanged)
- new scan requests are honored instead of blocked and they also use
the old db (this is new)
After the reload is complete:
- existing scan requests use the old db (this is unchanged)
- new scan requests use the new db (this is unchanged)
The existing engine is refcounted so it'll be eventually freed when no
longer in use.
Reload requests while reload is pending are silently ignored (i.e. It
never forks more than a single reload thread).
Patch courtesy of Alberto Wu. We would also like to thank Julius Plenz
for original work on this issue, as well as Alexander Sulfrian,
Arjen de Korte, David Heidelberg, and Ged Haywood for their work
updating and testing these patches.
PDFs may contain Javascript actions in objects. Those may actually be
indirect references to other objects where the Javascript resides rather
than storing the Javascript in the current object. The thing is,
sometimes the indirect object is empty (no actual script), in which case
the PDF may not have any active content.
This commit changes the Javascript detection logic so it only records
the stats/JSON after detecting the "Javascript" and "JS" tags in the
object (indirect or direct) where the Javascript is supposed to reside.
This moves the logic from an action callback when the object dictionary
key "Javascript" is detected over into the object extraction logic,
after all of the objects have been parsed.
Reviewing Coverity bug reports we found that the return value to this
filter_search call was effectively being ignored, causing no filtering
to occur. Fixing this issue resulted in a unit test that uses the
following match list regex to fail when searching for `ebay.com`.:
.+\\.paypal\\.(com|de|fr|it)([/?].*)?:.+\\.ebay\\.(at|be|ca|ch|co\\.uk|de|es|fr|ie|in|it|nl|ph|pl|com(\\.(au|cn|hk|my|sg))?)/
After investigating further, this is because the regex_list_add_pattern
call, which parses the regex for suffixes and attempts to add these to
the filter, can't handle the `com(\\.(au|cn|hk|my|sg))?` portion of
the regex. As a result, it only adds `ebay.at`, `ebay.be`, `ebay.ca`, up
through `ebay.pl` into the filter). With the code returning if no filter match
is found, the `ebay.com` suffix not existing in the filter causes incoming URLs
to be treated as if there are no corresponding regexes for ebay.com, which results
in no regex rules being evaluated against it.
We should get the regex parsing code working (and ensure it handles any
other complex cases in daily.cdb) before re-enabling this code. The code
has had no effect for 12+ years at this point, though, so it's probably
safe to wait a bit longer without it.
I had some time over the weekend and implemented fixes for warnings
in clamdtop/clamdtop.c:
- 279008 - In make_connection_real: Code can never be reached because
of a logical contradiction (CWE-561). host and port are freed and set
to NULL before they are used to set conn->remote. The fix is to
cleanup host and port after conn->remote is set.
- 147369 - In get_ip: Code can never be reached because of a logical
contradiction (CWE-561). Removed an unnecessary return statement that
could never be reached.
- 147624 - In get_port: Leak of memory or pointers to system resources
(CWE-404). dupip wasn’t being freed in the case where no port could
be found.
- 279007 - In make_connection_real: Pointer is checked against null
but then dereferenced anyway (CWE-476). This function checked soname
for NULL but wouldn’t fail in this case, and subsequent code would
dereference it. The NULL check could just be removed, though, because
the only calling function of this static function ensured that soname
is not NULL.
- 147316 - In cleanup: Value returned from a library function is not
checked for errors before being used. This value may indicate an error
condition. (CWE-252). The code ignores the value of send when exiting
because at that point it doesn’t really matter if the send succeeds.
The fix is to cast this function call to void to explicitly ignore
the return value.
- 147318 - In recv_line: Value returned from a library function is not
checked for errors before being used. This value may indicate an error
condition. (CWE-252) The code ignores the value of send when exiting
because at that point it doesn’t really matter if the send succeeds.
The fix is to cast this function call to void to explicitly ignore
the return value.
Fixed the following Coverity issues:
- 225236 - In cli_egg_extract_file: Dereference of an explicit
null value (CWE-476). The first fail case checked handle for
NULL and then dereferenced it in the done block
- 225209 - In executeIfNewVersion: Leak of memory or pointers
to system resources (CWE-404). modifiedCommand was defined
twice, with the inner instance being assigned to and the
outer instance being freed
- 225201 - In regex_list_match: Code can never be reached
because of a logical contradiction (CWE-561). The code had
logic off to the side that may have been missed:
filter_search_rc = filter_search(&matcher->filter, (const unsigned char *)bufrev, buffer_len) != -1;
if (filter_search_rc == -1) {
- 225198 - In phishingCheck: Leak of memory or pointers to
system resources (CWE-404). A fail case caused by malloc
failing would leak previously allocated memory.
- 225197 - In updatecustomdb: A pointer to freed memory
is dereferenced, used as a function argument, or otherwise
used (CWE-416). In a fail case, a pointer was freed and
then used in a debug print statement
- 225190 - In updatedb: A pointer to freed memory is
dereferenced, used as a function argument, or otherwise used
(CWE-416). In a fail case, a pointer was freed and then used
in a debug print statement
- 225195 - In cli_egg_open: The sizeof operator is used on a
wrong argument that incidentally has the same size (CWE-467).
sizeof(char **) was being used instead of sizeof(char *)
- 225193 - In egg_parse_comment_header: Code can never be
reached because of a logical contradiction (CWE-561).
A cleanup case for variable comment was unnecessary, and
to fix comment was removed entirely.
- 225147 - In get_server_node: Code can never be reached
because of a logical contradiction (CWE-561). A cleanup
case for variable url was unnecessary
- 225168 - In download_complete_callback: Missing break
statement between cases in switch statement (CWE-484).
In the case where forking failed, freshclam would check
the database without forking but then continue on to
execute the code intended to be done in the child process
because of a missing break statement
- 225152 - In cli_egg_lzma_decompress: Use of an
uninitialized variable (CWE-457). Certain fail cases
would call cli_LzmaShutdown on an uninitialized stream.
Now it’s only called after initialization occurs.
Looking through the list of issues, I spotted some easy ones and submitted
some fixes:
- 225229 - In cli_rarload: Leak of memory or pointers to system resources.
If finding the necessary libunrar functions fails (should be rare),we now
dlclose libunrar.
225224 - In main (freshclam.c): A copied piece of code is inconsistent with
the original (CWE-398). A minor copy-paste error was present, and optOutList
could be cleaned up in one of the failure edge cases.
225228 - In decodecdb: Out-of-bounds access to a buffer (CWE-119). Off by one
error when tokenizing certain CDB sig fields for printing with sigtool. Ex:
$ cat test.cdb
a:CL_TYPE_7Z:1-2-3:/.*/:1-2-3:1-2-3:0:1-2-3::
$ cat test.cdb | ../installed/bin/sigtool --decode
VIRUS NAME: a
CONTAINER TYPE: CL_TYPE_7Z
CONTAINER SIZE: WITHIN RANGE 1 to 2
FILENAME REGEX: /.*/
COMPRESSED FILESIZE: WITHIN RANGE 1 to 2
UNCOMPRESSED FILESIZE: WITHIN RANGE 1 to 2
ENCRYPTION: NO
FILE POSITION: =================================================================
==17245==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffe3136d10 at pc 0x7f0f31c3f414 bp 0x7fffe3136c70 sp 0x7fffe3136c60
WRITE of size 8 at 0x7fffe3136d10 thread T0
#0 0x7f0f31c3f413 in cli_strtokenize ../../libclamav/str.c:524
#1 0x559e9797dc91 in decodecdb ../../sigtool/sigtool.c:2929
#2 0x559e9797ea66 in decodesig ../../sigtool/sigtool.c:3058
#3 0x559e9797f31e in decodesigs ../../sigtool/sigtool.c:3162
#4 0x559e97981fbc in main ../../sigtool/sigtool.c:3638
#5 0x7f0f3100fb96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
#6 0x559e9795a1d9 in _start (/home/zelda/workspace/clamav-devel/installed/bin/sigtool+0x381d9)
Address 0x7fffe3136d10 is located in stack of thread T0 at offset 48 in frame
#0 0x559e9797d113 in decodecdb ../../sigtool/sigtool.c:2840
This frame has 1 object(s):
[32, 48) 'range' <== Memory access at offset 48 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
(longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow ../../libclamav/str.c:524 in cli_strtokenize
- 225223 - In cli_egg_deflate_decompress: Reads an uninitialized pointer or
its target (CWE-457). Certain fail cases would call inflateEnd on an
uninitialized stream. Now it’s only called after initialization occurs.
- 225220 - In buildcld: Use of an uninitialized variable (CWE-457). Certain
fail cases would result in oldDir being used before initialization. It now
gets zeroed before the first fail case.
- 225219 - In cli_egg_open: Leak of memory or pointers to system resources
(CWE-404). If certain realloc’s failed, several structures would not be cleaned up
- 225218 - In cli_scanhwpml: Code block is unreachable because of the syntactic
structure of the code (CWE-561). With certain macros set, there could be two
consecutive return statements.
Micah Snyder