> Recently someone noted on BUGTRAQ that ftp(1) has two security
 > problems:
 >  [...]
 >
 > Problem:
 >     it is possible to trick the client into executing arbitrary code
 >     on the client's machine by returning a filename such as '|sh',
 >     whose contents are the list of shell commands to execute.
 > 
 > Suggested fix:
 >     modify recvrequest() to take an extra argument, which means
 >     "ignore special names such as '-' and '|*'". use this flag
 >     when calling recvrequest() from mget().
 >     I've done this, and it appears to work.

On second thought I don't think this is ideal: you also, if possible,
want to protect against someone seeing a file called '|sh' on an ftp
server and trying to get it. If you do "get |sh", at present, the
contents of the file on the server will be piped to sh since get
copies its argv[1] to argv[2] if the user doesn't supply one.

So I propose instead prepending "./" to any automatically generated
local name, both in get and mget, that begins with '|' or is '-'.
Also, names beginning with '/' should have a '.' prepended to them --
otherwise the server can send "/root/.rhosts" or
"/home/victim/.rhosts" or whatever.

Additionally, everything that mget generates should have ".." path
elements filtered out. I've left off this processing for get, on the
grounds that someone who does "get ../foo" should expect the file to
appear in the parent directory. Maybe this should be changed to be
more consistent.

My fix does reasonably unhappy things if you do "mget ../*".
Suggestions for improvements are welcome.

The following patch fixes the Linux ftp client, and should be pretty
readily adaptable to the NetBSD one. 

--- cmds.c	1997/06/13 07:25:11	1.20
+++ cmds.c	1997/08/17 18:19:47
@@ -34,9 +34,9 @@
 /*
  * from: @(#)cmds.c	5.26 (Berkeley) 3/5/91
  */
 char cmds_rcsid[] = 
-   "$Id: cmds.c,v 1.20 1997/06/13 07:25:11 dholland Exp $";
+   "$Id: cmds.c,v 1.23 1997/08/17 18:18:30 dholland Exp $";
 
 /*
  * FTP User Program -- Command Routines.
  */
@@ -90,8 +90,67 @@
 static void quote1(const char *initial, int argc, char **argv);
 
 
 /*
+ * pipeprotect: protect against "special" local filenames by prepending
+ * "./". Special local filenames are "-" and "|..." AND "/...".
+ */
+static char *pipeprotect(char *name) 
+{
+	char *nu;
+	if (strcmp(name, "-") && *name!='|' && *name!='/') {
+		return name;
+	}
+
+	/* We're going to leak this memory. XXX. */
+	nu = malloc(strlen(name)+3);
+	if (nu==NULL) {
+		perror("malloc");
+		code = -1;
+		return NULL;
+	}
+	strcpy(nu, ".");
+	if (*name != '/') strcat(nu, "/");
+	strcat(nu, name);
+	return nu;
+}
+
+/*
+ * Look for embedded ".." in a pathname and change it to "!!", printing
+ * a warning.
+ */
+static char *pathprotect(char *name)
+{
+	int gotdots=0, i, len;
+	
+	/* Convert null terminator to trailing / to catch a trailing ".." */
+	len = strlen(name)+1;
+	name[len-1] = '/';
+
+	/*
+	 * State machine loop. gotdots is < 0 if not looking at dots,
+	 * 0 if we just saw a / and thus might start getting dots,
+	 * and the count of dots seen so far if we have seen some.
+	 */
+	for (i=0; i<len; i++) {
+		if (name[i]=='.' && gotdots>=0) gotdots++;
+		else if (name[i]=='/' && gotdots<0) gotdots=0;
+		else if (name[i]=='/' && gotdots==2) {
+		    printf("Warning: embedded .. in %.*s (changing to !!)\n",
+			   len-1, name);
+		    name[i-1] = '!';
+		    name[i-2] = '!';
+		    gotdots = 0;
+		}
+		else if (name[i]=='/') gotdots = 0;
+		else gotdots = -1;
+	}
+	name[len-1] = 0;
+	return name;
+}
+
+
+/*
  * `Another' gets another argument, and stores the new argc and argv.
  * It reverts to the top level (via main.c's intr()) on EOF/error.
  *
  * Returns false if no new arguments have been added.
@@ -594,9 +653,17 @@
 	char *oldargv1, *oldargv2;
 
 	if (argc == 2) {
 		argc++;
-		argv[2] = argv[1];
+		/* 
+		 * Protect the user from accidentally retrieving special
+		 * local names.
+		 */
+		argv[2] = pipeprotect(argv[1]);
+		if (!argv[2]) {
+			code = -1;
+			return 0;
+		}
 		loc++;
 	}
 	if (argc < 2 && !another(&argc, &argv, "remote-file"))
 		goto usage;
@@ -768,10 +835,21 @@
 			}
 			if (mapflag) {
 				tp = domap(tp);
 			}
-			recvrequest("RETR", tp, cp, "w",
-			    tp != cp || !interactive);
+			/* Reject embedded ".." */
+			tp = pathprotect(tp);
+
+			/* Prepend ./ to "-" or "!*" or leading "/" */
+			tp = pipeprotect(tp);
+			if (tp == NULL) {
+				/* hmm... how best to handle this? */
+				mflag = 0;
+			}
+			else {
+				recvrequest("RETR", tp, cp, "w",
+					    tp != cp || !interactive);
+			}
 			if (!mflag && fromatty) {
 				ointer = interactive;
 				interactive = 1;
 				if (confirm("Continue with","mget")) {


-- 
   - David A. Holland             |    VINO project home page:
     dholland@eecs.harvard.edu    | http://www.eecs.harvard.edu/vino