[PATCH] Samba VirusFilter (version 12)

ronnie sahlberg ronniesahlberg at gmail.com
Tue Jan 23 23:59:36 UTC 2018

On Sat, Jan 20, 2018 at 4:54 AM, Jeremy Allison via samba-technical
<samba-technical at lists.samba.org> wrote:
> 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.

Why do you need/want to move the file away to a different directory?
An alternative could be to instead modify the acl for the file to make it
not readable/not writeable for all users and leave it in place.
Maybe even an extra ACE that does alllow reading/writing the file but only for a
dedicated virus scanner user. Could be useful in case future the
current version of anti-virus
can only detect the virus but future updates of the anti-virus package
learns how to
remove/repair the file.

You could even modify the query data you return for the file to set
the offline bit
as another visual cue to the user that "the file is there but there is
something wrong with it and you can't read it".

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

More information about the samba-technical mailing list