[PATCH] Samba VirusFilter (version 12)

[Trever L. Adams]

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).

Thank you.

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/20180112/82df38fa/signature.sig>