[PATCH] Samba VirusFilter (version 12)

[Jeremy Allison]

On Fri, Jan 12, 2018 at 06:45:42PM -0700, Trever L. Adams wrote:
> On 01/12/2018 05:41 PM, Jeremy Allison wrote:
> >
> > OK, I'm working on getting this ready for integration (I have a
> > reasonable patch to the code) - however I came across this (this
> > is after my fixes) inside virusfilter_do_infected_file_action():
> >
> >                 /*
> >                  * FIXME !!!!! mkstemp()
> >                  * Is a direct call onto the underlying
> >                  * filesystem. This will fail if there are
> >                  * any modules underneath us that transform
> >                  * paths, or we're redirecting to a distributed
> >                  * filesystem like gluster or ceph. JRA.
> >                  *
> >                  * Currently, using this call means this module
> >                  * is *NOT* stackable, and must be the bottom
> >                  * module in a stack that backs onto a directly
> >                  * attached filesystem.
> >                  */
> >                 become_root();
> >                 q_fd = mkstemp(q_filepath);
> >                 if (q_fd == -1) {
> >                         saved_errno = errno;
> >                 }
> >                 unbecome_root();
> >
> > If this module is meant to be stackable we can't use
> > calls like mkstemp(), as they directly access the underlying
> > filesystem.
> >
> > Now I know that the virus scanners can only be accessing
> > the real underlying filesystem (so this module currently
> > can't work over ceph, gluster or another distributed
> > filesystem), but there are modules that do filename transforms
> > and so if we want this to be a stackable module we must find
> > a portable way that goes through the VFS to do this.
> >
> > It's not terrible or a security hole, as all we're doing is
> > creating a temp file name inside a root-only writable directory
> > that we want to do the rename of a quarantine file to, so we
> > just need to find a portable way of generating a random
> > filename we want to move it to. We're not actually using
> > the returned fd here at all.
> >
> > Jeremy.
> >
> Opinion: vfs_streams_depot should be one of those that transforms the
> name since it changes the file name the system is accessing. This would
> fix the problem I mentioned earlier. It seems I had a patch for this,
> but cannot find it. By chance do you have a copy of such a thing as part
> of working with me? It seems like it modified the open function and
> added a flag and changed is_ntfs_stream_smb_fname to check for that
> flag. Or should it have something like catia_realpath(...) and
> virusfilter do a realpath call? (I am sorry, still new to the VFS
> interface and trying to figure everything out.
> 
> As for the distributed file system, if it is mounted as an actual fs by
> the kernel, it can be scanned. Do these modules do this? Or are they
> hidden from the kernel view some how?
> 
> Ok, a possible solution:
> 
> Rename action currently just blindly renames. It does no checks. This
> was by design. It may be incorrect, I wrote the code and stand open for
> criticism of it. I figured since the user can see the files and may
> notice the rename, things aren't all that bad.
> 
> Quarantine is not viewable by the user (not normally anyway). There
> should be some way of preserving multiple copies. For about two weeks
> now, I have been wondering if they should have a time stamp (i.e.
> 2018-01-12-18:29:57.1234) appended to the name instead of the mkstemp
> format. The problem is how do we check to see if the underlying name
> exists? Is such a step necessary? What resolution provides a reasonable
> enough granularity (i.e. seconds, milliseconds, etc. I figure at least
> seconds)?
> 
> Sorry that this FIXME was missed. As you can see I still have a few
> questions (including the one about vfs_streams_depot above).

As all we're doing is moving the file out of the users way
in quarantine, how about moving it into a subdirectory
named for the process id of the smbd ? As we know each process
id is unique whilst running then we can safely do an lstat
for the name before doing the rename.

I'll try coding this up, let me know if this works for you.