Ready to ship.

svn path=/trunk/; revision=1935

diff --git a/NEWS b/NEWS
index 00e9b1016e5ca742fc889154dd6d2b2058bd622a..5d40b91b6f881051fcd4fca0f90ad9605a5eaf76 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -19,6 +19,8 @@ in addition to the current leading-edge one, until the next gold version.
 * Marty Lee fixed a bug in activation of hosts named on the command-line.
 * The fetchall option forces RETR again.  We can cope with USA.NET now. 
 * Gunther Leber's patch to make fetchmail -V less chatty when mode is ETRN.
+* Gunther Leber's code to sanitize %T and %F expansion in the MDA string.
+* Jonathan Marten's fix for list option handling.
 
 There are 281 people on fetchmail-friends and 222 on fetchmail-announce.
 

diff --git a/driver.c b/driver.c
index b7971f38167e57c8513a582de3e61b4a3472abce..50e1e833557e0316f9d0dc6a977a07a95ff97d88 100644 (file)
--- a/driver.c
+++ b/driver.c
@@ -619,6 +619,19 @@ static int stuffline(struct query *ctl, char *buf)
     return(n);
 }
 
+
+static void sanitize(s)
+/* replace unsafe shellchars by an _ */
+char *s;
+{
+    static char *ok_chars = " 1234567890!@%-_=+:,./abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
+    char *cp;
+
+    for (cp = s; *(cp += strspn(cp, ok_chars)); /* NO INCREMENT */)
+       *cp = '_';
+}
+
+
 #define EMPTYLINE(s)   ((s)[0] == '\r' && (s)[1] == '\n' && (s)[2] == '\0')
 
 static int readheaders(sock, fetchlen, reallen, ctl, num)
@@ -1096,8 +1109,8 @@ int num;          /* index of message */
     }
     else if (ctl->mda)         /* we have a declared MDA */
     {
-       int     length = 0;
-       char    *names, *before, *after;
+       int     length = 0, fromlen = 0, nameslen = 0;
+       char    *names = NULL, *before, *after, *from = NULL;
 
        for (idp = xmit_names; idp; idp = idp->next)
            if (idp->val.status.mark == XMIT_ACCEPT)
@@ -1105,32 +1118,22 @@ int num;                /* index of message */
 
        destaddr = "localhost";
 
-       length = strlen(ctl->mda) + 1;
+       length = strlen(ctl->mda);
        before = xstrdup(ctl->mda);
 
-       /* sub user addresses for %T (or %s for backward compatibility) */
-       cp = (char *)NULL;
-       if (strstr(before, "%s") || (cp = strstr(before, "%T")))
+       /* get user addresses for %T (or %s for backward compatibility) */
+       if (strstr(before, "%s") || strstr(before, "%T"))
        {
-           char        *sp;
-
-           if (cp && cp[1] == 'T')
-               cp[1] = 's';
-
-           /* \177 had better be out-of-band for MDA commands */
-           for (sp = before; *sp; sp++)
-               if (*sp == '%' && sp[1] != 's' && sp[1] != 'T')
-                   *sp = '\177';
-
            /*
             * We go through this in order to be able to handle very
             * long lists of users and (re)implement %s.
             */
+           nameslen = 0;
            for (idp = xmit_names; idp; idp = idp->next)
                if (idp->val.status.mark == XMIT_ACCEPT)
-                   length += (strlen(idp->id) + 1);
+                   nameslen += (strlen(idp->id) + 1);  /* string + ' ' */
 
-           names = (char *)xmalloc(++length);
+           names = (char *)xmalloc(nameslen + 1);      /* account for '\0' */
            names[0] = '\0';
            for (idp = xmit_names; idp; idp = idp->next)
                if (idp->val.status.mark == XMIT_ACCEPT)
@@ -1138,48 +1141,82 @@ int num;                /* index of message */
                    strcat(names, idp->id);
                    strcat(names, " ");
                }
-           after = (char *)xmalloc(length);
-#ifdef SNPRINTF
-           snprintf(after, length, before, names);
-#else
-           sprintf(after, before, names);
-#endif /* SNPRINTF */
-           free(names);
-           free(before);
-           before = after;
-
-           for (sp = before; *sp; sp++)
-               if (*sp == '\177')
-                   *sp = '%';
+           names[--nameslen] = '\0';   /* chop trailing space */
+
+           /* sanitize names in order to contain *only* harmless shell chars */
+           sanitize(names);
        }
 
