Ready to ship.

svn path=/trunk/; revision=1935

4 files changed, 104 insertions, 64 deletions

diff --git a/NEWS b/NEWS
index 00e9b101..5d40b91b 100644
--- 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 b7971f38..50e1e833 100644
--- 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 fda397b3..3e3ecf2a 100644
--- 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 bf75889f..11c01026 100644
--- 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