TODO items done

[treveradams]

I believe I have done the following items from your TODO list. I haven't tested or compiled yet, I was wondering if you would be interested in it as soon as I have done some testing.

Write manpages Remove a cache entry on close if file was modified when svf-:scan on open = yes and svf-:scan on close = no Extend quarantine options: Support "rename" action:

I investigated using the samba memcache implementation, it does not seem feasable unless the reply can be made a constant length.

[treveradams]

I am implementing rename as keep tree and keep name but quarantine. Is this what you mean by keep tree? Not that it makes the tree in the quarantine directory, but keeps things in place.

/* Fixup SVF_ACTION_RENAME as it is simply an alias that *
 * forces keep name and keep tree for QUARANTINE. */
if (svf_h->infected_file_action == SVF_ACTION_RENAME){
    svf_h->infected_file_action == SVF_ACTION_QUARANTINE;
            svf_h->quarantine_keep_name = true;
            svf_h->quarantine_keep_tree = true;
}

If so, I am ready to compile and test. If not, then I need to change several things. I do have open check to see if it starts with prefix and suffix and if so refuses to open the file. So, the way I am asking does act as a quarantine, but it is quarantine in place.

[fumiyas]

Please commit changes in each enhancements and fixes (do NOT commit as a single change) to your cloned repository, then send pull requests on GitHub. I'll see it.

Thank you!

[treveradams]

fumiyas, ok. I will break it up the best I can. I do need to know what you meant by "keep tree" do you mean quarantine in place or do you mean create directory tree structure under the quarantine directory? I assumed the former.

[fumiyas]

Thank you for your many many pull requests! Sorry for my late reviewing and pulling... Please wait for my work because I'm going to travel for a week.

[treveradams]

I am sorry if they were too many. I thought I was doing as you asked committing each separately. I look forward to working with your upon your return.

[fumiyas]

TODO:

• [x] Write manpages #9, #12, #17
• [x] Update configuration templates to match manpages and changes. #10
• [x] Use samba memcached #16, #18
• [x] Samba memcache improvement? #18
  • https://lists.samba.org/archive/samba-technical/2016-June/thread.html#114391
• [x] Remove a cache entry on close if file was modified when svf-*:scan on open = yes and svf-*:scan on close = no #11
• [x] svf-*:* must be svf_*:* in etc/svf-*.conf.tmpl 1f1391788d15b172438c4c6273eb9308ee595d27
• [x] Rename module filename to vfs_svf_*.so #24
• [x] svf_*:infected file action = quarantine #15
• [x] svf_*:infected file action = rename #15

[fumiyas]

quarantine keep tree = yes means that if a infected file foo.exe is placed in smb://server/share/bar/qux, then it is quaratined to /path/to/quarantine directory/bar/qux/foo.exe. If quarantine keep tree = no (by default?), it is quaratined to /path/to/quarantine directory/foo.exe

It is similar to "recycle:keeptree" option in vfs_recycle(8).

[treveradams]

I am sorry about the misunderstanding. I will rework the documentation and configuration changes now. I will rework the code within the next week.

[treveradams]

10 and #17 should be okay for merging now.

[treveradams]

I do have one problem with code I gave you. The line cache_h->cache = memcache_init(cache_h->ctx, entry_limit * (sizeof(svf_cache_entry) + 128)); should probably be: cache_h->cache = memcache_init(cache_h->ctx, entry_limit * (sizeof(svf_cache_entry) + SVF_IO_BUFFER_SIZE));

The file names and the results all take space. This will allocate a large amount of memory that may not be used. It can be used to cache more than the configured number of file entries when it isn't used (Samba's memcache does this). I missed fixing this up. I was going to do half of that, but then I realized that many locales and many installations may have a large path lengths or file name lengths.

[treveradams]

It may be good to ask for the following to be committed to Samba if/when you try to get the module committed. For now, the code will work around the right entry being missing, but Jeremy Allison of the Samba project said it was the right thing to do this patch.

