From: Simon Tatham <anakin@pobox.com>
Date: Thu, 16 Dec 2004 19:36:47 +0000 (+0000)
Subject: General mechanism for ensuring a dodgy SFTP server can't return
X-Git-Tag: 0.58~308
X-Git-Url: https://asedeno.scripts.mit.edu/gitweb/?a=commitdiff_plain;h=6c81ee6706f75ccab7bc285d00139126524cce36;p=PuTTY.git

General mechanism for ensuring a dodgy SFTP server can't return
malicious filenames via FXP_READDIR.

[originally from svn r4995]
---

diff --git a/pscp.c b/pscp.c
index 1c601e97..0fa1839a 100644
--- a/pscp.c
+++ b/pscp.c
@@ -687,7 +687,6 @@ void scp_sftp_listdir(char *dirname)
 
 	    for (i = 0; i < names->nnames; i++)
 		ournames[nnames++] = names->names[i];
-
 	    names->nnames = 0;	       /* prevent free_names */
 	    fxp_free_names(names);
 	}
@@ -1289,8 +1288,21 @@ int scp_get_sink_action(struct scp_sink_action *act)
 		    namesize += names->nnames + 128;
 		    ournames = sresize(ournames, namesize, struct fxp_name);
 		}
-		for (i = 0; i < names->nnames; i++)
-		    ournames[nnames++] = names->names[i];
+		for (i = 0; i < names->nnames; i++) {
+		    if (!strcmp(names->names[i].filename, ".") ||
+			!strcmp(names->names[i].filename, "..")) {
+			/*
+			 * . and .. are normal consequences of
+			 * reading a directory, and aren't worth
+			 * complaining about.
+			 */
+		    } else if (!vet_filename(names->names[i].filename)) {
+			tell_user(stderr, "ignoring potentially dangerous server-"
+				  "supplied filename '%s'\n",
+				  names->names[i].filename);
+		    } else
+			ournames[nnames++] = names->names[i];
+		}
 		names->nnames = 0;	       /* prevent free_names */
 		fxp_free_names(names);
 	    }
diff --git a/psftp.c b/psftp.c
index b1cfe16d..1612b3b2 100644
--- a/psftp.c
+++ b/psftp.c
@@ -40,14 +40,6 @@ static Config cfg;
  * Higher-level helper functions used in commands.
  */
 
-/*
- * Determine whether a string is entirely composed of dots.
- */
-static int is_dots(char *str)
-{
-    return str[strspn(str, ".")] == '\0';
-}
-
 /*
  * Attempt to canonify a pathname starting from the pwd. If
  * canonification fails, at least fall back to returning a _valid_
@@ -291,10 +283,19 @@ int sftp_get_file(char *fname, char *outfname, int recurse, int restart,
 		    ournames = sresize(ournames, namesize, struct fxp_name *);
 		}
 		for (i = 0; i < names->nnames; i++)
-		    if (!is_dots(names->names[i].filename) &&
+		    if (strcmp(names->names[i].filename, ".") &&
+			strcmp(names->names[i].filename, "..") &&
 			(!wildcard || wc_match(wildcard,
-					       names->names[i].filename)))
-			ournames[nnames++] = fxp_dup_name(&names->names[i]);
+					       names->names[i].filename))) {
+			if (!vet_filename(names->names[i].filename)) {
+			    printf("ignoring potentially dangerous server-"
+				   "supplied filename '%s'\n",
+				   names->names[i].filename);
+			} else {
+			    ournames[nnames++] =
+				fxp_dup_name(&names->names[i]);
+			}
+		    }
 		fxp_free_names(names);
 	    }
 	    sftp_register(req = fxp_close_send(dirhandle));
diff --git a/psftp.h b/psftp.h
index af8917db..2f323c7b 100644
--- a/psftp.h
+++ b/psftp.h
@@ -149,6 +149,16 @@ WildcardMatcher *begin_wildcard_matching(char *name);
 char *wildcard_get_filename(WildcardMatcher *dir);
 void finish_wildcard_matching(WildcardMatcher *dir);
 
+/*
+ * Vet a filename returned from the remote host, to ensure it isn't
+ * in some way malicious. The idea is that this function is applied
+ * to filenames returned from FXP_READDIR, which means we can panic
+ * if we see _anything_ resembling a directory separator.
+ * 
+ * Returns TRUE if the filename is kosher, FALSE if dangerous.
+ */
+int vet_filename(char *name);
+
 /*
  * Create a directory. Returns 0 on error, !=0 on success.
  */
diff --git a/unix/uxsftp.c b/unix/uxsftp.c
index 9958a798..7045c322 100644
--- a/unix/uxsftp.c
+++ b/unix/uxsftp.c
@@ -341,6 +341,17 @@ void finish_wildcard_matching(WildcardMatcher *dir) {
     sfree(dir);
 }
 
+int vet_filename(char *name)
+{
+    if (strchr(name, '/'))
+	return FALSE;
+
+    if (name[0] == '.' && (!name[1] || (name[1] == '.' && !name[2])))
+	return FALSE;
+
+    return TRUE;
+}
+
 int create_directory(char *name)
 {
     return mkdir(name, 0777) == 0;
diff --git a/windows/winsftp.c b/windows/winsftp.c
index c0712571..82bea80d 100644
--- a/windows/winsftp.c
+++ b/windows/winsftp.c
@@ -445,6 +445,17 @@ void finish_wildcard_matching(WildcardMatcher *dir)
     sfree(dir);
 }
 
+int vet_filename(char *name)
+{
+    if (strchr(name, '/') || strchr(name, '\\') || strchr(name, ':'))
+	return FALSE;
+
+    if (!name[strspn(name, ".")])      /* entirely composed of dots */
+	return FALSE;
+
+    return TRUE;
+}
+
 int create_directory(char *name)
 {
     return CreateDirectory(name, NULL) != 0;