Fix TOCTOU / CAP_DAC_READ_SEARCH issue w/ call to access()

[duckfez]

Call to access() before attempting to actually open the file in question is a TOCTOU type bug, as the permissions on the file could change between the two operations.

Also, this makes using clamd with POSIX capabilities - CAP_DAC_READ_SEARCH specifically - not work because access() doesn't ask/answer the proper question. See https://access.redhat.com/solutions/1296703.

I added save_errno because I didn't know if thrmgr_setactivetask or thrmgr_group_need_terminate did things that would change the value of errno before I got a chance to check it.

[micahsnyder]

Per our discussing in IRC, some missing context:

CAP_DAC_READ_SEARCH is a posix capability that allows the kernel to bless a process, saying "You can read any file just like you were root." @duckfez started down this road b/c of not being happy with how the logging for clamdscan --fdpass works.

Ideally clamdscan and clamonacc would use fd-passing so they could run as root or run with CAP_DAC_READ_SEARCH (for clamdscan), and then clamd wouldn't need root or CAP_DAC_READ_SEARCH. However, --fdpass logging lacks filenames. Instead, the log file in /var/log/clamd.scan when you use --fdpass shows fd[12] as all the file names.

Running as clamd user w/ CAP_DAC_READ_SEARCH gives us true file names. Add AmbientCapabilities=CAP_DAC_READ_SEARCH to the systemd unit file and when clamd is launched it gets blessed. No more --fdpass needed and you get true file names and the actual privs given to clamd are not that much higher than the --fdpass mode. Thankfully there was only one call to access() to fix.

If a process has CAP_DAC_READ_SEARCH it is, for all intents and purposes, "root" for the purpose of opening files for read. If can't open files with root's privs to write, nor can it do other rootly things. But, sure, it can read hashes right out of /etc/shadow (eg: https://www.youtube.com/watch?v=3YwR7sSdX5Y).

That said, there's two ways to bless a process, the good way and the bad way.

• The bad way is what the above youtube video shows -- the /bin/tar binary has been blessed directly. Anything that invokes /bin/tar gets the capability.
• The good way is to let systemd bless only the instance of the process it launches, and launching that process is already a privileged operation.

[micahsnyder]

Pending some testing after 0.103 is released, it looks good to me!

[micahsnyder]

@duckfez I was reviewing your PR and would suggest one change. It's not a part of the cl_scanfile_callback API to set errno. Instead, it think it makes more sense to do the errno check within the cl_scanfile_callback function and return CL_EACCESS if access was denied. Eg:

diff --git a/clamd/scanner.c b/clamd/scanner.c
index 473398696..7968b2501 100644
--- a/clamd/scanner.c
+++ b/clamd/scanner.c
@@ -144,7 +144,6 @@ cl_error_t scan_callback(STATBUF *sb, char *filename, const char *msg, enum cli_
     int type = scandata->type;
     struct cb_context context;
     char *real_filename = NULL;
-    int save_errno;

     if (NULL != filename) {
         if (CL_SUCCESS != cli_realpath((const char *)filename, &real_filename)) {
@@ -264,7 +263,6 @@ cl_error_t scan_callback(STATBUF *sb, char *filename, const char *msg, enum cli_
     context.virsize  = 0;
     context.scandata = scandata;
     ret              = cl_scanfile_callback(filename, &virname, &scandata->scanned, scandata->engine, scandata->options, &context);
-    save_errno       = errno;
     thrmgr_setactivetask(NULL, NULL);

     if (thrmgr_group_need_terminate(scandata->conn->group)) {
@@ -278,7 +276,7 @@ cl_error_t scan_callback(STATBUF *sb, char *filename, const char *msg, enum cli_
         ret = CL_EMEM;
     }

-    if ((ret == CL_EOPEN) && (save_errno == EACCES)) {
+    if (ret == CL_EACCES) {
         if (conn_reply(scandata->conn, filename, "Access denied.", "ERROR") == -1) {
             free(filename);
             return CL_ETIMEOUT;
diff --git a/libclamav/scanners.c b/libclamav/scanners.c
index 2f46c348d..ec4c502e1 100644
--- a/libclamav/scanners.c
+++ b/libclamav/scanners.c
@@ -5094,8 +5094,13 @@ cl_error_t cl_scanfile_callback(const char *filename, const char **virname, unsi
     if (!fname)
         return CL_EARG;

-    if ((fd = safe_open(fname, O_RDONLY | O_BINARY)) == -1)
-        return CL_EOPEN;
+    if ((fd = safe_open(fname, O_RDONLY | O_BINARY)) == -1) {
+        if (errno == EACCES) {
+            return CL_EACCES;
+        } else {
+            return CL_EOPEN;
+        }
+    }

     if (fname != filename)
         free((char *)fname);

Otherwise I think the change makes sense. Will send it through testing with my recommendation appended to double-check before any merge.

[micahsnyder]

Oh, regarding CAP_DAC_READ_SEARCH -- it seems that we should not promote it's use. I think it adds extra security pitfalls to the clamav configuration.

Off the top of my head:

• If clamd is running with CAP_DAC_READ_SEARCH, an unprivileged user could initiate scans of directories they don't own. If clamd.conf is configured to report OK for every file, they'll be able to see all the filenames.
• If the clamd temp directory is not protect, they'll be able to intercept any time files created by clamd from scans of files they don't own.

Some good news relating to fd-passing though, I found that we can look up filenames by file descriptors for --fdpass scans. I have this working on Linux and Mac, at least. It seems to work well. I'm hoping to include this with other clamonacc bugfixes in the 0.103.1 patch release, soon.

[duckfez]

@micahsnyder I think I can live with those changes. You certainly understand the API better than I do. Do you want me to make them?

[micahsnyder]

@duckfez No worries I'll take care of it. Just wanted to run it by you before I did it.

[micahsnyder]

Merged here: 0bc6135...625e506

Thanks again @duckfez