Bug 13246 - backport Samba VirusFilter
Summary: backport Samba VirusFilter
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: 4.8.0rc1
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-24 14:41 UTC by David Disseldorp
Modified: 2020-12-11 09:40 UTC (History)
3 users (show)

See Also:


Attachments
backport for 4.8-test (137.25 KB, patch)
2018-01-24 15:06 UTC, David Disseldorp
slow: review+
Details
add release notes entry for vfs_virusfilter (902 bytes, patch)
2018-01-24 15:22 UTC, David Disseldorp
slow: review+
Details
fake clamav daemon for testing in future. (1.95 KB, text/x-python)
2018-01-24 20:09 UTC, Trever Adams
no flags Details
v2 backport for 4.8-test (139.68 KB, patch)
2018-01-26 13:18 UTC, David Disseldorp
slow: review-
Details
v3 backport for 4.8-test (140.06 KB, patch)
2018-01-27 13:36 UTC, David Disseldorp
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Disseldorp 2018-01-24 14:41:40 UTC
This ticket will be used to track the inclusion of Samba VirusFilter in Samba 4.8.0.
Comment 1 David Disseldorp 2018-01-24 15:06:56 UTC
Created attachment 13924 [details]
backport for 4.8-test

backport was a straight cherry-pick . I've only compile-tested this patchset.
Comment 2 Ralph Böhme 2018-01-24 15:11:17 UTC
Reassigning to Karolin for inclusion in 4.8.
Comment 3 David Disseldorp 2018-01-24 15:22:12 UTC
Created attachment 13925 [details]
add release notes entry for vfs_virusfilter

@Trever: Feel free to expand on this if you'd like to add more detail.
Comment 4 Ralph Böhme 2018-01-24 16:25:21 UTC
Comment on attachment 13925 [details]
add release notes entry for vfs_virusfilter

Minimal but to the point. Let's see what Trever and Jeremy have to say about this.
Comment 5 Trever Adams 2018-01-24 16:33:47 UTC
I think this is good. Thank you.
Comment 6 Trever Adams 2018-01-24 17:09:58 UTC
Please, don't ship this just yet. I just hit a crasher. I am sorry this was missed.
Comment 7 David Disseldorp 2018-01-24 17:15:00 UTC
(In reply to Trever Adams from comment #6)
> Please, don't ship this just yet. I just hit a crasher. I am sorry this was
> missed.

Thanks for the heads up, taking this of Karolin's queue for now.
Comment 8 Trever Adams 2018-01-24 17:23:36 UTC
I am wondering if I have a bad compile. I updated the system this was on and recompiled.

[2018/01/24 10:18:30.028695, 10, pid=20989, effective(3000007, 100), real(3000007, 0), class=virusfilter] ../source3/modules/vfs_virusfilter.c:1013(virusfilter_scan)
  virusfilter_scan: Searching cache entry: fname: (null)
[2018/01/24 10:18:30.028711, 10, pid=20989, effective(3000007, 100), real(3000007, 0), class=virusfilter] ../source3/modules/vfs_virusfilter.c:1024(virusfilter_scan)
  virusfilter_scan: Cache entry not found

fname should never be null.

static virusfilter_result virusfilter_scan(
	struct vfs_handle_struct *handle,
	struct virusfilter_config *config,
	const struct files_struct *fsp)
{
	virusfilter_result scan_result;
	char *scan_report = NULL;
	const char *fname = fsp->fsp_name->base_name;
	const char *cwd_fname = fsp->conn->cwd_fname->base_name;
	struct virusfilter_cache_entry *scan_cache_e = NULL;
	bool is_cache = false;
	virusfilter_action file_action = VIRUSFILTER_ACTION_DO_NOTHING;
	bool add_scan_cache = true;
	bool ok = false;

It is impossible for it to be NULL is it not?
Comment 9 Jeremy Allison 2018-01-24 17:26:54 UTC
What does the gdb backtrace say ?
Comment 10 Jeremy Allison 2018-01-24 17:27:46 UTC
To get into gdb, set:

panic action = /bin/sleep 9999999

in the [global] section of your smb.conf. Now when it crashes you'll be able to attach with gdb and fully inspect the smbd.
Comment 11 Trever Adams 2018-01-24 17:34:41 UTC
I have never seen fname be null before. I am sorry for missing this.

I believe the problem is that fname is NULL which, how can it be unless there is corruption somewhere. The problem does NOT happen if the file is infected. Only if it is clean. (Yes, I did test clean files, although possibly not the last two patches, but I think I did.)

#0  0x00007f41c9b887ea in waitpid () from /lib64/libc.so.6
#1  0x00007f41c9af4827 in do_system () from /lib64/libc.so.6
#2  0x00007f41cb676fc7 in smb_panic_s3 (why=0x7f41cd952cbd "internal error")
    at ../source3/lib/util.c:817
#3  0x00007f41cd900496 in smb_panic (why=0x7f41cd952cbd "internal error")
    at ../lib/util/fault.c:166
#4  0x00007f41cd900182 in fault_report (sig=11) at ../lib/util/fault.c:83
#5  0x00007f41cd900197 in sig_fault (sig=11) at ../lib/util/fault.c:94
#6  <signal handler called>
#7  0x00007f41c9b57286 in __strlen_sse2 () from /lib64/libc.so.6
#8  0x00007f41a64529f8 in virusfilter_clamav_scan (handle=0x55dcd54417b0, 
    config=0x55dcd5c022c0, fsp=0x55dcd5197b60, reportp=0x7fff795ebb70)
    at ../source3/modules/vfs_virusfilter_clamav.c:86
#9  0x00007f41a644ce09 in virusfilter_scan (handle=0x55dcd54417b0, 
    config=0x55dcd5c022c0, fsp=0x55dcd5197b60)
    at ../source3/modules/vfs_virusfilter.c:1038