$ diff --git a/lib/util/memcache.c b/lib/util/memcache.c
index 9e9a208..f93c898 100644
--- a/lib/util/memcache.c
+++ b/lib/util/memcache.c
@@ -53,6 +53,7 @@ static bool memcache_is_talloc(enum memcache_number n)
        case GETPWNAM_CACHE:
        case PDB_GETPWSID_CACHE:
        case SINGLETON_CACHE_TALLOC:
        case SHARE_MODE_LOCK_CACHE:
+       case SVF_SCAN_RESULTS_CACHE_TALLOC:
                result = true;
                break;
diff --git a/lib/util/memcache.h b/lib/util/memcache.h
index 2602fb7..a80d36e 100644
--- a/lib/util/memcache.h
+++ b/lib/util/memcache.h
@@ -41,7 +41,8 @@ enum memcache_number {
        SINGLETON_CACHE_TALLOC, /* talloc */
        SINGLETON_CACHE,
        SMB1_SEARCH_OFFSET_MAP,
-       SHARE_MODE_LOCK_CACHE   /* talloc */
+       SHARE_MODE_LOCK_CACHE,  /* talloc */
+       SVF_SCAN_RESULTS_CACHE_TALLOC /* talloc */
 };

 /*

[treveradams]

After doing some thinking, reading, etc. I figure my new pull request is indeed the correct approach. It is a lower memory usage than what I suggested here. The system admin can always lower the number of files for which to cache results, but is going to expect that the number given is pretty close to accurate instead of an extremely over estimated number.

I have included the patch above in the contrib directory added in that pull request (PR #18).

[treveradams]

If there are any problems in PR #15, I would appreciate hearing back as they have been working here without incident for several days now (except for my sent mail 2013 folder being blocked due to a EICAR test virus being sent to test a mail configuration then).

[treveradams]

I believe you have merged all of my pull requests relating to this issue. Thank you. I have not repulled on my test machine, but things look great. Thank you!

[treveradams]

One of my patches messed up the sophos module man page. I will be creating a pull request shortly that fixes this and acts on issue #19.

[treveradams]

Regarding "Rename module filename to vfs_svf.so", would it be better to name it virusfilter.so to be more descriptive? Either way I believe I can take care of this if you want to. I ask because it seems all of the other vfs modules in samba are not prefixed with vfs and have descriptive names. The man pages start with vfs. Or, things could be left as is.

[fumiyas]

It seems good to me.

Sorry for my late reply.

[treveradams]

On the TODO list in this issue, is there anything left that needs to be done? So far as I know, I have done them all except the module rename, but I think that is actually done as is.

[treveradams]

File names: /usr/local/samba/lib/vfs/recycle.so /usr/local/samba/share/man/man8/vfs_recycle.8

/usr/local/samba/lib/vfs/svf_clamav.so /usr/local/samba/lib/vfs/svf_fsav.so /usr/local/samba/lib/vfs/svf_sophos.so

(Note, I am not sure what to do about installing the man pages and things at this time, so they weren't installed.)

I believe we do NOT want to rename the modules. As I said on July 20, the vfs_ prefix is used for the manpages, but not for the module.

[fumiyas]

Which one do you think better?

1. Current name style:
   • /usr/local/samba/lib/vfs/svf_clamav.so
   • /usr/local/samba/share/man/man8/vfs_svf_clamav.8
2. General name style:
   • /usr/local/samba/lib/vfs/virusfilter_clamav.so
   • /usr/local/samba/share/man/man8/vfs_virusfilter_clamav.8
3. Or another? (Any ideas?)

I dislike original acronym words for upstream merge because they are NOT self‐evident. That is why I think 12 is better.

[treveradams]

I am confused. You like 1 or 2 better?

[treveradams]

If it is option 2, I am testing my changes now.

[treveradams]

Things check out ok. If pull request #24 meets your approval, it should close this issue (#8).

[fumiyas]

Ah, sorry... I like 2. I've fixed comment in https://github.com/fumiyas/samba-virusfilter/issues/8#issuecomment-245320821.