Allow for different (but POSIX compliant) behaviour of basename(3) and

prevent a use-after-free in that case in the new sftp-compat code.

POSIX allows basename(3) to either return a pointer to static storage
or modify the passed string and return a pointer to that.  OpenBSD does
the former and works as is, but on other platforms "filename" points
into "tmp" which was just freed.  This makes the freeing of tmp
consistent with the other variable in the loop.

Pinpointed by the -portable Valgrind regress test.  ok djm@ deraadt@

1 files changed, 4 insertions, 3 deletions

diff --git a/usr.bin/ssh/scp.c b/usr.bin/ssh/scp.c
index 21ca77e50ae..c30ebb553d5 100644
--- a/usr.bin/ssh/scp.c
+++ b/usr.bin/ssh/scp.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: scp.c,v 1.216 2021/08/02 23:38:27 djm Exp $ */
+/* $OpenBSD: scp.c,v 1.217 2021/08/04 01:34:55 dtucker Exp $ */
 /*
  * scp - secure remote copy.  This is basically patched BSD rcp which
  * uses ssh to do the data transfer (instead of using rcmd).
@@ -1428,11 +1428,9 @@ sink_sftp(int argc, char *dst, const char *src, struct sftp_conn *conn)
 		tmp = xstrdup(g.gl_pathv[i]);
 		if ((filename = basename(tmp)) == NULL) {
 			error("basename %s: %s", tmp, strerror(errno));
-			free(tmp);
 			err = -1;
 			goto out;
 		}
-		free(tmp);
 
 		if (local_is_dir(dst))
 			abs_dst = path_append(dst, filename);
@@ -1451,10 +1449,13 @@ sink_sftp(int argc, char *dst, const char *src, struct sftp_conn *conn)
 		}
 		free(abs_dst);
 		abs_dst = NULL;
+		free(tmp);
+		tmp = NULL;
 	}
 
 out:
 	free(abs_src);
+	free(tmp);
 	globfree(&g);
 	if (err == -1) {
 		fatal("Failed to download file '%s'", src);