[PATCH] Samba VirusFilter (version 12)

Jeremy Allison jra at samba.org
Sat Jan 13 00:41:22 UTC 2018


On Mon, Jan 08, 2018 at 07:14:53PM -0700, Trever L. Adams wrote:
> On 01/08/2018 06:19 PM, Jeremy Allison wrote:
> 
>     On Thu, Jan 04, 2018 at 04:33:56PM -0700, Trever L. Adams wrote:
> 
> 
>         I appreciate the help. I didn't think you were complaining. Hopefully my
>         answer gave understanding while correcting the flaw your question made
>         obvious. I am glad you asked. I found a similar problem in the RENAME
>         action code path. I have fixed that as well. I figure where you will
>         soon be providing me with additional fixes, flooding the list with an
>         interim fix would not be productive.
> 
>         https://github.com/treveradams/samba/tree/testing
> 
>         The last 5 commits in that tree are current (always will be until this
>         is finished as I use that as the way to propagate the changes to the
>         server(s) I use for testing.
> 
>     So I should only be looking at the last 5 commits in that tree, yes ?
> 
>     I ask as that tree still contains commits like:
> 
>     commit 41a43b513f944dddfbedf376c293e5d5e40de76d
>     Author: Trever L. Adams <trever.adams at gmail.com>
>     Date:   Tue Oct 18 13:32:53 2016 -0600
> 
>         Samba-VirusFilter: add write_data_timeout and write_data_iov_timeout.
> 
>         This patch adds those functions to sys_rw_data.[ch].
> 
>     which have been changed by the rewrite that Ralph did.
> 
>     What code tree do you want me to work on ? The patchset
>     that Ralph posted (which is pretty close to what we need)
>     or a https://github.com/treveradams/samba/tree/testing tree ?
> 
>     Jeremy.
> 
> 
> It appears that you have some remnants of the old testing tree. I do force
> updates to it. I know this isn't normal, but where it is desired to keep this
> squashed that is what I do.
> 
> Please, git reset --hard 4b1b5b141d3a46847eeec169a08516b65ab27255 then git
> pull.
> 
> This should give you 01d803bb190f86c078b1c49d1282c78abda3f856 as the last
> commit.
> 
> This should have all of the work you, Ralph, Jim and I have done over the last
> year+. And yes, it should be the last five (memcache, common, sophos, fsav,
> clamav in that order).
> 
> Sorry for the confusion.

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.



More information about the samba-technical mailing list