Further tightening up in PSCP. Fixed a couple more holes whereby a

malicious SCP server could have written to areas other than the ones
the user requested; cleared up buffer overruns everywhere. Hopefully
we now do not use arbitrary buffer limits _anywhere_.

[originally from svn r1205]

misc.c

@@ -4,7 +4,53 @@
 #include <assert.h>
 #include "putty.h"
 
-/*
+/* ----------------------------------------------------------------------
+ * String handling routines.
+ */
+
+char *dupstr(char *s)
+{
+    int len = strlen(s);
+    char *p = smalloc(len + 1);
+    strcpy(p, s);
+    return p;
+}
+
+/* Allocate the concatenation of N strings. Terminate arg list with NULL. */
+char *dupcat(char *s1, ...)
+{
+    int len;
+    char *p, *q, *sn;
+    va_list ap;
+
+    len = strlen(s1);
+    va_start(ap, s1);
+    while (1) {
+	sn = va_arg(ap, char *);
+	if (!sn)
+	    break;
+	len += strlen(sn);
+    }
+    va_end(ap);
+
+    p = smalloc(len + 1);
+    strcpy(p, s1);
+    q = p + strlen(p);
+
+    va_start(ap, s1);
+    while (1) {
+	sn = va_arg(ap, char *);
+	if (!sn)
+	    break;
+	strcpy(q, sn);
+	q += strlen(q);
+    }
+    va_end(ap);
+
+    return p;
+}
+
+/* ----------------------------------------------------------------------
  * Generic routines to deal with send buffers: a linked list of
  * smallish blocks, with the operations
  * 
@@ -99,7 +145,7 @@ void bufchain_prefix(bufchain *ch, void **data, int *len)
     *data = ch->head->buf + ch->head->bufpos;
 }
 
-/*
+/* ----------------------------------------------------------------------
  * My own versions of malloc, realloc and free. Because I want
  * malloc and realloc to bomb out and exit the program if they run
  * out of memory, realloc to reliably call malloc if passed a NULL
@@ -411,6 +457,10 @@ void safefree(void *ptr)
 #endif
 }
 
+/* ----------------------------------------------------------------------
+ * Debugging routines.
+ */
+
 #ifdef DEBUG
 static FILE *debug_fp = NULL;
 static int debug_got_console = 0;

misc.h

@@ -3,6 +3,9 @@
 
 #include "puttymem.h"
 
+char *dupstr(char *s);
+char *dupcat(char *s1, ...);
+
 struct bufchain_granule;
 typedef struct bufchain_tag {
     struct bufchain_granule *head, *tail;

psftp.c

@@ -24,52 +24,6 @@
  * send buffer.
  */
 
-/* ----------------------------------------------------------------------
- * String handling routines.
- */
-
-char *dupstr(char *s)
-{
-    int len = strlen(s);
-    char *p = smalloc(len + 1);
-    strcpy(p, s);
-    return p;
-}
-
-/* Allocate the concatenation of N strings. Terminate arg list with NULL. */
-char *dupcat(char *s1, ...)
-{
-    int len;
-    char *p, *q, *sn;
-    va_list ap;
-
-    len = strlen(s1);
-    va_start(ap, s1);
-    while (1) {
-	sn = va_arg(ap, char *);
-	if (!sn)
-	    break;
-	len += strlen(sn);
-    }
-    va_end(ap);
-
-    p = smalloc(len + 1);
-    strcpy(p, s1);
-    q = p + strlen(p);
-
-    va_start(ap, s1);
-    while (1) {
-	sn = va_arg(ap, char *);
-	if (!sn)
-	    break;
-	strcpy(q, sn);
-	q += strlen(q);
-    }
-    va_end(ap);
-
-    return p;
-}
-
 /* ----------------------------------------------------------------------
  * sftp client state.
  */

scp.c

@@ -672,6 +672,23 @@ static char *colon(char *str)
 	return (NULL);
 }
 
