Allowing the service to shutdown instead of ignoring SIGTERM and waiting for 1m30s, which is extremely irritating and blocking the shutdown of the machine
We add the _OR_GOTO_DONE suffix to the macros that go to done if the
allocation fails. This makes it obvious what is different about the
macro versus the equivalent function, and that error handling is
built-in.
Renamed the cli_strdup to safer_strdup to make it obvious that it exists
because it is safer than regular strdup. Regular strdup doesn't have the
NULL check before trying to dup, and so may result in a NULL-deref
crash.
Also remove unused STRDUP (_OR_GOTO_DONE) macro, since the one with the
NULL-check is preferred.
The cli_max_malloc, cli_max_calloc, and cli_max_realloc functions
provide a way to protect against allocating too much memory
when the size of the allocation is derived from the untrusted input.
Specifically, we worry about values in the file being scanned being
manipulated to exhaust the RAM and crash the application.
There is no need to check the limits if the size of the allocation
is fixed, or if the size of the allocation is necessary for signature
loading, or the general operation of the applications.
E.g. checking the max-allocation limit for the size of a hash, or
for the size of the scan recursion stack, is a complete waste of
time.
Although we significantly increased the max-allocation limit in
a recent release, it is best not to check an allocation if the
allocation will be safe. It would be a waste of time.
I am also hopeful that if we can reduce the number allocations
that require a limit-check to those that require it for the safe
scan of a file, then eventually we can store the limit in the scan-
context, and make it configurable.
We have some special functions to wrap malloc, calloc, and realloc to
make sure we don't allocate more than some limit, similar to the
max-filesize and max-scansize limits. Our wrappers are really only
needed when allocating memory for scans based on untrusted user input,
where a scan file could have bytes that claim you need to allocate
some ridiculous amount of memory. Right now they're named:
- cli_malloc
- cli_calloc
- cli_realloc
- cli_realloc2
... and these names do not convey their purpose
This commit renames them to:
- cli_max_malloc
- cli_max_calloc
- cli_max_realloc
- cli_max_realloc2
The realloc ones also have an additional feature in that they will not
free your pointer if you try to realloc to 0 bytes. Freeing the memory
is undefined by the C spec, and only done with some realloc
implementations, so this stabilizes on the behavior of not doing that,
which should prevent accidental double-free's.
So for the case where you may want to realloc and do not need to have a
maximum, this commit adds the following functions:
- cli_safer_realloc
- cli_safer_realloc2
These are used for the MPOOL_REALLOC and MPOOL_REALLOC2 macros when
MPOOL is disabled (e.g. because mmap-support is not found), so as to
match the behavior in the mpool_realloc/2 functions that do not make use
of the allocation-limit.
There are a large number of allocations for fix sized buffers using the
`cli_malloc` and `cli_calloc` calls that check if the requested size is
larger than our allocation threshold for allocations based on untrusted
input. These allocations will *always* be higher than the threshold, so
the extra stack frame and check for these calls is a waste of CPU.
This commit replaces needless calls with A -> B:
- cli_malloc -> malloc
- cli_calloc -> calloc
- CLI_MALLOC -> MALLOC
- CLI_CALLOC -> CALLOC
I also noticed that our MPOOL_MALLOC / MPOOL_CALLOC are not limited by
the max-allocation threshold, when MMAP is found/enabled. But the
alternative was set to cli_malloc / cli_calloc when disabled. I changed
those as well.
I didn't change the cli_realloc/2 calls because our version of realloc
not only implements a threshold but also stabilizes the undefined
behavior in realloc to protect against accidental double-free's.
It may be worth implementing a cli_realloc that doesn't have the
threshold built-in, however, so as to allow reallocaitons for things
like buffers for loading signatures, which aren't subject to the same
concern as allocations for scanning possible malware.
There was one case in mbox.c where I changed MALLOC -> CLI_MALLOC,
because it appears to be allocating based on untrusted input.
Patch provides more insight into error conditions which may arise
when adding a directory to the watch hierarchy. If a specific file
caused the issue, the filename is provided to help users with any
troubleshooting needed.
The change to define _GNU_SOURCE means that the compiler knows that
syscall returns a long, not an int -- so then it warns about the wrong
format characters for printing that value.
Prototypes (or the declarations themselves, if there is no
corresponding prototype) for functions that take no arguments are
required by the C standard to specify (void) as their argument list;
for example,
regex_pcre.h:79:1: error: function declaration isn't a prototype
[-Werror=strict-prototypes]
79 | cl_error_t cli_pcre_init_internal();
Future versions of clang may become strict about this, and there's no
harm in conforming to the standard right now, so we fix all such
instances in this commit.
On Linux, thpool.c uses syscall() from unistd.h, but that function is
not defined without _GNU_SOURCE:
c-thread-pool/thpool.c: In function 'jobqueue_pull':
c-thread-pool/thpool.c:474:105: error: implicit declaration of function
'syscall' [-Werror=implicit-function-declaration]
In general that's not great, because it hinders some compiler diagnostics,
but it will also cause problems down the road if (for example) clang-16
decides to enable -Werror=implicit-function-declaration by default.
This commit changes the _POSIX_C_SOURCE definition at the top of
thpool.c to _GNU_SOURCE, as in the syscall(2) man page.
A race condition existed where clamonacc would call logg and attempt to
write to a logfile either before the logg interface had been initialized
or after it had been cleaned up.
This happens due to logg calls at cleanup during asynchronous thread
shutdowns, and during startup when watching directories with ongoing
event triggers.
This resulted in new files with garbage-filled names being created and
written to under the initial process' runtime path.
Changes:
Move logg setup to start of clamonacc.c main()
Change all raceable calls to logg to mprintf instead
* clamonacc: fix unused variable compile-time warning
Remove unused variable 'ret' from onas_queue.c and get rid of the
following compile-time warning:
~/clamav/clamonacc/scan/onas_queue.c: In function ‘onas_scan_queue_th’:
~/clamav/clamonacc/scan/onas_queue.c:161:9: warning: unused variable ‘ret’ [-Wunused-variable]
161 | int ret;
| ^~~
* libclamav: fix unused variable compile-time warning
Remove unused variable 'err' from libclamav/png.c, and get rid of
the following compile-time warning:
~/clamav/libclamav/png.c: In function ‘cli_parsepng’:
~/clamav/libclamav/png.c:101:9: warning: unused variable ‘err’ [-Wunused-variable]
101 | int err = Z_OK;
| ^~~
The clamonacc-ddd thread (clamonacc/inotif/inotif.c) has currently no
handling for inotify events where the passed watch descriptor is lower
than zero (e.g.: "-1"). Although not actually mentioned in the inotify
documentation, this case has actually been observed:
[...]
ClamInotif: inotify event wd:-1, mask:1073742080, cookie:0, length:16, child:<CHILD-PATH>
Clamonacc: onas_clamonacc_exit(), signal 11
Assumption: This probably occurs when a inotify event is generated for
the creation of a directory including subdirectories. The clamonacc-ddd
thread then discovers all children of the directory and is generating
inotify watch descriptors for all children. A subsequent inotify event
is generated for a subdirectory, but an inotify watch descriptor already
exists from the previous discovery.
In any case, this leads to an out-of-bounds array access on the internal
data structure "wdlt" of clamonacc while trying to look up the "path"
from there. This in turn can lead to a SIGSEGV of the clamonacc-ddd
thread.
The clamonacc-ddd thread (clamonacc/inotif/inotif.c) has currently no
explicit handling for the special inotify kernel events IN_UNMOUNT,
IN_Q_OVERFLOW and IN_IGNORED. It treats the inotify_event structs in
the same way as regular inotify events, although for those special
events the "len" and "name" fields a value of zero respectively NULL.
This can lead to a SIGSEGV of the clamonacc-ddd thread when calling
"strlen(name)" on a NULL value.
This commit adds just a quick'n'dirty workaround for the SIGSEGV and
some logging output. It DOES NOT fix the more overall issue, that a
IN_Q_OVERFLOW inotify event also leads to an out-of-sync situation
between the monitored filesystem/directory and the internal data
structures "ddd_ht" and "wdlt" of clamonacc.
The clamonacc-ddd thread (clamonacc/inotif/inotif.c) currently and in-
discriminately stops processing of inotify watch descriptors in the
function "onas_ddd_unwatch_hierarchy" upon any error returned from the
call to "inotify_rm_watch". This in turn also prevents further updates
and cleanup of the internal data structures "ddd_ht" and "wdlt", which
leads to a invalid value of the "path" variable.
This causes a leak of inotify watch descriptors in the case of e.g. a
move of a directory containing a hierarchy of watched subdirectories.
See the inotify watch descriptor ("wd:") values in the section "Example
and reproducer" below.
This commit adds a workaround for this issue by specifically ignoring
the "ENOENT" errno in case the call to "inotify_rm_watch" results in
a non-zero return code. This allows the cleanup to properly proceed
for all children.
Example and reproducer:
- Starting point:
Log:
ClamInotif: created watch descriptor (wd:1) for path: /clamonacc/monitored
[...]
ClamInotif: created watch descriptor (wd:129637) for path: /clamonacc/monitored/[...]
- Initial directory and subdirectory creation:
mkdir -p /clamonacc/monitored/.test_ff/{1,2,3,4,5}
Log:
ClamInotif: inotify event wd:1, mask:1073742080, cookie:0, length:16, child:.test_ff
ClamInotif: inotify event path: /clamonacc/monitored
ClamInotif: CREATE - adding /clamonacc/monitored/.test_ff to /clamonacc/monitored with wd:1
ClamInotif: created watch descriptor (wd:129638) for path: /clamonacc/monitored/.test_ff
ClamInotif: created watch descriptor (wd:129639) for path: /clamonacc/monitored/.test_ff/3
ClamInotif: created watch descriptor (wd:129640) for path: /clamonacc/monitored/.test_ff/4
ClamInotif: created watch descriptor (wd:129641) for path: /clamonacc/monitored/.test_ff/1
ClamInotif: created watch descriptor (wd:129642) for path: /clamonacc/monitored/.test_ff/5
ClamInotif: created watch descriptor (wd:129643) for path: /clamonacc/monitored/.test_ff/2
- Move / rename of the parent directory:
mv -i /clamonacc/monitored/.test_ff/ /clamonacc/monitored/test_ff
Log:
ClamInotif: inotify event wd:1, mask:1073741888, cookie:12094045, length:16, child:.test_ff
ClamInotif: inotify event path: /clamonacc/monitored
ClamInotif: MOVED_FROM - removing /clamonacc/monitored/.test_ff from /clamonacc/monitored with wd:1
ERROR: ClamInotif: error removing watch descriptor (wd:129638) for path: /clamonacc/monitored/.test_ff, No such file or directory
ClamInotif: inotify event wd:1, mask:1073741952, cookie:12094045, length:16, child:test_ff
ClamInotif: inotify event path: /clamonacc/monitored
ClamInotif: MOVED_TO - adding /clamonacc/monitored/test_ff to /clamonacc/monitored with wd:1
ClamInotif: created watch descriptor (wd:129644) for path: /clamonacc/monitored/test_ff
ClamInotif: created watch descriptor (wd:129639) for path: /clamonacc/monitored/test_ff/3
ClamInotif: created watch descriptor (wd:129640) for path: /clamonacc/monitored/test_ff/4
ClamInotif: created watch descriptor (wd:129641) for path: /clamonacc/monitored/test_ff/1
ClamInotif: created watch descriptor (wd:129642) for path: /clamonacc/monitored/test_ff/5
ClamInotif: created watch descriptor (wd:129643) for path: /clamonacc/monitored/test_ff/2
- Removal of the parent directory unter the new name:
rm -r /clamonacc/monitored/test_ff
Log:
ClamInotif: inotify event wd:129644, mask:1073742336, cookie:0, length:16, child:3
ClamInotif: inotify event path: /clamonacc/monitored/test_ff
ClamInotif: DELETE - removing /clamonacc/monitored/test_ff/3 from /clamonacc/monitored/test_ff with wd:129644
ClamInotif: removed watch descriptor (wd:129639) for path: /clamonacc/monitored/test_ff/3
ClamInotif: inotify event wd:129644, mask:1073742336, cookie:0, length:16, child:4
ClamInotif: inotify event path: /clamonacc/monitored/test_ff
ClamInotif: DELETE - removing /clamonacc/monitored/test_ff/4 from /clamonacc/monitored/test_ff with wd:129644
ClamInotif: removed watch descriptor (wd:129640) for path: /clamonacc/monitored/test_ff/4
ClamInotif: inotify event wd:129644, mask:1073742336, cookie:0, length:16, child:1
ClamInotif: inotify event path: /clamonacc/monitored/test_ff
ClamInotif: DELETE - removing /clamonacc/monitored/test_ff/1 from /clamonacc/monitored/test_ff with wd:129644
ClamInotif: removed watch descriptor (wd:129641) for path: /clamonacc/monitored/test_ff/1
ClamInotif: inotify event wd:129644, mask:1073742336, cookie:0, length:16, child:5
ClamInotif: inotify event path: /clamonacc/monitored/test_ff
ClamInotif: DELETE - removing /clamonacc/monitored/test_ff/5 from /clamonacc/monitored/test_ff with wd:129644
ClamInotif: removed watch descriptor (wd:129642) for path: /clamonacc/monitored/test_ff/5
ClamInotif: inotify event wd:129644, mask:1073742336, cookie:0, length:16, child:2
ClamInotif: inotify event path: /clamonacc/monitored/test_ff
ClamInotif: DELETE - removing /clamonacc/monitored/test_ff/2 from /clamonacc/monitored/test_ff with wd:129644
ClamInotif: removed watch descriptor (wd:129643) for path: /clamonacc/monitored/test_ff/2
ClamInotif: inotify event wd:1, mask:1073742336, cookie:0, length:16, child:test_ff
ClamInotif: inotify event path: /clamonacc/monitored
ClamInotif: DELETE - removing /clamonacc/monitored/test_ff from /clamonacc/monitored with wd:1
ClamInotif: removed watch descriptor (wd:129644) for path: /clamonacc/monitored/test_ff
This is also not strictly necessary, but i found it rather helpful to
be able to identify and distinguish the individual clamonacc threads in
the usual Linux system tools ("ps", "top", "gdb", etc.).
Setting the thread name is already done for the threads started by the
bundled c-thread-pool library ("thread-pool-N"). Only the clamonacc
threads all identify themselfes as "clamonacc".
This commit now sets the following - shortened (due to the 16 character
limit) - thread names:
- "clamonacc-ddd" for the ddd/inotify thread.
- "clamonacc-sq" for the scan queue thread.
- the main clamonacc thread currently keeps the name "clamonacc" in-
herited from the main process.
This is not strictly necessary, but i found it rather helpful to know
what is going on during the debugging process and while trying to under-
stand how clamonacc actually works.
Explicit setting of pthread_sigmask in the individual threads
should not be necessary at all(?), since it is already done in
the main process/thread and should be inherited from there. But
since the sigfillset/sigdelset lines are already there, at least
they should be consistent with regard to the undefined behaviour
caused by ignoring SIGFPE, SIGILL, SIGSEGV, or SIGBUS signals.
* Added loglevel parameter to logg()
* Fix logg and mprintf internals with new loglevels
* Update all logg calls to set loglevel
* Update all mprintf calls to set loglevel
* Fix hidden logg calls
* Executed clam-format
To build with code signing, the macOS build must have:
-G Xcode \
-D CLAMAV_SIGN_FILE=ON \
-D CODE_SIGN_IDENTITY="...your codesign ID..." \
-D DEVELOPMENT_TEAM_ID="...your team ID..." \
You can find the codesign ID using:
/usr/bin/env xcrun security find-identity -v -p codesigning
The team ID should also be listed in the identity description.
Also I changed the package name for APPLE to be "clamav" so it doesn't
put "ClamAV <version>" in the PKG PackageInfo like this:
com.cisco.ClamAV 0.104.0.libraries
Instead, it should just be something like:
com.cisco.clamav.libraries
Version is a separate field in that file and shouldn't be in the name.
Add the process memory scanning feature from ClamWin's ClamScan.
This commit extends that feature to make it available in ClamDScan
as well.
This adds three new options to ClamScan and ClamDScan on Windows:
* --memory
* --kill
* --unload
--allmatch and --stream are available for ClamDScan.
To reduce code duplication, this refactors clamd related code
used in both scanmem.c and proto.c into clamdcom.
Moved send_fdpass(), send_stream(), chkpath(), dconnect(), and
dsresult(); as well as some type definitions.
Special thanks to Gianluigi Tiesi for allowing us to integrate the
Windows process memory scanning feature from ClamWin into the ClamAV.
CMake/CPack is already used to build:
- TGZ source tarball
- WiX-based installer (Windows)
- ZIP install packages (Windows)
This commit adds support for building:
- macOS PKG installer
- DEB package
- RPM package
This should also enable building FreeBSD packages, but while I was able
to build all of the static dependencies using Mussels, CMake/CPack 3.20
doesn't appear to have the the FreeBSD generator despite being in the
documentation.
The package names are will be in this format:
clamav-<version><suffix>.<os>.<arch>.<extension>
This includes changing the Windows .zip and .msi installer names.
E.g.:
- clamav-0.104.0-rc.macos.x86_64.pkg
- clamav-0.104.0-rc.win.win32.msi
- clamav-0.104.0-rc.win.win32.zip
- clamav-0.104.0-rc.win.x64.msi
- clamav-0.104.0-rc.linux.x86_64.deb
- clamav-0.104.0-rc.linux.x86_64.rpm
Notes about building the packages:
I've only tested this with building ClamAV using static dependencies that
I build using the clamav_deps "host-static" recipes from the "clamav"
Mussels cookbook. Eg:
msl build clamav_deps -t host-static
Here's an example configuration to build clam in this way, installing to
/usr/local/clamav:
```sh
cmake .. \
-D CMAKE_FIND_PACKAGE_PREFER_CONFIG=TRUE \
-D CMAKE_PREFIX_PATH=$HOME/.mussels/install/host-static \
-D CMAKE_INSTALL_PREFIX="/usr/local/clamav" \
-D CMAKE_MODULE_PATH=$HOME/.mussels/install/host-static/lib/cmake \
-D CMAKE_BUILD_TYPE=RelWithDebInfo \
-D ENABLE_EXAMPLES=OFF \
-D JSONC_INCLUDE_DIR="$HOME/.mussels/install/host-static/include/json-c" \
-D JSONC_LIBRARY="$HOME/.mussels/install/host-static/lib/libjson-c.a" \
-D ENABLE_JSON_SHARED=OFF \
-D BZIP2_INCLUDE_DIR="$HOME/.mussels/install/host-static/include" \
-D BZIP2_LIBRARY_RELEASE="$HOME/.mussels/install/host-static/lib/libbz2_static.a" \
-D OPENSSL_ROOT_DIR="$HOME/.mussels/install/host-static" \
-D OPENSSL_INCLUDE_DIR="$HOME/.mussels/install/host-static/include" \
-D OPENSSL_CRYPTO_LIBRARY="$HOME/.mussels/install/host-static/lib/libcrypto.a" \
-D OPENSSL_SSL_LIBRARY="$HOME/.mussels/install/host-static/lib/libssl.a" \
-D LIBXML2_INCLUDE_DIR="$HOME/.mussels/install/host-static/include/libxml2" \
-D LIBXML2_LIBRARY="$HOME/.mussels/install/host-static/lib/libxml2.a" \
-D PCRE2_INCLUDE_DIR="$HOME/.mussels/install/host-static/include" \
-D PCRE2_LIBRARY="$HOME/.mussels/install/host-static/lib/libpcre2-8.a" \
-D CURSES_INCLUDE_DIR="$HOME/.mussels/install/host-static/include" \
-D CURSES_LIBRARY="$HOME/.mussels/install/host-static/lib/libncurses.a" \
-D ZLIB_INCLUDE_DIR="$HOME/.mussels/install/host-static/include" \
-D ZLIB_LIBRARY="$HOME/.mussels/install/host-static/lib/libz.a" \
-D LIBCHECK_INCLUDE_DIR="$HOME/.mussels/install/host-static/include" \
-D LIBCHECK_LIBRARY="$HOME/.mussels/install/host-static/lib/libcheck.a"
```
Set CPACK_PACKAGING_INSTALL_PREFIX to customize the resulting package's
install location. This can be different than the install prefix. E.g.:
```sh
-D CMAKE_INSTALL_PREFIX="/usr/local/clamav" \
-D CPACK_PACKAGING_INSTALL_PREFIX="/usr/local/clamav" \
```
Then `make` and then one of these, depending on the platform:
```sh
cpack # macOS: productbuild is default
cpack -G DEB # Debian-based
cpack -G RPM # RPM-based
```
On macOS you'll need to `pip3 install markdown` so that the NEWS.md file can
be converted to html so it will render in the installer.
On RPM-based systems, you'll need rpmbuild (install rpm-build)
This commit also fixes an issue where the html manual (if present) was
not correctly added to the Windows (or now other) install packages.
Fix num to hex function for Windows installer guid
Fix win32 cpack build
Fix macOS cpack build
This fixes a fatal issue that would occur when unable to queue events due to
clamonacc improperly using all available fds.
It also fixes the core fd socket leak issue at the heart of the segfault by
properly cleaning up after a failed curl connection.
Lastly, worst case recovery code now allows more time for consumer queue
to catchup. It accomplishes this by increasing wait time and adding
retry logic.
More info: https://github.com/Cisco-Talos/clamav/issues/184
CMake is now required to build.
The built-in LLVM is no longer available.
Also removed support for libltdl calls, which is not used in the CMake
builds, was only used when building with Autotools.
TODO: Fix CMake LLVM support & update to work with modern versions.
The named "shared" is confusing, especially now that these features are
built as a static library instead of being directly compiled into the
various applications.
- 192959 Resource leak - In cli_bcomp_compare_check: Leak of
memory or pointers to system resources. Several fail cases
could lead to `buffer` or `tmp_buffer` being leaked
- 192934 Resource leak - In cli_bcomp_normalize_buffer: Leak of
memory or pointers to system resources. `hex_buffer` leaked
under certain conditions
- 185977 Resource leak - In ole2_process_property: Leak of memory
or pointers to system resources. A fail case could lead to
`outstr` and `outstr2` being leaked
- 185941 Resource leak - In header_cb (clamsubmit): Leak of
memory or pointers to system resources. A fail case could lead
to `mem` being leaked
- 185925 Resource leak - In load_oneyara: Leak of memory or
pointers to system resources. Several fail cases could lead
to `newident` being leaked
- 185918 Resource leak - In parsehwp3_docsummary: Leak of memory
or pointers to system resources. Not actually a leak, but
caused by checking for a condition that can’t occur.
- 185915 Resource leak - In parsehwp3_docinfo: Leak of memory or
pointers to system resources. Not actually a leak, but caused
by checking for a condition that can’t occur.
- 147644 Resource leak - In tcpserver: Leak of memory or pointers
to system resources. A fail case could lead to `info` being leaked
- 147642 Resource leak - In onas_ht_add_hierarchy: Leak of memory
or pointers to system resources. Several fail cases could lead
to `hnode` or `elem` memory leaks
Coverity warnings:
- 293628 Uninitialized pointer read - In reload_db: Reads
an uninitialized pointer or its target. A fail case
could lead to `rldata` being used before initialization
- 293627 Uninitialized pointer read - In reload_th: Reads
an uninitialized pointer or its target. A fail case could
lead to `engine` being used before initialization
- 265483 Uninitialized pointer write - In parseEmailFile:
Write to target of an uninitialized pointer. A fail case
could lead `ret` to be dereferenced and written to
- 265482 Resource leak - In parseEmailFile: Leak of memory
or pointers to system resources. A fail case could lead
to `head` being leaked
- 225221 Resource leak - In onas_get_opt_list: Leak of memory
or pointers to system resources. A fail case could lead to
`opt_list` being leaked
- 225181 Resource leak - In onas_ht_rm_hierarchy: Leak of
memory or pointers to system resources. A fail case could
lead to `prntname` being leaked
- 193874 Resource leak - In cli_genfname: Leak of memory
or pointers to system resources. A fail case could lead
to `sanitized_prefix` being leaked
- 225196 Resource leak - In onas_fan_eloop: Leak of memory
or pointers to system resources. A fail cases could lead
to `event_data` being leaked
Also, I added some unresolved comments regarding clamonacc
functionality, and added a version compatibility check that
is shown in the example code in the `fanotify` man page
