From 24c75baa57e47c7953cfa22f7cbaee4dbcd2a38c Mon Sep 17 00:00:00 2001 From: don Date: Sat, 13 Mar 2010 05:07:36 +0000 Subject: [PATCH] Use new popenenv function instead of open; fixes remote code exploit as the spamass-milter user when run using -x. (closes: #573228) --- debian/changelog | 2 + spamass-milter.cpp | 162 ++++++++++++++++++++++++--------------------- spamass-milter.h | 1 + 3 files changed, 88 insertions(+), 77 deletions(-) diff --git a/debian/changelog b/debian/changelog index 5fb3ce2..dd6c3f2 100644 --- 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 Wed, 11 Mar 2009 03:59:39 -0700 diff --git a/spamass-milter.cpp b/spamass-milter.cpp index d4879f1..65657c1 100644 --- 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 431c65a..6f3bbcf 100644 --- 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 -- 2.39.2