+/*
+ * Return a pointer to the portion of str that comes after the last
+ * slash or backslash.
+ */
+static char *stripslashes(char *str)
+{
+    char *p;
+
+    p = strrchr(str, '/');
+    if (p) str = p+1;
+
+    p = strrchr(str, '\\');
+    if (p) str = p+1;
+
+    return str;
+}
+
 /*
  *  Wait for a response from the other side.
  *  Return 0 if ok, -1 if error.
@@ -909,7 +926,6 @@ static void run_err(const char *fmt, ...)
  */
 static void source(char *src)
 {
-    char buf[2048];
     unsigned long size;
     char *last;
     HANDLE f;
@@ -1017,8 +1033,7 @@ static void source(char *src)
  */
 static void rsource(char *src)
 {
-    char buf[2048];
+    char *last, *findfile;
-    char *last;
     HANDLE dir;
     WIN32_FIND_DATA fdat;
     int ok;
@@ -1039,32 +1054,32 @@ static void rsource(char *src)
     if (scp_send_dirname(last, 0755))
 	return;
 
-    sprintf(buf, "%s/*", src);
+    findfile = dupcat(src, "/*", NULL);
-    dir = FindFirstFile(buf, &fdat);
+    dir = FindFirstFile(findfile, &fdat);
     ok = (dir != INVALID_HANDLE_VALUE);
     while (ok) {
 	if (strcmp(fdat.cFileName, ".") == 0 ||
 	    strcmp(fdat.cFileName, "..") == 0) {
-	} else if (strlen(src) + 1 + strlen(fdat.cFileName) >= sizeof(buf)) {
+	    /* ignore . and .. */
-	    run_err("%s/%s: Name too long", src, fdat.cFileName);
 	} else {
-	    sprintf(buf, "%s/%s", src, fdat.cFileName);
+	    char *foundfile = dupcat(src, "/", fdat.cFileName);
-	    source(buf);
+	    source(foundfile);
+	    sfree(foundfile);
 	}
 	ok = FindNextFile(dir, &fdat);
     }
     FindClose(dir);
+    sfree(findfile);
 
     (void) scp_send_enddir();
 }
 
 /*
- *  Execute the sink part of the SCP protocol.
+ * Execute the sink part of the SCP protocol.
  */
 static void sink(char *targ, char *src)
 {
-    char buf[2048];
+    char *destfname;
-    char namebuf[2048];
     char ch;
     int targisdir = 0;
     int settime;
@@ -1092,44 +1107,87 @@ static void sink(char *targ, char *src)
 
 	if (act.action == SCP_SINK_ENDDIR)
 	    return;
-	/* Security fix: ensure the file ends up where we asked for it. */
+
 	if (targisdir) {
-	    char t[2048];
+	    /*
-	    char *p;
+	     * Prevent the remote side from maliciously writing to
-	    strcpy(t, targ);
+	     * files outside the target area by sending a filename
+	     * containing `../'. In fact, it shouldn't be sending
+	     * filenames with any slashes in at all; so we'll find
+	     * the last slash or backslash in the filename and use
+	     * only the part after that. (And warn!)
+	     * 
+	     * In addition, we also ensure here that if we're
+	     * copying a single file and the target is a directory
+	     * (common usage: `pscp host:filename .') the remote
+	     * can't send us a _different_ file name. We can
+	     * distinguish this case because `src' will be non-NULL
+	     * and the last component of that will fail to match
+	     * (the last component of) the name sent.
+	     */
+	    char *striptarget, *stripsrc;
+
+	    striptarget = stripslashes(act.name);
+	    if (striptarget != act.name) {
+		tell_user(stderr, "warning: remote host sent a compound"
+			  " pathname - possibly malicious! (ignored)");
+	    }
+
+	    /*
+	     * Also check to see if the target filename is '.' or
+	     * '..', or indeed '...' and so on because Windows
+	     * appears to interpret those like '..'.
+	     */
+	    if (striptarget[strspn(striptarget, ".")] == '\0') {
+		bump("security violation: remote host attempted to write to"
+		     " a '.' or '..' path!");
+	    }
+
+	    if (src) {
+		stripsrc = stripslashes(src);
+		if (strcmp(striptarget, stripsrc)) {
+		    tell_user(stderr, "warning: remote host attempted to"
+			      " write to a different filename: disallowing");
+		}
+		/* Override the name the server provided with our own. */
+		striptarget = stripsrc;
+	    }
+
 	    if (targ[0] != '\0')
-		strcat(t, "/");
+		destfname = dupcat(targ, "\\", striptarget, NULL);
-	    p = act.name + strlen(act.name);
+	    else
-	    while (p > act.name && p[-1] != '/' && p[-1] != '\\')
+		destfname = dupstr(striptarget);
-		p--;
-	    strcat(t, p);
-	    strcpy(namebuf, t);
 	} else {
-	    strcpy(namebuf, targ);
+	    /*
+	     * In this branch of the if, the target area is a
+	     * single file with an explicitly specified name in any
+	     * case, so there's no danger.
+	     */
+	    destfname = dupstr(targ);
 	}
-	attr = GetFileAttributes(namebuf);
+	attr = GetFileAttributes(destfname);
 	exists = (attr != (DWORD) - 1);
 
 	if (act.action == SCP_SINK_DIR) {
 	    if (exists && (attr & FILE_ATTRIBUTE_DIRECTORY) == 0) {
-		run_err("%s: Not a directory", namebuf);
+		run_err("%s: Not a directory", destfname);
 		continue;
 	    }
 	    if (!exists) {
-		if (!CreateDirectory(namebuf, NULL)) {
+		if (!CreateDirectory(destfname, NULL)) {
-		    run_err("%s: Cannot create directory", namebuf);
+		    run_err("%s: Cannot create directory", destfname);
 		    continue;
 		}
 	    }
-	    sink(namebuf, NULL);
+	    sink(destfname, NULL);
 	    /* can we set the timestamp for directories ? */
 	    continue;
 	}
 
-	f = CreateFile(namebuf, GENERIC_WRITE, 0, NULL,
+	f = CreateFile(destfname, GENERIC_WRITE, 0, NULL,
 		       CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0);
 	if (f == INVALID_HANDLE_VALUE) {
-	    run_err("%s: Cannot create file", namebuf);
+	    run_err("%s: Cannot create file", destfname);
 	    continue;
 	}
 
@@ -1139,12 +1197,7 @@ static void sink(char *targ, char *src)
 	stat_bytes = 0;
 	stat_starttime = time(NULL);
 	stat_lasttime = 0;
-	if ((stat_name = strrchr(namebuf, '/')) == NULL)
+	stat_name = stripslashes(destfname);
-	    stat_name = namebuf;
-	else
-	    stat_name++;
-	if (strrchr(stat_name, '\\') != NULL)
-	    stat_name = strrchr(stat_name, '\\') + 1;
 
 	received = 0;
 	while (received < act.size) {
@@ -1188,10 +1241,12 @@ static void sink(char *targ, char *src)
 
 	CloseHandle(f);
 	if (wrerror) {
-	    run_err("%s: Write error", namebuf);
+	    run_err("%s: Write error", destfname);
 	    continue;
 	}
 	(void) scp_finish_filerecv();
+	sfree(destfname);
+	sfree(act.name);
     }
 }
 
@@ -1255,6 +1310,7 @@ static void toremote(int argc, char *argv[])
     (void) response();
 
     for (i = 0; i < argc - 1; i++) {
+	char *srcpath, *last;
 	HANDLE dir;
 	WIN32_FIND_DATA fdat;
 	src = argv[i];
@@ -1263,6 +1319,25 @@ static void toremote(int argc, char *argv[])
 	    errs++;
 	    continue;
 	}
+
+	/*
+	 * Trim off the last pathname component of `src', to
+	 * provide the base pathname which will be prepended to
+	 * filenames returned from Find{First,Next}File.
+	 */
+	srcpath = dupstr(src);
+	last = stripslashes(srcpath);
+	if (last == srcpath) {
+	    last = strchr(srcpath, ':');
+	    if (last)
+		last++;
+	    else
+		last = srcpath;
+	}
+printf("src=:%s:\nsrcpath=:%s:\nlast=:%s:\n", src, srcpath, last);
+	*last = '\0';
+printf("srcpath=:%s:\n", srcpath);
+
 	dir = FindFirstFile(src, &fdat);
 	if (dir == INVALID_HANDLE_VALUE) {
 	    run_err("%s: No such file or directory", src);
@@ -1270,7 +1345,7 @@ static void toremote(int argc, char *argv[])
 	}
 	do {
 	    char *last;
-	    char namebuf[2048];
+	    char *filename;
 	    /*
 	     * Ensure that . and .. are never matched by wildcards,
 	     * but only by deliberate action.
@@ -1292,23 +1367,12 @@ static void toremote(int argc, char *argv[])
 		} else
 		    continue;	       /* ignore this one */
 	    }
-	    if (strlen(src) + strlen(fdat.cFileName) >= sizeof(namebuf)) {
+	    filename = dupcat(srcpath, fdat.cFileName, NULL);
-		tell_user(stderr, "%s: Name too long", src);
+	    source(filename);
-		continue;
+	    sfree(filename);
-	    }
-	    strcpy(namebuf, src);
-	    if ((last = strrchr(namebuf, '/')) == NULL)
-		last = namebuf;
-	    else
-		last++;
-	    if (strrchr(last, '\\') != NULL)
-		last = strrchr(last, '\\') + 1;
-	    if (last == namebuf && strrchr(namebuf, ':') != NULL)
-		last = strchr(namebuf, ':') + 1;
-	    strcpy(last, fdat.cFileName);
-	    source(namebuf);
 	} while (FindNextFile(dir, &fdat));
 	FindClose(dir);
+	sfree(srcpath);
     }
 }