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 1c601e979363cc356abc75526c2c0585ba13d9e1..0fa1839af5db8da5d35771278a10e847b79c82e9 100644 (file)
--- 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 b1cfe16d22fc3decf87ece3e8591159a88cd6490..1612b3b202695a0617d8917db637c54025aa8ca6 100644 (file)
--- 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 af8917dbe0484904d308bfa5066ac0c2605348d8..2f323c7b6c88afd9c412cf83cb14783f72a33981 100644 (file)
--- 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 9958a7989c49305abf1d2e7cff0e579a323706cc..7045c32281f03fa827c580b365f9c231409078ae 100644 (file)
--- 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 c0712571481c7974692598c3ace7413a93a06891..82bea80d1fadb82263ab4b5ae47c9e85d654bcab 100644 (file)
--- 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;