Use new popenenv function instead of open; fixes remote code exploit

as the spamass-milter user when run using -x. (closes: #573228)

diff --git a/debian/changelog b/debian/changelog
index 5fb3ce249c68a57ab4459793036fcd9494bd7ea0..dd6c3f263ac1050b7661af3f7d6477524db281d8 100644 (file)
--- a/debian/changelog
+++ b/debian/changelog
@@ -9,6 +9,8 @@ spamass-milter (0.3.1-9) UNRELEASED; urgency=low
     - handle them more sanely in the init script
     - document how to deal with them in README.Debian and
       /etc/spamass-milter/default
+  * Use new popenenv function instead of open; fixes remote code exploit
+    as the spamass-milter user when run using -x. (closes: #573228)
 
  -- Don Armstrong <don@debian.org>  Wed, 11 Mar 2009 03:59:39 -0700
 

diff --git a/spamass-milter.cpp b/spamass-milter.cpp
index d4879f1b490490462bccb031f9279a04fe122d16..65657c193021b13020da2e3c69a5cda7935a1f63 100644 (file)
--- a/spamass-milter.cpp
+++ b/spamass-milter.cpp
@@ -172,10 +172,6 @@ bool flag_expand = false;  /* alias/virtusertable expansion */
 bool ignore_authenticated_senders = false;
 bool warnedmacro = false;      /* have we logged that we couldn't fetch a macro? */
 
-#if defined(__FreeBSD__) /* popen bug - see PR bin/50770 */
-static pthread_mutex_t popen_mutex = PTHREAD_MUTEX_INITIALIZER;
-#endif
-
 // {{{ main()
 
 int
@@ -467,59 +463,24 @@ assassinate(SMFICTX* ctx, SpamAssassin* assassin)
                           send another copy.  The milter API will not let you send the
                           message AND return a failure code to the sender, so this is
                           the only way to do it. */
-#if defined(__FreeBSD__)
-                       int rv;
-#endif
-                       
-#if defined(HAVE_ASPRINTF)
-                       char *buf;
-#else
-                       char buf[1024];
-#endif
-                       char *fmt="%s \"%s\"";
+                       char *popen_argv[3];
                        FILE *p;
 
-#if defined(HAVE_ASPRINTF)
-                       asprintf(&buf, fmt, SENDMAIL, spambucket);
-#else
-#if defined(HAVE_SNPRINTF)
-                       snprintf(buf, sizeof(buf)-1, fmt, SENDMAIL, spambucket);
-#else
-                       /* XXX possible buffer overflow here */
-                       sprintf(buf, fmt, SENDMAIL, spambucket);
-#endif
-#endif
-
-                       debug(D_COPY, "calling %s", buf);
-#if defined(__FreeBSD__) /* popen bug - see PR bin/50770 */
-                       rv = pthread_mutex_lock(&popen_mutex);
-                       if (rv)
-                       {
-                               debug(D_ALWAYS, "Could not lock popen mutex: %s", strerror(rv));
-                               abort();
-                       }               
-#endif
-                       p = popen(buf, "w");
+                       popen_argv[0] = SENDMAIL;
+                       popen_argv[1] = spambucket;
+                       popen_argv[2] = NULL;
+                       
+                       debug(D_COPY, "calling %s %s", SENDMAIL, spambucket);
+                       p = popenv(popen_argv, "w");
                        if (!p)
                        {
-                               debug(D_COPY, "popen failed(%s).  Will not send a copy to spambucket", strerror(errno));
+                               debug(D_COPY, "popenv failed(%s).  Will not send a copy to spambucket", strerror(errno));
                        } else
                        {
                                // Send message provided by SpamAssassin
                                fwrite(assassin->d().c_str(), assassin->d().size(), 1, p);
-                               pclose(p); p = NULL;
+                               fclose(p); p = NULL;
                        }
-#if defined(__FreeBSD__)
-                       rv = pthread_mutex_unlock(&popen_mutex);
-                       if (rv)
-                       {
-                               debug(D_ALWAYS, "Could not unlock popen mutex: %s", strerror(rv));
-                               abort();
-                       }               
-#endif
-#if defined(HAVE_ASPRINTF)
-                       free(buf);
-#endif 
                }
                return SMFIS_REJECT;
        }
@@ -864,30 +825,19 @@ mlfi_envrcpt(SMFICTX* ctx, char** envrcpt)
                /* open a pipe to sendmail so we can do address expansion */
 
                char buf[1024];
