Mailing List Archive

Patch for OnAccessIncludePath traversal across file systems
Don’t expect this to get accepted but would like some feedback since this is my 1st time doing much of anything in C. I think this could be a useful feature but should probably be a non-default option.

This patch is against 1.101.4.

--- a/clamd/onaccess_hash.c
+++ b/clamd/onaccess_hash.c
@@ -33,6 +33,7 @@
#include <string.h>
#include <errno.h>
#include <stdbool.h>
+#include <mntent.h>

#include <sys/fanotify.h>

@@ -622,6 +623,22 @@ int onas_ht_add_hierarchy(struct onas_ht *ht, const char *pathname) {
if (!elem) return CL_EMEM;

if (onas_ht_insert(ht, elem)) return -1;
+
+ char buf[10240];
+ struct mntent ent;
+ struct mntent *mntent;
+ FILE *mountinfo;
+ mountinfo = setmntent("/proc/mounts", "r");
+ if (mountinfo == NULL) {
+ logg("!ScanOnAccess: setmntent failed\n");
+ return CL_EARG;
+ }
+ while ((mntent = getmntent_r(mountinfo, &ent, buf, sizeof(buf))) != NULL) {
+ if (strcmp(curr->fts_path, pathname) != 0 && strcmp(curr->fts_path, mntent->mnt_dir) == 0) {
+ onas_ht_add_hierarchy(ht, curr->fts_path);
+ }
+ }
+ endmntent(mountinfo);
}

_priv_fts_close(ftsp);

For more context seet: https://lists.gt.net/clamav/users/77314.
_______________________________________________

clamav-devel mailing list
clamav-devel@lists.clamav.net
https://lists.clamav.net/mailman/listinfo/clamav-devel

Please submit your patches to our Bugzilla: http://bugzilla.clamav.net

Help us build a comprehensive ClamAV guide:
https://github.com/vrtadmin/clamav-faq

http://www.clamav.net/contact.html#ml
Re: Patch for OnAccessIncludePath traversal across file systems [ In reply to ]
After more testing this seems better:

--- a/clamd/onaccess_hash.c 2019-10-10 19:19:06.000000000 -0500
+++ b/clamd/onaccess_hash.c 2019-10-10 19:14:23.000000000 -0500
@@ -33,6 +33,7 @@
#include <string.h>
#include <errno.h>
#include <stdbool.h>
+#include <mntent.h>

#include <sys/fanotify.h>

@@ -589,6 +590,22 @@

struct onas_hnode *hnode = NULL;