#10 0x00007f41a644e526 in virusfilter_vfs_close (handle=0x55dcd54417b0, 
    fsp=0x55dcd5197b60) at ../source3/modules/vfs_virusfilter.c:1393
#11 0x00007f41cd4d1ce7 in smb_vfs_call_close (handle=0x55dcd54417b0, 
    fsp=0x55dcd5197b60) at ../source3/smbd/vfs.c:1736
#12 0x00007f41cd4ba613 in fd_close (fsp=0x55dcd5197b60)
    at ../source3/smbd/open.c:790
#13 0x00007f41cd4c801f in close_normal_file (req=0x55dcd517b030, 
---Type <return> to continue, or q <return> to quit---
    fsp=0x55dcd5197b60, close_type=NORMAL_CLOSE) at ../source3/smbd/close.c:762
#14 0x00007f41cd4c9756 in close_file (req=0x55dcd517b030, fsp=0x55dcd5197b60, 
    close_type=NORMAL_CLOSE) at ../source3/smbd/close.c:1234
#15 0x00007f41cd5182de in smbd_smb2_close (req=0x55dcd517a940, 
    fsp=0x55dcd5197b60, in_flags=0, out_flags=0x55dcd517ae82, 
    out_creation_ts=0x55dcd517ae88, out_last_access_ts=0x55dcd517ae98, 
    out_last_write_ts=0x55dcd517aea8, out_change_ts=0x55dcd517aeb8, 
    out_allocation_size=0x55dcd517aec8, out_end_of_file=0x55dcd517aed0, 
    out_file_attributes=0x55dcd517aed8) at ../source3/smbd/smb2_close.c:260
#16 0x00007f41cd518569 in smbd_smb2_close_send (mem_ctx=0x55dcd517a940, 
    ev=0x55dcd4f76c70, smb2req=0x55dcd517a940, in_fsp=0x55dcd5197b60, 
    in_flags=0) at ../source3/smbd/smb2_close.c:334
#17 0x00007f41cd5179b7 in smbd_smb2_request_process_close (req=0x55dcd517a940)
    at ../source3/smbd/smb2_close.c:70
#18 0x00007f41cd506aba in smbd_smb2_request_dispatch (req=0x55dcd517a940)
    at ../source3/smbd/smb2_server.c:2627
#19 0x00007f41cd50aa07 in smbd_smb2_io_handler (xconn=0x55dcd50baf30, 
    fde_flags=1) at ../source3/smbd/smb2_server.c:3914
#20 0x00007f41cd50ab0d in smbd_smb2_connection_handler (ev=0x55dcd4f76c70, 
    fde=0x55dcd5e10280, flags=1, private_data=0x55dcd50baf30)
    at ../source3/smbd/smb2_server.c:3952