-       /* substitute From address for %F */
-       if ((cp = strstr(before, "%F")))
+       /* get From address for %F */
+       if (strstr(before, "%F"))
        {
-           char *from = return_path;
-           char        *sp;
+           from = xstrdup(return_path);
 
-           /* \177 had better be out-of-band for MDA commands */
-           for (sp = before; *sp; sp++)
-               if (*sp == '%' && sp[1] != 'F')
-                   *sp = '\177';
+           /* sanitize from in order to contain *only* harmless shell chars */
+           sanitize(from);
 
-           length += strlen(from);
-           after = (char *)xmalloc(length);
-           cp[1] = 's';
-#ifdef SNPRINTF
-           snprintf(after, length, before, from);
-#else
-           sprintf(after, before, from);
-#endif /* SNPRINTF */
-           free(before);
-           before = after;
-
-           for (sp = before; *sp; sp++)
-               if (*sp == '\177')
-                   *sp = '%';
+           fromlen = strlen(from);
        }
 
+       /* do we have to build an mda string? */
+       if (names || from) {
+               
+               char    *sp, *dp;
+
+
+               /* find length of resulting mda string */
+               sp = before;
+               while (sp = strstr(sp, "%s")) {
+                       length += nameslen - 2; /* subtract %s */
+                       sp += 2;
+               }
+               sp = before;
+               while (sp = strstr(sp, "%T")) {
+                       length += nameslen - 2; /* subtract %T */
+                       sp += 2;
+               }
+               sp = before;
+               while (sp = strstr(sp, "%F")) {
+                       length += fromlen - 2;  /* subtract %F */
+                       sp += 2;
+               }
+               
+               after = xmalloc(length + 1);
+
+               /* copy mda source string to after, while expanding %[sTF] */
+               for (dp = after, sp = before; *dp = *sp; dp++, sp++) {
+                       if (sp[0] != '%')       continue;
+
+                       /* need to expand? BTW, no here overflow, because in
+                       ** the worst case (end of string) sp[1] == '\0' */
+                       if (sp[1] == 's' || sp[1] == 'T') {
+                               strcpy(dp, names);
+                               dp += nameslen;
+                               sp++;   /* position sp over [sT] */
+                               dp--;   /* adjust dp */
+                       } else if (sp[1] == 'F') {
+                               strcpy(dp, from);
+                               dp += fromlen;
+                               sp++;   /* position sp over F */
+                               dp--;   /* adjust dp */
+                       }
+               }
+
+               if (names) {
+                       free(names);
+                       names = NULL;
+               }
+               if (from) {
+                       free(from);
+                       from = NULL;
+               }
+
+               free(before);
+
+               before = after;
+       }
+
+
        if (outlevel == O_VERBOSE)
            error(0, 0, "about to deliver with: %s", before);
 
@@ -1195,6 +1232,7 @@ int num;          /* index of message */
 
        sinkfp = popen(before, "w");
        free(before);
+       before = NULL;
 
 #ifdef HAVE_SETEUID
        /* this will fail quietly if we didn't start as root */

diff --git a/fetchmail.c b/fetchmail.c
index fda397b3fbc4a72e34d149997be03b2e828127d3..3e3ecf2ad26ad17ecfe9d00fc2e2eccd1f28c363 100644 (file)
--- a/fetchmail.c
+++ b/fetchmail.c
@@ -708,13 +708,13 @@ static void optmerge(struct query *h2, struct query *h1, int force)
      * is nonempty (treat h1 as an override).  
      */
 
-    if (!force)
-    {
-       append_str_list(&h2->server.localdomains, &h1->server.localdomains);
-       append_str_list(&h2->localnames, &h1->localnames);
-       append_str_list(&h2->mailboxes, &h1->mailboxes);
-       append_str_list(&h2->smtphunt, &h1->smtphunt);
-    }
+#define LIST_MERGE(fld) if (force ? !!h1->fld : !h2->fld) \
+                                       append_str_list(&h2->fld, &h1->fld)
+    LIST_MERGE(server.localdomains);
+    LIST_MERGE(localnames);
+    LIST_MERGE(mailboxes);
+    LIST_MERGE(smtphunt);
+#undef LIST_MERGE
 
 #define FLAG_MERGE(fld) if (force ? !!h1->fld : !h2->fld) h2->fld = h1->fld
     FLAG_MERGE(server.via);

diff --git a/fetchmail.man b/fetchmail.man
index bf75889fd50039568cc833bdd4b628d61b0b8273..11c01026f4221d54cf83a6027773a96fc526ee25 100644 (file)
--- a/fetchmail.man
+++ b/fetchmail.man
@@ -1588,10 +1588,10 @@ link can be tapped.
 .PP
 Use of the %F or %T escapes in an mda option could open a security
 hole, because they pass text manipulable by an attacker to a shell
-command.  The hole is reduced by the fact that fetchmail temporarily
-discards any suid privileges it may have while running the MDA.  To
-avoid potential problems, (1) enclose the %F and %T escapes in single
-quotes within the option, and (2) never use an mda command containing
+command.  Potential shell characters are replaced by `_' before
+execution.  The hole is further reduced by the fact that fetchmail
+temporarily discards any suid privileges it may have while running the
+MDA.  For maximum safety, however, don't use an mda command containing
 %F or %T when fetchmail is run from the root account itself.
 .PP
 Send comments, bug reports, gripes, and the like to Eric S. Raymond