[PATCH] Samba VirusFilter (version 12)

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

[PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
Thank you to all who have been most helpful it getting things cleaned
up. Hopefully this is ready for merging.

Trever


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

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
Trever

A few more comments.

Jim

In quarantine_create_dir 'ret' is bool. Test should be 'if (ret)'

On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:
> + ret = quarantine_directory_exist(handle, new_dir);
> + if (ret != 0)

There are places where you have early returns and call 'unbecome_root()'
in both cases.
Move 'unbecome_root()' before early exit if test (where possible).

On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:

> + int_ok = virusfilter_vfs_next_move(handle, smb_fname,
> +   q_smb_fname);
> + if (int_ok == -1)
> + {
> + unbecome_root();
> + DBG_ERR("Rename failed: %s/%s: Rename failed: %s\n",
> + cwd_fname, fname,
> + strerror(errno));
> + return VIRUSFILTER_ACTION_DO_NOTHING;
> + }
> + unbecome_root();

On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:
> + int_ok = SMB_VFS_NEXT_UNLINK(handle, smb_fname);
> + if (int_ok == -1) {
> + unbecome_root();
> + DBG_ERR("Delete failed: %s/%s: Unlink failed: %s\n",
> + cwd_fname, fname,
> + strerror(errno));
> + return VIRUSFILTER_ACTION_DO_NOTHING;
> + }
> + unbecome_root();

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Trever

In virusfilter_io_write isn't this the same as
     bool ok;
     ok = write_data_iov_timeout(...);
     return ok;

Jim

On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:
> + switch (write_data_iov_timeout(io_h->stream, &iov, 1,
> + io_h->io_timeout)) {
> + case false:
> + return false;
> + default:
> + return true;
> + }


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Jan 02, 2018 at 10:34:47AM -0700, Trever L. Adams via samba-technical wrote:
> Thank you to all who have been most helpful it getting things cleaned
> up. Hopefully this is ready for merging.

Will still need a few README.Coding changes, but closer !

Can you explain what this code does ?

                q_filepath = talloc_asprintf(talloc_tos(), "%s/%s%s%s", q_dir,
                                             q_prefix, base_name, q_suffix);

                TALLOC_FREE(q_dir);
                TALLOC_FREE(q_prefix);
                TALLOC_FREE(q_suffix);

                become_root();

                q_smb_fname = synthetic_smb_fname(mem_ctx, q_filepath,
                                                  smb_fname->stream_name, NULL,
                                                  smb_fname->flags);
                if (q_smb_fname == NULL) {
>>>>>>>>>>>>>           unlink(q_filepath);
                        unbecome_root();
                        return VIRUSFILTER_ACTION_DO_NOTHING;
                }

                int_ok = virusfilter_vfs_next_move(handle, smb_fname,
                                                   q_smb_fname);
                if (int_ok == -1)
                {
                        unbecome_root();
                        DBG_ERR("Rename failed: %s/%s: Rename failed: %s\n",
                                cwd_fname, fname,
                                strerror(errno));
                        return VIRUSFILTER_ACTION_DO_NOTHING;
                }
                unbecome_root();

Why are you unlinking q_filepath under root protections here ?

Once I understand that, I'll send you a README.coding fix
you can merge in and then I'll do a final review.

Thanks !

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
On 01/02/2018 04:26 PM, Jeremy Allison wrote:

> On Tue, Jan 02, 2018 at 10:34:47AM -0700, Trever L. Adams via samba-technical wrote:
>> Thank you to all who have been most helpful it getting things cleaned
>> up. Hopefully this is ready for merging.
> Will still need a few README.Coding changes, but closer !
>
> Can you explain what this code does ?
>
>                 q_filepath = talloc_asprintf(talloc_tos(), "%s/%s%s%s", q_dir,
>                                              q_prefix, base_name, q_suffix);
>
>                 TALLOC_FREE(q_dir);
>                 TALLOC_FREE(q_prefix);
>                 TALLOC_FREE(q_suffix);
>
>                 become_root();
>
>                 q_smb_fname = synthetic_smb_fname(mem_ctx, q_filepath,
>                                                   smb_fname->stream_name, NULL,
>                                                   smb_fname->flags);
>                 if (q_smb_fname == NULL) {
>>>>>>>>>>>>>>           unlink(q_filepath);
>                         unbecome_root();
>                         return VIRUSFILTER_ACTION_DO_NOTHING;
>                 }
>
>                 int_ok = virusfilter_vfs_next_move(handle, smb_fname,
>                                                    q_smb_fname);
>                 if (int_ok == -1)
>                 {
>                         unbecome_root();
>                         DBG_ERR("Rename failed: %s/%s: Rename failed: %s\n",
>                                 cwd_fname, fname,
>                                 strerror(errno));
>                         return VIRUSFILTER_ACTION_DO_NOTHING;
>                 }
>                 unbecome_root();
>
> Why are you unlinking q_filepath under root protections here ?
>
> Once I understand that, I'll send you a README.coding fix
> you can merge in and then I'll do a final review.
>
> Thanks !
>
> Jeremy.
>
Jeremy,

I am very sorry. This was a mistake from when I needed a safe call when
debugging a memory related crash over a year ago. It some how slipped
into my commits back then. You called me on it before. As noted
elsewhere I went back to a previous version of this (quarantine) code in
a recent changeset to simplify things and make the quarantine
functionality work like more people might expect it to. Unfortunately,
while integrating changes from the then current version into the old
one, this bug slipped back in.

The unlink is supposed to be TALLOC_FREE. I have changed it locally.
Again, my apologies for this slip. Thank you for catching it a second time.

I am sorry that I am missing other coding style issues. I look forward
to merging your changes.

Trever



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

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 01/02/2018 11:03 AM, jim via samba-technical wrote:
> Trever
>
> A few more comments.
>
> Jim
>
> In quarantine_create_dir 'ret' is bool. Test should be 'if (ret)'
Correct. Thank you for the catch.
>
> On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:
>> +        ret = quarantine_directory_exist(handle, new_dir);
>> +        if (ret != 0)
>
> There are places where you have early returns and call
> 'unbecome_root()' in both cases.
> Move 'unbecome_root()' before early exit if test (where possible).
There was another one of the cases below. All three have been corrected.
Thank you.

>
> On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:
>> +        int_ok = virusfilter_vfs_next_move(handle, smb_fname,
>> +                           q_smb_fname);
>> +        if (int_ok == -1)
>> +        {
>> +            unbecome_root();
>> +            DBG_ERR("Rename failed: %s/%s: Rename failed: %s\n",
>> +                cwd_fname, fname,
>> +                strerror(errno));
>> +            return VIRUSFILTER_ACTION_DO_NOTHING;
>> +        }
>> +        unbecome_root();
>
> On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:
>> +        int_ok = SMB_VFS_NEXT_UNLINK(handle, smb_fname);
>> +        if (int_ok == -1) {
>> +            unbecome_root();
>> +            DBG_ERR("Delete failed: %s/%s: Unlink failed: %s\n",
>> +                cwd_fname, fname,
>> +                strerror(errno));
>> +            return VIRUSFILTER_ACTION_DO_NOTHING;
>> +        }
>> +        unbecome_root();
>
>


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

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On 01/02/2018 12:05 PM, jim via samba-technical wrote:

> Trever
>
> In virusfilter_io_write isn't this the same as
>     bool ok;
>     ok = write_data_iov_timeout(...);
>     return ok;
>
> Jim
>
> On 1/2/2018 12:34 PM, Trever L. Adams via samba-technical wrote:
>> +    switch (write_data_iov_timeout(io_h->stream, &iov, 1,
>> +        io_h->io_timeout)) {
>> +    case false:
>> +        return false;
>> +    default:
>> +        return true;
>> +    }
>
>
>
Correct. This code path was very different at one point. Thank you for
catching this. It has been corrected.

Trever


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

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Tue, Jan 02, 2018 at 06:31:06PM -0700, Trever L. Adams via samba-technical wrote:

> On 01/02/2018 04:26 PM, Jeremy Allison wrote:
> > On Tue, Jan 02, 2018 at 10:34:47AM -0700, Trever L. Adams via samba-technical wrote:
> >> Thank you to all who have been most helpful it getting things cleaned
> >> up. Hopefully this is ready for merging.
> > Will still need a few README.Coding changes, but closer !
> >
> > Can you explain what this code does ?
> >
> >                 q_filepath = talloc_asprintf(talloc_tos(), "%s/%s%s%s", q_dir,
> >                                              q_prefix, base_name, q_suffix);
> >
> >                 TALLOC_FREE(q_dir);
> >                 TALLOC_FREE(q_prefix);
> >                 TALLOC_FREE(q_suffix);
> >
> >                 become_root();
> >
> >                 q_smb_fname = synthetic_smb_fname(mem_ctx, q_filepath,
> >                                                   smb_fname->stream_name, NULL,
> >                                                   smb_fname->flags);
> >                 if (q_smb_fname == NULL) {
> >>>>>>>>>>>>>>           unlink(q_filepath);
> >                         unbecome_root();
> >                         return VIRUSFILTER_ACTION_DO_NOTHING;
> >                 }
> >
> >                 int_ok = virusfilter_vfs_next_move(handle, smb_fname,
> >                                                    q_smb_fname);
> >                 if (int_ok == -1)
> >                 {
> >                         unbecome_root();
> >                         DBG_ERR("Rename failed: %s/%s: Rename failed: %s\n",
> >                                 cwd_fname, fname,
> >                                 strerror(errno));
> >                         return VIRUSFILTER_ACTION_DO_NOTHING;
> >                 }
> >                 unbecome_root();
> >
> > Why are you unlinking q_filepath under root protections here ?
> >
> > Once I understand that, I'll send you a README.coding fix
> > you can merge in and then I'll do a final review.
> >
> > Thanks !
> >
> > Jeremy.
> >
> Jeremy,
>
> I am very sorry. This was a mistake from when I needed a safe call when
> debugging a memory related crash over a year ago. It some how slipped
> into my commits back then. You called me on it before. As noted
> elsewhere I went back to a previous version of this (quarantine) code in
> a recent changeset to simplify things and make the quarantine
> functionality work like more people might expect it to. Unfortunately,
> while integrating changes from the then current version into the old
> one, this bug slipped back in.
>
> The unlink is supposed to be TALLOC_FREE. I have changed it locally.
> Again, my apologies for this slip. Thank you for catching it a second time.

Hey, no worries Trevor ! This is *fiendishly* complex stuff,
and I really appreciate you doing the work. I wasn't trying
to complain, just understand :-).