#21 0x00007f41c9e9a670 in epoll_event_loop_once () from /lib64/libtevent.so.0
#22 0x00007f41c9e98af7 in std_event_loop_once () from /lib64/libtevent.so.0
---Type <return> to continue, or q <return> to quit---
#23 0x00007f41c9e94f5d in _tevent_loop_once () from /lib64/libtevent.so.0
#24 0x00007f41c9e9517b in tevent_common_loop_wait () from /lib64/libtevent.so.0
#25 0x00007f41c9e98a97 in std_event_loop_wait () from /lib64/libtevent.so.0
#26 0x00007f41cd4ef1c8 in smbd_process (ev_ctx=0x55dcd4f76c70, 
    msg_ctx=0x55dcd4f75f80, sock_fd=48, interactive=false)
    at ../source3/smbd/process.c:4127
#27 0x000055dcd320b9da in smbd_accept_connection (ev=0x55dcd4f76c70, 
    fde=0x55dcd5e1dd40, flags=1, private_data=0x55dcd5bcbf40)
    at ../source3/smbd/server.c:1030
#28 0x00007f41c9e9a670 in epoll_event_loop_once () from /lib64/libtevent.so.0
#29 0x00007f41c9e98af7 in std_event_loop_once () from /lib64/libtevent.so.0
#30 0x00007f41c9e94f5d in _tevent_loop_once () from /lib64/libtevent.so.0
#31 0x00007f41c9e9517b in tevent_common_loop_wait () from /lib64/libtevent.so.0
#32 0x00007f41c9e98a97 in std_event_loop_wait () from /lib64/libtevent.so.0
#33 0x000055dcd320c774 in smbd_parent_loop (ev_ctx=0x55dcd4f76c70, 
    parent=0x55dcd4f876b0) at ../source3/smbd/server.c:1397
#34 0x000055dcd320e8ff in main (argc=4, argv=0x7fff795ec7f8)
    at ../source3/smbd/server.c:2164

As I look at this,

  virusfilter_scan: Scan result: Clean: /home/MIDDLEEARTH-data/sv.stupiddog.infected
[2018/01/24 10:30:03.947950, 10, pid=21322, effective(3000007, 100), real(3000007, 0), class=virusfilter] ../source3/modules/vfs_virusfilter.c:1113(virusfilter_scan)
  virusfilter_scan: Adding new cache entry: sv.stupiddog.infected, 1
[2018/01/24 10:30:03.971563,  3, pid=21322, effective(3000007, 100), real(3000007, 0), class=virusfilter] ../source3/modules/vfs_virusfilter.c:1380(virusfilter_vfs_close)
  virusfilter_vfs_close: Not scanned: File not modified: /home/MIDDLEEARTH-data/(null)

it appears that this would be closing/opening of a directory. Is this accurate? How do I test to make sure it isn't a directory if that is the case?
Comment 12 Trever Adams 2018-01-24 17:39:16 UTC
Sorry, I found out what I was looking for. I have used it before (fsp->is_directory).
Comment 13 Trever Adams 2018-01-24 17:57:01 UTC
I think I found the problem. Somewhere along the line
char *fname = fsp->fsp_name->base_name = NULL;
crept in. Likely one of my iterative cleanups.

I am doing one or two other small cleanups while there and testing. I am not sure why this sometimes caused a problem and others did exactly what it should have including the log messages.
Comment 14 Jeremy Allison 2018-01-24 17:59:18 UTC
Oh, that's a fault of the review process - both Ralph and I missed it, sorry.

Please send a patch to samba-technical asap.

Ultimately we'll need a test suite for this, probably using a fake virus-scanner backend that looks for files matching a wildcard of *bad* and marks them as infected.
Comment 15 Trever Adams 2018-01-24 18:04:29 UTC
Is the Samba project willing to ship a copy of eicar test virus https://www.eicar.org? I suppose that would still need the test setup to have clamav.

I found where it crept in. It was in a cleanup. I am sorry that I missed correcting it. It appeared after version 14.

I am still waiting for a compile to finish before I can test my changes. (Which should short circuit on directories saving a little processing.)
Comment 16 Trever Adams 2018-01-24 18:05:06 UTC
Sorry, that is version 12.
Comment 17 Jeremy Allison 2018-01-24 18:09:23 UTC
Post the changes as *two* patches. One that fixes the fsp->base_name = NULL issue, and one that fixes the 'is_directory' check.