+ char buf[10240];
+ struct mntent ent;
+ struct mntent *mntent;
+ FILE *mountinfo;
+ mountinfo = setmntent("/proc/mounts", "r");
+ if (mountinfo == NULL) {
+ logg("!ScanOnAccess: setmntent failed\n");
+ return CL_EARG;
+ }
+ while ((mntent = getmntent_r(mountinfo, &ent, buf, sizeof(buf))) != NULL) {
+ if (strcmp(curr->fts_path, pathname) != 0 && strcmp(curr->fts_path, mntent->mnt_dir) == 0) {
+ onas_ht_add_hierarchy(ht, curr->fts_path);
+ }
+ }
+ endmntent(mountinfo);
+
/* May want to handle other options in the future. */
switch (curr->fts_info) {
case FTS_D:

_______________________________________________

clamav-devel mailing list
clamav-devel@lists.clamav.net
https://lists.clamav.net/mailman/listinfo/clamav-devel

Please submit your patches to our Bugzilla: http://bugzilla.clamav.net

Help us build a comprehensive ClamAV guide:
https://github.com/vrtadmin/clamav-faq

http://www.clamav.net/contact.html#ml
Re: Patch for OnAccessIncludePath traversal across file systems [ In reply to ]
Hi Arthur,

I think I understand what you're trying to solve but you should really get Mickey's feedback on the code, as she's the subject matter expert on ClamAV's on-access scanning. I will make a couple of recommendations...

I see your patch targets an earlier version of ClamAV. We don't add new features to older versions if we can help it, and the on-access code was migrated out of clamd and into a new program called clamonacc. Specifically, the function you're modifying is now found under clamonacc/inotif/hash.c. Please fork & clone https://github.com/Cisco-Talos/clamav-devel to contribute code to the ClamAV project. The default branch will be set to dev/0.103, for all features going into 0.103.

Your patch creates a large buffer on the stack. Stack space can be quite limited. I would recommend allocating the buffer on the heap with malloc (and maybe realloc). I should also note that "buf" is an overly simple variable name. I would prefer something more descriptive. Inline comments, using the /* */ format are also appreciated. Having said that, I googled around a bit to try to figure out where your 10240 number comes from. It looks to me like maybe that was a typo from "1024", which is a number suggested here by some alpine developers who noted they needed a large buffer so that they don't receive an error or truncated entry (behavior varies between musl/glibc implementations). https://gitlab.alpinelinux.org/alpine/aports/issues/7093

If 1024 is correct, I would also recommend either adding a comment to explain the size and/or creating a preprocessor macro (#define) with a descriptive name. Something like:
#define MOUNT_ENTRY_BUFFER_MAX_SIZE 1024

Respectfully,
Micah


Micah Snyder
ClamAV Development
Talos
Cisco Systems, Inc.



?On 10/10/19, 8:28 PM, "clamav-devel on behalf of Arthur Ramsey" <clamav-devel-bounces@lists.clamav.net on behalf of arthurramsey19@gmail.com> wrote:

After more testing this seems better:

--- a/clamd/onaccess_hash.c 2019-10-10 19:19:06.000000000 -0500
+++ b/clamd/onaccess_hash.c 2019-10-10 19:14:23.000000000 -0500
@@ -33,6 +33,7 @@
#include <string.h>
#include <errno.h>
#include <stdbool.h>
+#include <mntent.h>

#include <sys/fanotify.h>

@@ -589,6 +590,22 @@

struct onas_hnode *hnode = NULL;

+ char buf[10240];
+ struct mntent ent;
+ struct mntent *mntent;
+ FILE *mountinfo;
+ mountinfo = setmntent("/proc/mounts", "r");
+ if (mountinfo == NULL) {
+ logg("!ScanOnAccess: setmntent failed\n");
+ return CL_EARG;
+ }
+ while ((mntent = getmntent_r(mountinfo, &ent, buf, sizeof(buf))) != NULL) {
+ if (strcmp(curr->fts_path, pathname) != 0 && strcmp(curr->fts_path, mntent->mnt_dir) == 0) {
+ onas_ht_add_hierarchy(ht, curr->fts_path);
+ }
+ }
+ endmntent(mountinfo);
+
/* May want to handle other options in the future. */
switch (curr->fts_info) {
case FTS_D:

_______________________________________________

clamav-devel mailing list
clamav-devel@lists.clamav.net
https://lists.clamav.net/mailman/listinfo/clamav-devel

Please submit your patches to our Bugzilla: http://bugzilla.clamav.net

Help us build a comprehensive ClamAV guide:
https://github.com/vrtadmin/clamav-faq

http://www.clamav.net/contact.html#ml


_______________________________________________

clamav-devel mailing list
clamav-devel@lists.clamav.net
https://lists.clamav.net/mailman/listinfo/clamav-devel

Please submit your patches to our Bugzilla: http://bugzilla.clamav.net

Help us build a comprehensive ClamAV guide:
https://github.com/vrtadmin/clamav-faq

http://www.clamav.net/contact.html#ml
Re: Patch for OnAccessIncludePath traversal across file systems [ In reply to ]
Fair feedback but see https://lists.gt.net/clamav/users/77314 <https://lists.gt.net/clamav/users/77314> for more info as to why this won’t work for files created on overlayfs in docker containers. The trailing 0 in the buffer size was a typo.

Thanks,
Arthur
_______________________________________________

clamav-devel mailing list
clamav-devel@lists.clamav.net
https://lists.clamav.net/mailman/listinfo/clamav-devel

Please submit your patches to our Bugzilla: http://bugzilla.clamav.net

Help us build a comprehensive ClamAV guide:
https://github.com/vrtadmin/clamav-faq

http://www.clamav.net/contact.html#ml
Re: Patch for OnAccessIncludePath traversal across file systems [ In reply to ]
See correction “won’t work with 0.102.0+”:

> On Oct 11, 2019, at 3:10 PM, Arthur Ramsey <arthurramsey19@gmail.com> wrote:
>
> Fair feedback but see https://lists.gt.net/clamav/users/77314 <https://lists.gt.net/clamav/users/77314> for more info as to why this won’t work with 0.102.0+ for files created on overlayfs in docker containers. The trailing 0 in the buffer size was a typo.
>
> Thanks,
> Arthur

_______________________________________________

clamav-devel mailing list
clamav-devel@lists.clamav.net
https://lists.clamav.net/mailman/listinfo/clamav-devel

Please submit your patches to our Github: https://github.com/Cisco-Talos/clamav-devel/pulls

Help us build a comprehensive ClamAV guide:
https://github.com/vrtadmin/clamav-faq

http://www.clamav.net/contact.html#ml