> I am sorry that I am missing other coding style issues. I look forward
> to merging your changes.

OK, I'll try and finish them up and send them to you soon.

Jeremy.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
On 01/02/2018 07:59 PM, Jeremy Allison wrote:

> On Tue, Jan 02, 2018 at 06:31:06PM -0700, Trever L. Adams via samba-technical wrote:
>> On 01/02/2018 04:26 PM, Jeremy Allison wrote:
>>> On Tue, Jan 02, 2018 at 10:34:47AM -0700, Trever L. Adams via samba-technical wrote:
>>>> Thank you to all who have been most helpful it getting things cleaned
>>>> up. Hopefully this is ready for merging.
>>> Will still need a few README.Coding changes, but closer !
>>>
>>> Can you explain what this code does ?
>>>
>>>                 q_filepath = talloc_asprintf(talloc_tos(), "%s/%s%s%s", q_dir,
>>>                                              q_prefix, base_name, q_suffix);
>>>
>>>                 TALLOC_FREE(q_dir);
>>>                 TALLOC_FREE(q_prefix);
>>>                 TALLOC_FREE(q_suffix);
>>>
>>>                 become_root();
>>>
>>>                 q_smb_fname = synthetic_smb_fname(mem_ctx, q_filepath,
>>>                                                   smb_fname->stream_name, NULL,
>>>                                                   smb_fname->flags);
>>>                 if (q_smb_fname == NULL) {
>>>>>>>>>>>>>>>>           unlink(q_filepath);
>>>                         unbecome_root();
>>>                         return VIRUSFILTER_ACTION_DO_NOTHING;
>>>                 }
>>>
>>>                 int_ok = virusfilter_vfs_next_move(handle, smb_fname,
>>>                                                    q_smb_fname);
>>>                 if (int_ok == -1)
>>>                 {
>>>                         unbecome_root();
>>>                         DBG_ERR("Rename failed: %s/%s: Rename failed: %s\n",
>>>                                 cwd_fname, fname,
>>>                                 strerror(errno));
>>>                         return VIRUSFILTER_ACTION_DO_NOTHING;
>>>                 }
>>>                 unbecome_root();
>>>
>>> Why are you unlinking q_filepath under root protections here ?
>>>
>>> Once I understand that, I'll send you a README.coding fix
>>> you can merge in and then I'll do a final review.
>>>
>>> Thanks !
>>>
>>> Jeremy.
>>>
>> Jeremy,
>>
>> I am very sorry. This was a mistake from when I needed a safe call when
>> debugging a memory related crash over a year ago. It some how slipped
>> into my commits back then. You called me on it before. As noted
>> elsewhere I went back to a previous version of this (quarantine) code in
>> a recent changeset to simplify things and make the quarantine
>> functionality work like more people might expect it to. Unfortunately,
>> while integrating changes from the then current version into the old
>> one, this bug slipped back in.
>>
>> The unlink is supposed to be TALLOC_FREE. I have changed it locally.
>> Again, my apologies for this slip. Thank you for catching it a second time.
> Hey, no worries Trevor ! This is *fiendishly* complex stuff,
> and I really appreciate you doing the work. I wasn't trying
> to complain, just understand :-).
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.

Thank you again.

Trever


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

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
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 <[hidden email]>
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.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Samba VirusFilter (version 12)

Samba - samba-technical mailing list
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 <[hidden email]>
> 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.

Trever


signature.asc (903 bytes) Download Attachment