-               char *fmt="%s -bv \"%s\" 2>&1";
-
-#if defined(HAVE_SNPRINTF)
-               snprintf(buf, sizeof(buf)-1, fmt, SENDMAIL, envrcpt[0]);
-#else
-               /* XXX possible buffer overflow here */
-               sprintf(buf, fmt, SENDMAIL, envrcpt[0]);
-#endif
-
-               debug(D_RCPT, "calling %s", buf);
+               char *popen_argv[4];
+               
+               popen_argv[0] = SENDMAIL;
+               popen_argv[1] = "-bv";
+               popen_argv[2] = envrcpt[0];
+               popen_argv[3] = NULL;
 
-#if defined(__FreeBSD__) /* popen bug - see PR bin/50770 */
-               rv = pthread_mutex_lock(&popen_mutex);
-               if (rv)
-               {
-                       debug(D_ALWAYS, "Could not lock popen mutex: %s", strerror(rv));
-                       abort();
-               }               
-#endif
+               debug(D_RCPT, "calling %s -bv %s", SENDMAIL, envrcpt[0]);
 
-               p = popen(buf, "r");
+               p = popenv(popen_argv, "r");
                if (!p)
                {
-                       debug(D_RCPT, "popen failed(%s).  Will not expand aliases", strerror(errno));
+                       debug(D_RCPT, "popenv failed(%s).  Will not expand aliases", strerror(errno));
                        assassin->expandedrcpt.push_back(envrcpt[0]);
                } else
                {
@@ -912,16 +862,8 @@ mlfi_envrcpt(SMFICTX* ctx, char** envrcpt)
                                        assassin->expandedrcpt.push_back(p+7);
                                }
                        }
-                       pclose(p); p = NULL;
+                       fclose(p); p = NULL;
                }
-#if defined(__FreeBSD__)
-               rv = pthread_mutex_unlock(&popen_mutex);
-               if (rv)
-               {
-                       debug(D_ALWAYS, "Could not unlock popen mutex: %s", strerror(rv));
-                       abort();
-               }               
-#endif
        } else
        {
                assassin->expandedrcpt.push_back(envrcpt[0]);
@@ -2179,5 +2121,71 @@ void warnmacro(char *macro, char *scope)
        warnedmacro = true;
 }
 
+/*
+   untrusted-argument-safe popen function - only supports "r" and "w" modes
+   for simplicity, and always reads stdout and stderr in "r" mode.  Call
+   fclose to close the FILE.
+*/
+FILE *popenv(char *const argv[], const char *type)
+{
+       FILE *iop;
+       int pdes[2];
+       int save_errno;
+       if ((*type != 'r' && *type != 'w') || type[1])
+       {
+               errno = EINVAL;
+               return (NULL);
+       }
+       if (pipe(pdes) < 0)
+               return (NULL);
+       switch (fork()) {
+       
+       case -1:                        /* Error. */
+               save_errno = errno;
+               (void)close(pdes[0]);
+               (void)close(pdes[1]);
+               errno = save_errno;
+               return (NULL);
+               /* NOTREACHED */
+       case 0:                         /* Child. */
+               if (*type == 'r') {
+                       /*
+                        * The dup2() to STDIN_FILENO is repeated to avoid
+                        * writing to pdes[1], which might corrupt the
+                        * parent's copy.  This isn't good enough in
+                        * general, since the exit() is no return, so
+                        * the compiler is free to corrupt all the local
+                        * variables.
+                        */
+                       (void)close(pdes[0]);
+                       (void)dup2(pdes[1], STDOUT_FILENO);
+                       (void)dup2(pdes[1], STDERR_FILENO);
+                       if (pdes[1] != STDOUT_FILENO && pdes[1] != STDERR_FILENO) {
+                               (void)close(pdes[1]);
+                       } 
+               } else {
+                       if (pdes[0] != STDIN_FILENO) {
+                               (void)dup2(pdes[0], STDIN_FILENO);
+                               (void)close(pdes[0]);
+                       }
+                       (void)close(pdes[1]);
+               }
+               execv(argv[0], argv);
+               exit(127);
+               /* NOTREACHED */
+       }
+
+       /* Parent; assume fdopen can't fail. */
+       if (*type == 'r') {
+               iop = fdopen(pdes[0], type);
+               (void)close(pdes[1]);
+       } else {
+               iop = fdopen(pdes[1], type);
+               (void)close(pdes[0]);
+       }
+
+       return (iop);
+}
+
 // }}}
 // vim6:ai:noexpandtab

diff --git a/spamass-milter.h b/spamass-milter.h
index 431c65aa639d1d33047031303399092e6452d9c0..6f3bbcfd9f30e808a189f96d3b92c3a242bfabe0 100644 (file)
--- a/spamass-milter.h
+++ b/spamass-milter.h
@@ -186,5 +186,6 @@ int ip_in_networklist(struct in_addr ip, struct networklist *list);
 void parse_debuglevel(char* string);
 char *strlwr(char *str);
 void warnmacro(char *macro, char *scope);
+FILE *popenv(char *const argv[], const char *type);
 
 #endif