[AMaViS-user] amavis-milter permissions [patch]

[Adam C. M. <ad...@mi...>]

Folks,
I've ran into a permissions issue running amavis-milter (from
amavisd-new-20030616) in which the ClamAV user is unable to scan files
created by amavis-milter.  I had been using this setup previously with a
dual-MTA setup but since amavis-milter defaults to producing directories
and files as mode 0700 and 0600 respectively my setup is broken.

I have read the documentation and realize that a simple approach would
be to run the milter and clamav under the same user-id however I'd
prefer for purposes of integration with other applications, to keep
ClamAV and amavisd-new running with their own different, default
user-ids.  I currently have the ClamAV user in group `amavis' to allow
readability with the dual-MTA setup.

I see from the code that there are concerns with permissions,
race-conditions, etc. so I don't know if this patch is an acceptable
solution but it solves my problem.  Does anyone else have a better way
to give milter the ability to safely produce group-readable files
without compromising security?

FWIW I'd argue that running everything as one user comes with it's own
set of security problems.  For example I like that `clamav' is in group
`amavis' because it produces a one-way trust relationship from the
former to the latter while it doesn't allow amavis the ability to write
to the ClamAV virus patterns database.

The patch:

--- helper-progs/amavis-milter.c.chmod  2004-03-31 19:42:23.000000000 -0500
+++ helper-progs/amavis-milter.c        2004-04-09 10:11:01.000000000 -0400
@@ -202,7 +202,7 @@
     int count = 0;

     if (use_fixed_name) {
-       if (mkdir(s, S_IRWXU) == 0) return s;  /* succeeded */
+       if (mkdir(s, S_IRWXU|S_IRGRP|S_IXGRP) == 0) return s;  /*
succeeded */
        amavis_syslog(DBG_FATAL, "(amavis_mkdtemp) creating directory %s
failed: %s",
                      s, strerror(errno));
     }
@@ -234,7 +234,7 @@
        }
# endif
        if (stt) {
-           if (!mkdir(s, S_IRWXU)) {
+           if (!mkdir(s, S_IRWXU|S_IRGRP|S_IXGRP)) {
                return s;
            } else {
                continue;
@@ -489,10 +489,11 @@
     }

     /* may be too restrictive for you, but's good to avoid problems */
-    if (!S_ISDIR(StatBuf.st_mode) || StatBuf.st_uid != geteuid() ||
StatBuf.st_gid != getegid() || !(StatBuf.st_mode & S_IRWXU)) {
+    if (!S_ISDIR(StatBuf.st_mode) || StatBuf.st_uid != geteuid() ||
StatBuf.st_gid != getegid() || !(StatBuf.st_mode &
(S_IRWXU|S_IRGRP|S_IXGRP))) {
        amavis_syslog(DBG_FATAL,
                "Security Warning: %s must be a directory owned by "
-               "User %d and Group %d, and read-/write-able by the User "
+               "User %d and Group %d, readable/writeable by the User "
+               "and readable by group "
                "only. Exit.", messagepath, geteuid(), getegid());
        return SMFIS_TEMPFAIL;
     }
@@ -512,7 +513,8 @@
        return SMFIS_TEMPFAIL;
     }

-    if ((priv->mlfi_fp = fopen(priv->mlfi_fname, "w+")) == NULL) {
+    if ((priv->mlfi_fp = fopen(priv->mlfi_fname, "w+")) == NULL
+       || fchmod(fileno(priv->mlfi_fp), S_IRUSR|S_IWUSR|S_IRGRP) == -1) {
        amavis_syslog(DBG_FATAL, "(mlfi_envfrom) creating file %s
failed: %s",
                      priv->mlfi_fname, strerror(errno));
        free(priv->mlfi_fname);   priv->mlfi_fname = NULL;

-- 
Adam C. Migus - http://people.migus.org/~adam/