Request for review of Samba VirusFilter preparation for merging with Samba

[Trever L. Adams]

On 09/10/2016 03:41 AM, Volker Lendecke wrote:
> On Fri, Sep 09, 2016 at 11:15:04AM -0600, Trever L. Adams wrote:
>> I have done some previous work on this. As stated, I am not the original
>> author or maintainer, but I am working with him to knock out his TODO
>> list and get it ready for merging. I believe we may be in the last week
>> or so of work on this.
>>
>> Anyone who works with integrating modules who is willing to work with me
>> (knowing I am only helping out), please review
>> https://github.com/fumiyas/samba-virusfilter/issues/23 and the github
>> tree
>> https://github.com/treveradams/samba-virusfilter/tree/merge_prep_reorganize.
>>
>> I would love to have input so that I can do my part in this as quickly,
>> efficiently and correctly as possible.
> Just took a brief look at vfs_virusfilter_vfs.c. A few comments:
>
> Don't use SMB_STRDUP and SMB_MALLOC in new code. It's all talloc now
> with a proper talloc hierarchy or for tmp variables with talloc_tos().
quarantine_create_dir is mine, it does use those, it is mostly a
copy/past from vfs_recycle.c. I have fixed that.
>
> "if (q_prefix) TALLOC_FREE(q_prefix);" is double-checking. TALLOC_FREE
> already checks for NULL.
Thank you. I will correct that. I do a lot of C work and I am just in
the habit of checking.
>
> There's a LOT of become_root() inside. To be honest, I feel a bit
> uneasy with that. Can you explain why that is required?
quarantine_create_dir: It is necessary to create directories.

quarantine_directory_exist: The directories it checks may not be
readable by the user.

virusfilter_action virusfilter_do_infected_file_action: It is necessary
for removing/renaming/moving files as the person accessing them may not
have permission to do so. I believe it is also necessary to make a
temporary file as the user may not have permission to do so. I didn't
test this as I only worked on that part, I didn't write it.

Virus scanning modules: This has to do with the permission of the
socket. As far as I can see it is only to allow the socket to be opened.
Once that is done permissions are dropped. I believe this is necessary
to maintain security of the socket so the virus scanner can't be used to
mess with files locally.
>
> Then -- no lines beyond 80 chars please.
Is this hard and fast? There are a few things that are right around
80-83 characters that would be difficult to split up.

Does this apply to function definitions in .h files?
>
> The virusfilter_io abstraction could see a lot of code re-use from our
> base libraries. We have for example write_data_iov that you could
> re-use. If that does not do what you need, can we share this code
> somehow?
I will look into that and work with the author/maintainer.
>
>
> If I get the code right, you're not linking to proprietary libaries
> but connect via sockets to an AV daemon. Is that right? We never
> really approached AV because that usually means linking to proprietary
> libraries. This problem is gone now?
>
> Volker
>
I only worked with the clamav part, but I quickly looked over the
others, as far as I know there is no linking, only connecting via
sockets. I believe there was a planned module to do external calls
(fork/exec/std(in|out) redirection), but I don't believe that has been
done, just planned.

Other than write_data_iov and other reuse issues, I believe I have done
as you have asked.

Thank you for the review!

Trever


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