Let's keep reviewing more simple this time please :-).
Comment 18 Jeremy Allison 2018-01-24 18:10:39 UTC
(In reply to Trever Adams from comment #15)

No, we don't need to have a real antivirus setup to test the code changes. Simply an external process spun up at test that simply marks any file matching a known wildcard as "infected".
Comment 19 Ralph Böhme 2018-01-24 18:12:06 UTC
(In reply to Trever Adams from comment #15)
echo 'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*' > eicar.dat

Voila!
Comment 20 Trever Adams 2018-01-24 19:15:41 UTC
The patches have been sent.

I am creating a python script that fakes being clamav. The protocol is simple.
Comment 21 David Disseldorp 2018-01-24 19:44:57 UTC
(In reply to Ralph Böhme from comment #19)
> (In reply to Trever Adams from comment #15)
> echo 'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*'
> > eicar.dat

Looks like it could put the source repo/git.samba.org in danger of getting black listed.
Comment 22 Trever Adams 2018-01-24 20:09:15 UTC
Created attachment 13926 [details]
fake clamav daemon for testing in future.

import re is only needed if you are using the check_bad_dog function.

check_eicar should be removed if it will get Samba in trouble.

If the check_eicar isn't used look at line 57 & 58 to make appropriate change.

In your test use the clamav module, use this script to pretend to be the clamav daemon.

I have only tested this with socat, which I am not sure how to use to send null terminated strings which vfs_virusfilter will send, so I changed the \0 to \r in readcstr for testing.
Comment 23 Trever Adams 2018-01-24 20:10:11 UTC
any file name containing regex "bad.dog" (the period of course being any character) will trip the check_bad_dog version.
Comment 24 Jeremy Allison 2018-01-24 21:21:37 UTC
Let's develop the test fake clamav daemon on the samba-technical list rather than on this bug.
Comment 25 Trever Adams 2018-01-24 22:55:31 UTC
Alright. I have put forth a proposal on the list.

As for this bug, things have been more thoroughly tested here. With the actual crasher bug fix committed, the work on back porting to 4.8 can go ahead. I would prefer to also have the optimization as it may be necessary for Sophos and FSAV backends to work properly. (ClamAV won't scan directories using the command used in that backend. Sophos and FSAV may and that may have strange consequences not just poor performance.)
Comment 26 David Disseldorp 2018-01-26 13:18:04 UTC
Created attachment 13932 [details]
v2 backport for 4.8-test

This is again a straight cherry-pick from master, with Trever's two latest fixes (c890011a769b497855748e130fa41e998babc305 and e320c4c9b7426be296b3c311861ba2ddeeacdf9f) included. Compile tested only.
Comment 27 Ralph Böhme 2018-01-26 16:26:34 UTC
Comment on attachment 13932 [details]
v2 backport for 4.8-test

Thanks David, but can you please add the bug urls to the commits? Thanks!
Comment 28 David Disseldorp 2018-01-27 13:36:45 UTC
Created attachment 13933 [details]
v3 backport for 4.8-test

Same patchset with Bug URL in commit msgs.
Comment 29 Jeremy Allison 2018-01-27 18:34:08 UTC
Comment on attachment 13933 [details]
v3 backport for 4.8-test

LGTM. The only change you might make is to squash the last 2 patches into the main fix so we don't have a record of the "bad" first commit, but I'm OK with it as-is if you don't want to do that extra work.

Jeremy.
Comment 30 David Disseldorp 2018-01-27 21:26:48 UTC
(In reply to Jeremy Allison from comment #29)
> Comment on attachment 13933 [details]
> v3 backport for 4.8-test
> 
> LGTM. The only change you might make is to squash the last 2 patches into
> the main fix so we don't have a record of the "bad" first commit, but I'm OK
> with it as-is if you don't want to do that extra work.

Given that this is a backport patchset, I'd much prefer that we cherry-pick the master commits as-is, rather than squash any changes.
Comment 31 David Disseldorp 2018-01-27 21:49:48 UTC
@Karolin, please apply the v3 backport and release notes patches for 4.8.next.
Comment 32 Karolin Seeger 2018-01-31 09:54:21 UTC
(In reply to David Disseldorp from comment #31)
Done.
Pushed to autobuild-v4-8-test.
Comment 33 David Disseldorp 2018-01-31 10:42:54 UTC
(In reply to Karolin Seeger from comment #32)
> (In reply to David Disseldorp from comment #31)
> Done.
> Pushed to autobuild-v4-8-test.

Thanks Karo, though it looks as though the release notes patch "add release notes entry for vfs_virusfilter" didn't make it into autobuild.
Comment 34 Karolin Seeger 2018-01-31 11:03:42 UTC
(In reply to David Disseldorp from comment #33)
Oh, you are right, sorry!
Pushed to autobuild-v4-8-test now.
Comment 35 David Disseldorp 2018-02-07 09:57:45 UTC
Closing... @Trever: please follow up with the selftest functionality on the mailing list. Thanks everyone