[Clamav-devel] Patch for OnAccessIncludePath traversal across file systems

[Micah Snyder (micasnyd)]

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 at lists.clamav.net on behalf of arthurramsey19 at 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 at 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