Latest AV code

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

Latest AV code

Samba - samba-technical mailing list
On Thu, Dec 21, 2017 at 11:23:01PM +0100, Ralph Böhme wrote:
>
> The one I have is "[PATCHES] Samba-VirusFilter (version 6)". Is that the latest one? If
> not, can you please bounce me their latest mail to s-t so I can follow up on the
> list? Thanks!

Here's the latest version I have.
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
Hi folks,

On Thu, Dec 21, 2017 at 03:06:32PM -0800, Jeremy Allison wrote:
> On Thu, Dec 21, 2017 at 11:23:01PM +0100, Ralph Böhme wrote:
> >
> > The one I have is "[PATCHES] Samba-VirusFilter (version 6)". Is that the latest one? If
> > not, can you please bounce me their latest mail to s-t so I can follow up on the
> > list? Thanks!
>
> Here's the latest version I have.

Jeremy, attached is the rewritten version as discussed.

Trevor, Satoh, a few days ago Jeremy asked me to look into the latest version of
the Virusfilter patchset (probably version 10 as posted on
https://lists.samba.org/archive/samba-technical/2017-July/121573.html), asking
for my opinion on the code.

I haven't been involved in the previous review rounds and somehow felt that the
current implementation using #includes of some generic code into three VFS
modules looked awkward.

I got bored the other day on a train from Frankenberg to Goslar, so I took the
liberty to bend the code to my will. Attached is a rewrite that removes the
#include magic by splitting the modules into one frontend VFS moulde
vfs_virusfilter and three scanner backends. The backend is then chosen by a new
option "virusfilter:scanner=clamav|fsav|sophos".

Trevor and Satoh, can you take a look and tell us if you agree with the rewrite?
This is the general design change that Jeremy and I agreed upon to be necessary
for the module to go upstream.

I tried to be careful when shuffling around the options and removing all the
macro stuff, so hopefully everything still works, but given the size of the
change this needs some testing I guess. The attached patchset compiles, but
hasn't seen any testing...

The attach patchset keeps the FIXUP patches as seperate patches on top of the
individual original patches. This is just to make it easier to see what I'm
doing. If you're agree with the new design, just fold the fixups into the
preceding patch.

If yes, then I guess there's little left that hinders final review by me and
possibly Jeremy. There are still lots of README.Coding violations, I've already
fixed many of them in the rewrite, I'd appreciate if you could fix the
remaining. Most of the violations are function calls in conditionals:

if (somefunc() COMPARISON EXPECTED_RESULT) { ... }

Please rewrite as:

VARIABLE = somefunc();
if (VARIABLE COMPARISON EXPECTED_RESULT) { ... }

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

virusfilter-rewrite.patch (280K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On Fri, Dec 22, 2017 at 03:47:02PM +0100, Ralph Böhme via samba-technical wrote:

> Hi folks,
>
> On Thu, Dec 21, 2017 at 03:06:32PM -0800, Jeremy Allison wrote:
> > On Thu, Dec 21, 2017 at 11:23:01PM +0100, Ralph Böhme wrote:
> > >
> > > The one I have is "[PATCHES] Samba-VirusFilter (version 6)". Is that the latest one? If
> > > not, can you please bounce me their latest mail to s-t so I can follow up on the
> > > list? Thanks!
> >
> > Here's the latest version I have.
>
> Jeremy, attached is the rewritten version as discussed.
>
> Trevor, Satoh, a few days ago Jeremy asked me to look into the latest version of
> the Virusfilter patchset (probably version 10 as posted on
> https://lists.samba.org/archive/samba-technical/2017-July/121573.html), asking
> for my opinion on the code.
>
> I haven't been involved in the previous review rounds and somehow felt that the
> current implementation using #includes of some generic code into three VFS
> modules looked awkward.
>
> I got bored the other day on a train from Frankenberg to Goslar, so I took the
> liberty to bend the code to my will. Attached is a rewrite that removes the
> #include magic by splitting the modules into one frontend VFS moulde
> vfs_virusfilter and three scanner backends. The backend is then chosen by a new
> option "virusfilter:scanner=clamav|fsav|sophos".
>
> Trevor and Satoh, can you take a look and tell us if you agree with the rewrite?
> This is the general design change that Jeremy and I agreed upon to be necessary
> for the module to go upstream.
>
> I tried to be careful when shuffling around the options and removing all the
> macro stuff, so hopefully everything still works, but given the size of the
> change this needs some testing I guess. The attached patchset compiles, but
> hasn't seen any testing...
>
> The attach patchset keeps the FIXUP patches as seperate patches on top of the
> individual original patches. This is just to make it easier to see what I'm
> doing. If you're agree with the new design, just fold the fixups into the
> preceding patch.
>
> If yes, then I guess there's little left that hinders final review by me and
> possibly Jeremy. There are still lots of README.Coding violations, I've already
> fixed many of them in the rewrite, I'd appreciate if you could fix the
> remaining. Most of the violations are function calls in conditionals:
>
> if (somefunc() COMPARISON EXPECTED_RESULT) { ... }
>
> Please rewrite as:
>
> VARIABLE = somefunc();
> if (VARIABLE COMPARISON EXPECTED_RESULT) { ... }

Ralph, thanks SO MUCH for that refactoring. It addresses just
about all the concerns I had with the patchset, and makes the
logic a lot easier to understand, which was the main reason
I was sat on this thing for so long.

Trevor, Satoh, can you confirm that this reworked set still
works for what you need ? If so, then I will work with you
(and get final Ralph review) for the README.Coding violations
and we can (finally) get this upstream.

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On Fri, Dec 22, 2017 at 02:27:23PM -0700, Trever L. Adams wrote:

> On 12/22/2017 02:21 PM, Jeremy Allison wrote:
> > On Fri, Dec 22, 2017 at 03:47:02PM +0100, Ralph Böhme via samba-technical wrote:
> >> Hi folks,
> >>
> >> On Thu, Dec 21, 2017 at 03:06:32PM -0800, Jeremy Allison wrote:
> >>> On Thu, Dec 21, 2017 at 11:23:01PM +0100, Ralph Böhme wrote:
> >>>> The one I have is "[PATCHES] Samba-VirusFilter (version 6)". Is that the latest one? If
> >>>> not, can you please bounce me their latest mail to s-t so I can follow up on the
> >>>> list? Thanks!
> >>> Here's the latest version I have.
> >> Jeremy, attached is the rewritten version as discussed.
> >>
> >> Trevor, Satoh, a few days ago Jeremy asked me to look into the latest version of
> >> the Virusfilter patchset (probably version 10 as posted on
> >> https://lists.samba.org/archive/samba-technical/2017-July/121573.html), asking
> >> for my opinion on the code.
> >>
> >> I haven't been involved in the previous review rounds and somehow felt that the
> >> current implementation using #includes of some generic code into three VFS
> >> modules looked awkward.
> >>
> >> I got bored the other day on a train from Frankenberg to Goslar, so I took the
> >> liberty to bend the code to my will. Attached is a rewrite that removes the
> >> #include magic by splitting the modules into one frontend VFS moulde
> >> vfs_virusfilter and three scanner backends. The backend is then chosen by a new
> >> option "virusfilter:scanner=clamav|fsav|sophos".
> >>
> >> Trevor and Satoh, can you take a look and tell us if you agree with the rewrite?
> >> This is the general design change that Jeremy and I agreed upon to be necessary
> >> for the module to go upstream.
> >>
> >> I tried to be careful when shuffling around the options and removing all the
> >> macro stuff, so hopefully everything still works, but given the size of the
> >> change this needs some testing I guess. The attached patchset compiles, but
> >> hasn't seen any testing...
> >>
> >> The attach patchset keeps the FIXUP patches as seperate patches on top of the
> >> individual original patches. This is just to make it easier to see what I'm
> >> doing. If you're agree with the new design, just fold the fixups into the
> >> preceding patch.
> >>
> >> If yes, then I guess there's little left that hinders final review by me and
> >> possibly Jeremy. There are still lots of README.Coding violations, I've already
> >> fixed many of them in the rewrite, I'd appreciate if you could fix the
> >> remaining. Most of the violations are function calls in conditionals:
> >>
> >> if (somefunc() COMPARISON EXPECTED_RESULT) { ... }
> >>
> >> Please rewrite as:
> >>
> >> VARIABLE = somefunc();
> >> if (VARIABLE COMPARISON EXPECTED_RESULT) { ... }
> > Ralph, thanks SO MUCH for that refactoring. It addresses just
> > about all the concerns I had with the patchset, and makes the
> > logic a lot easier to understand, which was the main reason
> > I was sat on this thing for so long.
> >
> > Trevor, Satoh, can you confirm that this reworked set still
> > works for what you need ? If so, then I will work with you
> > (and get final Ralph review) for the README.Coding violations
> > and we can (finally) get this upstream.
> >
> > Cheers,
> >
> > Jeremy.
> >
> Sure. I am not sure how to do a git clone of something like:
>
> https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/av
>
>
> How would I do that so I can use it as a starting point?

You don't need to get Ralph's branch, he's provided everything
in the published patchset.

Easiest thing to do Trevor is to just use a copy of master,
then apply Ralph's patchset on top. You can then work on
that to squash the FIXUP's, then work on the README.Coding
changes. Then use:

git format-patch --stdout <starting-refspec>.. >/tmp/virus-patch

to get the modified patchset and re-post to samba-technical.

Cheers,

        Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On Fri, Dec 22, 2017 at 01:51:04PM -0800, Jeremy Allison via samba-technical wrote:

> On Fri, Dec 22, 2017 at 02:27:23PM -0700, Trever L. Adams wrote:
> > >
> > Sure. I am not sure how to do a git clone of something like:
> >
> > https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/av
> >
> >
> > How would I do that so I can use it as a starting point?
>
> You don't need to get Ralph's branch, he's provided everything
> in the published patchset.
>
> Easiest thing to do Trevor is to just use a copy of master,
> then apply Ralph's patchset on top. You can then work on
> that to squash the FIXUP's, then work on the README.Coding
> changes. Then use:
>
> git format-patch --stdout <starting-refspec>.. >/tmp/virus-patch
>
> to get the modified patchset and re-post to samba-technical.
So here's Ralph's patchset with the FIXUP patches squashed
into the previous patches, in the hope this will help get
you started.

Can you confirm it still works with your test environment
so we know the refactoring didn't break anything ?

Thanks,

Jeremy.

av-squash (141K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/22/2017 02:58 PM, Jeremy Allison wrote:

> On Fri, Dec 22, 2017 at 01:51:04PM -0800, Jeremy Allison via samba-technical wrote:
>> On Fri, Dec 22, 2017 at 02:27:23PM -0700, Trever L. Adams wrote:
>>> Sure. I am not sure how to do a git clone of something like:
>>>
>>> https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/av
>>>
>>>
>>> How would I do that so I can use it as a starting point?
>> You don't need to get Ralph's branch, he's provided everything
>> in the published patchset.
>>
>> Easiest thing to do Trevor is to just use a copy of master,
>> then apply Ralph's patchset on top. You can then work on
>> that to squash the FIXUP's, then work on the README.Coding
>> changes. Then use:
>>
>> git format-patch --stdout <starting-refspec>.. >/tmp/virus-patch
>>
>> to get the modified patchset and re-post to samba-technical.
> So here's Ralph's patchset with the FIXUP patches squashed
> into the previous patches, in the hope this will help get
> you started.
>
> Can you confirm it still works with your test environment
> so we know the refactoring didn't break anything ?
>
> Thanks,
>
> Jeremy.
Jeremy and Ralph, some of what is below may have existed prior to the
refactoring. I will get back to you in a day or two as noted below.


    /*
     * Check quarantine directory now to save processing
     * and becoming root over and over.
     */

Which is around line 373 in vfs_virusfilter.c is incorrect. The
directory cannot be created ahead of time, if I understanding the
BackupIntent opens change correctly, the quarantine directory must be in
the same level or lower (a/b/c/ or a/b/c/d/e not a/b) as the file in
question. I would like to remove this. Is every one okay with this?

There is a new null deference problem in the config param for the
communications sockect. I have fixed this. The lp_* function for that
parameter was called twice, invalidating some of the necessary checks. I
have fixed this. scan on open is supposed to default true. In the change
it was set to false. I have fixed this.

I am going to need another day or two to fully test the code with
clamav, to do the coding style fixups and to make sure that everything
matches the man page defaults.

Thank you,
Trever


signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/25/2017 10:51 AM, Trever L. Adams wrote:

>
> Jeremy and Ralph, some of what is below may have existed prior to the
> refactoring. I will get back to you in a day or two as noted below.
>
>
>     /*
>      * Check quarantine directory now to save processing
>      * and becoming root over and over.
>      */
>
> Which is around line 373 in vfs_virusfilter.c is incorrect. The
> directory cannot be created ahead of time, if I understanding the
> BackupIntent opens change correctly, the quarantine directory must be in
> the same level or lower (a/b/c/ or a/b/c/d/e not a/b) as the file in
> question. I would like to remove this. Is every one okay with this?
>
> There is a new null deference problem in the config param for the
> communications sockect. I have fixed this. The lp_* function for that
> parameter was called twice, invalidating some of the necessary checks. I
> have fixed this. scan on open is supposed to default true. In the change
> it was set to false. I have fixed this.
>
> I am going to need another day or two to fully test the code with
> clamav, to do the coding style fixups and to make sure that everything
> matches the man page defaults.
>
> Thank you,
> Trever
>
In the actual scanner modules, there is a lot of

if (stringfunction){

} else if (stringfunction) {

} else if (stringfunction) {

}

The logic is fairly clear as written and would be very difficult to make
sense if one must capture the return and use that in the if/else if.

As this is protocol talking/checking is it possible to leave as is? I
believe it was previously decided by someone that it was, but if not I
will try to figure things out. Other than that, I have some testing to
do and I can provide updated patches.

Thank you.

Trever



signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On Tue, Dec 26, 2017 at 03:11:56AM -0700, Trever L. Adams wrote:

> In the actual scanner modules, there is a lot of
>
> if (stringfunction){
>
> } else if (stringfunction) {
>
> } else if (stringfunction) {
>
> }
>
> The logic is fairly clear as written and would be very difficult to make
> sense if one must capture the return and use that in the if/else if.

Fmpov: leave these as is. There are two possible reasons for not doing function
calls in compound statements: eases debugging and easier to read. Neither holds
in this case.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/26/2017 07:19 AM, Ralph Böhme wrote:

> On Tue, Dec 26, 2017 at 03:11:56AM -0700, Trever L. Adams wrote:
>> In the actual scanner modules, there is a lot of
>>
>> if (stringfunction){
>>
>> } else if (stringfunction) {
>>
>> } else if (stringfunction) {
>>
>> }
>>
>> The logic is fairly clear as written and would be very difficult to make
>> sense if one must capture the return and use that in the if/else if.
> Fmpov: leave these as is. There are two possible reasons for not doing function
> calls in compound statements: eases debugging and easier to read. Neither holds
> in this case.
>
> -slow
>
Thank you Ralph. Attached is the unsquashed patch relative to your work.
I am providing it as I would appreciate another set of eyes to make sure
my changes enhance readability, debug-ability and maintainability. I
also need help to make sure I haven't accidentally inverted logic before
I squash it into the appropriate patches. I need this as it is not
possible for me to test all possible combinations/functionality at this
time.

Outstanding for me to do:

* Recheck quarantine (my installations tend to be rename only)

* Recheck streams (not normally used in my installation)

* Address issue brought up before being the code and comment mentioned
below:

    /*
     * Check quarantine directory now to save processing
     * and becoming root over and over.
     */

Which is around line 373 in vfs_virusfilter.c is incorrect. The
directory cannot be created ahead of time, if I understanding the
BackupIntent opens change correctly, the quarantine directory must be in
the same level or lower (a/b/c/ or a/b/c/d/e not a/b) as the file in
question. I would like to remove this. Is every one okay with this?

With assistance on the last, I should be finished with testing some time
tomorrow.

Thank you.
Trever

0001-Better-follow-Samba-coding-style.patch (38K) Download Attachment
signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
Trever

ret variable is not a boolean.
The check 'if (ret)' should be 'if (ret != 0)'.
The check 'if (!ret)' should be 'if (ret == 0)'.

ok variable is a boolean.
You should use 'if (!ok)' for a false test - and not either (ok ==
false) or (ok != true).

Jim

On 12/26/17 14:35, Trever L. Adams via samba-technical wrote:
> + ret = quarantine_directory_exist(handle, new_dir);
> + if (ret)

On 12/26/17 14:35, Trever L. Adams via samba-technical wrote:
> + ret = tevent_req_set_endtime(req, ev, timeval_current_ofs_msec(
> +    io_h->connect_timeout));
> + if (!ret)
(multiple places)

On 12/26/17 14:35, Trever L. Adams via samba-technical wrote:
> + ok = quarantine_create_dir(handle, config, temp_path);
> + if (ok == false)

On 12/26/17 14:35, Trever L. Adams via samba-technical wrote:
> + ok = virusfilter_io_writefl_readl(io_h, &reply, "zSCAN %s/%s",
> +  cwd_fname, fname);
> + if (ok != true)
(multiple places)


Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/26/2017 01:41 PM, Jim Brown via samba-technical wrote:

> Trever
>
> ret variable is not a boolean.
> The check 'if (ret)' should be 'if (ret != 0)'.
> The check 'if (!ret)' should be 'if (ret == 0)'.
>
> ok variable is a boolean.
> You should use 'if (!ok)' for a false test - and not either (ok ==
> false) or (ok != true).
>
> Jim
Jim, thank you. I have taken care of these. However, in testing, I am
finding that the streams scanning code no longer works as expected.

  virusfilter_vfs_close: Not scanned: only file backed streams can be
scanned: /home/DATA/trever/eicartestingstream.txt(1/0)

In the past, the file name showed what vfs_streams_depot was doing:
/home/DATA/.streams/0F/17/02FD000000000000EF005C0400000000:attached.txt:$DATA

This changed in a 4.6 or a 4.7 release. I do not know which.

Does anyone know how I can get that information again so that I can get
things working?

Thank you.
Trever


signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Dec 26, 2017 at 12:35:55PM -0700, Trever L. Adams wrote:

> * Address issue brought up before being the code and comment mentioned
> below:
>
>     /*
>      * Check quarantine directory now to save processing
>      * and becoming root over and over.
>      */
>
> Which is around line 373 in vfs_virusfilter.c is incorrect. The
> directory cannot be created ahead of time,

Why not? The quarantine directory is a configurable path relative to the
share. Or am I missing something?

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Dec 26, 2017 at 11:01:59PM -0700, Trever L. Adams via samba-technical wrote:

> On 12/26/2017 01:41 PM, Jim Brown via samba-technical wrote:
> > Trever
> >
> > ret variable is not a boolean.
> > The check 'if (ret)' should be 'if (ret != 0)'.
> > The check 'if (!ret)' should be 'if (ret == 0)'.
> >
> > ok variable is a boolean.
> > You should use 'if (!ok)' for a false test - and not either (ok ==
> > false) or (ok != true).
> >
> > Jim
> Jim, thank you. I have taken care of these.

Great, thanks! And thanks Jim for pointing those out. Guess I should have given
you a more explicit hint towards "Please read README.Coding", sorry.

> However, in testing, I am
> finding that the streams scanning code no longer works as expected.
>
>   virusfilter_vfs_close: Not scanned: only file backed streams can be
> scanned: /home/DATA/trever/eicartestingstream.txt(1/0)
>
> In the past, the file name showed what vfs_streams_depot was doing:
> /home/DATA/.streams/0F/17/02FD000000000000EF005C0400000000:attached.txt:$DATA

Well, vfs_streams_depot doesn't implement the close VFS operation. Not in master
nor in older vesions, so this can't ever have worked.

-slow

--
Ralph Boehme, Samba Team       https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
It is relative to the directory containing the file. I had a version relative to share, which is what I wanted, but it broke with Backup intents changes earlier this year. I tried to follow one of the snapshot modules to fix things when I couldn't do it the other way.

Trever

On December 27, 2017 2:03:44 PM MST, "Ralph Böhme" <[hidden email]> wrote:

>On Tue, Dec 26, 2017 at 12:35:55PM -0700, Trever L. Adams wrote:
>> * Address issue brought up before being the code and comment
>mentioned
>> below:
>>
>>     /*
>>      * Check quarantine directory now to save processing
>>      * and becoming root over and over.
>>      */
>>
>> Which is around line 373 in vfs_virusfilter.c is incorrect. The
>> directory cannot be created ahead of time,
>
>Why not? The quarantine directory is a configurable path relative to
>the
>share. Or am I missing something?
>
>-slow
>
>--
>Ralph Boehme, Samba Team       https://samba.org/
>Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
It did. I may have done a ->next on one of the fsp structures. I don't remember. It was working. This functionality  (getting actual POSIX file name for scanning and possibly other security related vfs modules is essential. I am seeing the same problem for open. Both were working, both need actual POSIX file names of all objects that are file system backed.

Trever

On December 27, 2017 2:10:41 PM MST, "Ralph Böhme" <[hidden email]> wrote:

>On Tue, Dec 26, 2017 at 11:01:59PM -0700, Trever L. Adams via
>samba-technical wrote:
>> On 12/26/2017 01:41 PM, Jim Brown via samba-technical wrote:
>> > Trever
>> >
>> > ret variable is not a boolean.
>> > The check 'if (ret)' should be 'if (ret != 0)'.
>> > The check 'if (!ret)' should be 'if (ret == 0)'.
>> >
>> > ok variable is a boolean.
>> > You should use 'if (!ok)' for a false test - and not either (ok ==
>> > false) or (ok != true).
>> >
>> > Jim
>> Jim, thank you. I have taken care of these.
>
>Great, thanks! And thanks Jim for pointing those out. Guess I should
>have given
>you a more explicit hint towards "Please read README.Coding", sorry.
>
>> However, in testing, I am
>> finding that the streams scanning code no longer works as expected.
>>
>>   virusfilter_vfs_close: Not scanned: only file backed streams can be
>> scanned: /home/DATA/trever/eicartestingstream.txt(1/0)
>>
>> In the past, the file name showed what vfs_streams_depot was doing:
>>
>/home/DATA/.streams/0F/17/02FD000000000000EF005C0400000000:attached.txt:$DATA
>
>Well, vfs_streams_depot doesn't implement the close VFS operation. Not
>in master
>nor in older vesions, so this can't ever have worked.
>
>-slow
>
>--
>Ralph Boehme, Samba Team       https://samba.org/
>Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/27/2017 02:32 PM, Trever L. Adams wrote:
> It did. I may have done a ->next on one of the fsp structures. I don't
> remember. It was working. This functionality (getting actual POSIX
> file name for scanning and possibly other security related vfs modules
> is essential. I am seeing the same problem for open. Both were
> working, both need actual POSIX file names of all objects that are
> file system backed.
>
> Trever
I know this worked because the appropriate renames took place. If I
remove some checks that are in the system now (checking to see if it is
a stream and not the default) the file and attached streams get removed
(I think this is on open, I didn't check closely enough last night).
However, this is a problem because the file may be legitimate, the other
streams may also be as well. It should be possible to scan the actual
stream by actual file system name and remove it.

vfs_virusfilter is getting a close event for streams other than default.
However, I currently have no way of knowing what the file system file
actually is.

Thank you.
Trever

>
> On December 27, 2017 2:10:41 PM MST, "Ralph Böhme" <[hidden email]>
> wrote:
>
>     On Tue, Dec 26, 2017 at 11:01:59PM -0700, Trever L. Adams via samba-technical wrote:
>
>         On 12/26/2017 01:41 PM, Jim Brown via samba-technical wrote:
>
>             Trever ret variable is not a boolean. The check 'if (ret)'
>             should be 'if (ret != 0)'. The check 'if (!ret)' should be
>             'if (ret == 0)'. ok variable is a boolean. You should use
>             'if (!ok)' for a false test - and not either (ok == false)
>             or (ok != true). Jim
>
>         Jim, thank you. I have taken care of these.
>
>
>     Great, thanks! And thanks Jim for pointing those out. Guess I should have given
>     you a more explicit hint towards "Please read README.Coding", sorry.
>
No. I was just somehow not noticing them. I am sorry.

>
>         However, in testing, I am finding that the streams scanning
>         code no longer works as expected.   virusfilter_vfs_close: Not
>         scanned: only file backed streams can be scanned:
>         /home/DATA/trever/eicartestingstream.txt(1/0) In the past, the
>         file name showed what vfs_streams_depot was doing:
>         /home/DATA/.streams/0F/17/02FD000000000000EF005C0400000000:attached.txt:$DATA
>
>
>
>     Well, vfs_streams_depot doesn't implement the close VFS operation. Not in master
>     nor in older vesions, so this can't ever have worked.
>
>     -slow
>
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.


signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/27/2017 03:10 PM, Trever L. Adams wrote:

> On 12/27/2017 02:32 PM, Trever L. Adams wrote:
>> It did. I may have done a ->next on one of the fsp structures. I
>> don't remember. It was working. This functionality (getting actual
>> POSIX file name for scanning and possibly other security related vfs
>> modules is essential. I am seeing the same problem for open. Both
>> were working, both need actual POSIX file names of all objects that
>> are file system backed.
>>
>> Trever
> I know this worked because the appropriate renames took place. If I
> remove some checks that are in the system now (checking to see if it
> is a stream and not the default) the file and attached streams get
> removed (I think this is on open, I didn't check closely enough last
> night). However, this is a problem because the file may be legitimate,
> the other streams may also be as well. It should be possible to scan
> the actual stream by actual file system name and remove it.
>
> vfs_virusfilter is getting a close event for streams other than
> default. However, I currently have no way of knowing what the file
> system file actually is.
>
> Thank you.
> Trever
Alright, I have the quarantine functionality working. It works with a
default relative to connection path of .quarantine and I believe can be
set to anything absolute.

I am still trying to get this to scan file backed streams (such as
vfs_streams_depot). Like I said, without an exit strategy for
non-filebacked streams, I believe it works on open but erases the file
and all streams, not just the stream. So, it works for now, it just
ignores ALL streams. This can be fixed if there is a way to find the
backing file systems file full path name. If it ever replaces the
smb_fname/base_name in the stack-able modules, then all is good, if not,
I will need to add some code to try and use whatever functionality is
provided to export this.

This is NOT a full patch. It is relative to the last full work Ralph did
as I would like another review of it in patch form before I squash down
and send the entire thing as five patches.

Thank you.
Trever

latest-av.patch (50K) Download Attachment
signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/27/2017 05:03 PM, Trever L. Adams via samba-technical wrote:

>
> Alright, I have the quarantine functionality working. It works with a
> default relative to connection path of .quarantine and I believe can be
> set to anything absolute.
>
> I am still trying to get this to scan file backed streams (such as
> vfs_streams_depot). Like I said, without an exit strategy for
> non-filebacked streams, I believe it works on open but erases the file
> and all streams, not just the stream. So, it works for now, it just
> ignores ALL streams. This can be fixed if there is a way to find the
> backing file systems file full path name. If it ever replaces the
> smb_fname/base_name in the stack-able modules, then all is good, if not,
> I will need to add some code to try and use whatever functionality is
> provided to export this.
>
> This is NOT a full patch. It is relative to the last full work Ralph did
> as I would like another review of it in patch form before I squash down
> and send the entire thing as five patches.
>
> Thank you.
> Trever
Sorry, this was missed in that patch. It is not commited and will
therefore be in the final.

Trever

diff --git a/docs-xml/manpages/vfs_virusfilter.8.xml
b/docs-xml/manpages/vfs_virusfilter.8.xml
index f1d980126bf..ee49df11575 100644
--- a/docs-xml/manpages/vfs_virusfilter.8.xml
+++ b/docs-xml/manpages/vfs_virusfilter.8.xml
@@ -155,10 +155,10 @@
                <varlistentry>
                <term>virusfilter:quarantine directory = PATH</term>
                <listitem>
-               <para>Where to move infected files. This is relative to, and
-               must be found in, the directory in which the file is
found.</para>
-               <para>If this option is not set, the default is
".quarantine".
-               </para>
+               <para>Where to move infected files. This path must be an
+               absolute path.</para>
+               <para>If this option is not set, the default is
".quarantine"
+               relative to the share path. </para>
                </listitem>
                </varlistentry>


signature.asc (903 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Trever,
You missed one change to 'if (!ok)' and one change to 'if (ok)'.
Jim

On 12/27/2017 7:03 PM, Trever L. Adams wrote:
> @@ -635,8 +647,8 @@ static virusfilter_action virusfilter_do_infected_file_action(
>   } else {
>   DBG_DEBUG("quarantine: Creating "
>        "directory %s\n", temp_path);
> - if (quarantine_create_dir(handle,
> -    config, temp_path) == false)
> + ok = quarantine_create_dir(handle, config, temp_path);
> + if (ok == false)

On 12/27/2017 7:03 PM, Trever L. Adams wrote:
> @@ -112,10 +113,11 @@ static virusfilter_result virusfilter_fsav_scan_init(
>   DBG_DEBUG("fsavd: Checking if connection is alive\n");
>  
>   /* FIXME: I don't know the correct PING command format... */
> - if (virusfilter_io_writefl_readl(io_h, &reply, "PING") ==
> -    true)
> + ok = virusfilter_io_writefl_readl(io_h, &reply, "PING");
> + if (ok == true)

Reply | Threaded
Open this post in threaded view
|

Re: Latest AV code

Samba - samba-technical mailing list
On 12/27/2017 05:31 PM, jim wrote:

> Trever,
> You missed one change to 'if (!ok)' and one change to 'if (ok)'.
> Jim
>
> On 12/27/2017 7:03 PM, Trever L. Adams wrote:
>> @@ -635,8 +647,8 @@ static virusfilter_action
>> virusfilter_do_infected_file_action(
>>           } else {
>>               DBG_DEBUG("quarantine: Creating "
>>                     "directory %s\n", temp_path);
>> -            if (quarantine_create_dir(handle,
>> -                config, temp_path) == false)
>> +            ok = quarantine_create_dir(handle, config, temp_path);
>> +            if (ok == false)
>
> On 12/27/2017 7:03 PM, Trever L. Adams wrote:
>> @@ -112,10 +113,11 @@ static virusfilter_result
>> virusfilter_fsav_scan_init(
>>           DBG_DEBUG("fsavd: Checking if connection is alive\n");
>>             /* FIXME: I don't know the correct PING command format... */
>> -        if (virusfilter_io_writefl_readl(io_h, &reply, "PING") ==
>> -            true)
>> +        ok = virusfilter_io_writefl_readl(io_h, &reply, "PING");
>> +        if (ok == true)
>
Thank you for catching the second. The first, I think something is
wrong. I have a place in the patch where that line is removed, not added
and in the actual files it is the same. Are you sure it exists as you
are seeing it. I cannot find that line in the patch where it is added.
(It might be in the original rework Ralph did, because it was there
before, but...)

I also have realized I have a small/temporary memory leak in the init
function.

Thank you.

Trever


signature.asc (903 bytes) Download Attachment