[PATCH] Samba VirusFilter (version 12)

Trever L. Adams trever at middleearth.sapphiresunday.org
Wed Jan 24 09:39:39 UTC 2018


On 01/23/2018 04:59 PM, ronnie sahlberg wrote:
> 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.
The do nothing action blocks all access, but does not rename/move. It
also doesn't mess with ACEs. I do not believe it would be a good idea to
ever allow SYSTEM users to run such a file. I believe (I may be showing
ignorance here) that all virus scanners run as SYSTEM users or as the
user in question, for accessing files.

If you leave the defaults (keep name/keep directory) in quarantine, it
will move the file to a quarantine directory that includes all the
original path information and, assuming it doesn't move into a different
FS and then maybe then, all of the permissions. This quarantine
directory could be a share that only administrators can access or what
not and you can do all of what you are saying without adding a lot of
complexity to the module that may make it difficult to keep it properly
written (i.e. create a security risk).

So, rename keeps it in place but adds extra pieces to a name. This makes
it obvious what is going on. These renamed files are automatically
blocked from being accessed by ALL users. (This is active regardless of
which action you configure, so if you have the defaults, do NOT create a
file virusfilter.XXXXXXXX.infected, because you won't be able to access it.)

Quarantine does similar but moves things to a different path, which
hopefully is not accessible to the user (see defaults and the warning
about changing defaults). Such a path can be exported as a share with
restricted access and you can then do what you want and move things back
if a software package can clean it at a later time.

Delete removes the file.

Do nothing blocks access but does not delete/move/rename.

See the manpage for action scripts that can be done on finding a problem
and the notify script example that will email the user and administrator
information on the infection. This could be used to trigger a system to
rescan and attempt a cleaning, if cleaned, move it back.
>
>
> 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".
>
This one would be interesting. I do not know enough about the Samba
internals to know how to do this. It would be a good thing to do for all
actions, if configured to do so.

I am looking at the source for another vfs module now. It appears that I
could set the offline flag on detection of infection. But, it would be
permanently set. Any system that would fix the infection would have to
unset it.

If this is something that people believe should be done (setting the
offline flag for any action, or at least rename/do nothing), I will do
it. It looks like it would be just a few lines of code and straight forward.

Trever

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 886 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180124/b2ad8d96/signature.sig>


More information about the samba-technical mailing list