CVE-2020-12062: Improper Input Validation (CWE-20)
value: The scp client in OpenSSH 8.2 incorrectly sends duplicate responses to the server upon a utimes system call failure, which allows a malicious unprivileged user on the remote server to overwrite arbitrary files in the client's download directory by creating a crafted subdirectory anywhere on the remote server. The victim must use the command scp -rp to download a file hierarchy containing, anywhere inside, this crafted subdirectory. NOTE: the vendor points out that "this attack can achieve no more than a hostile peer is already able to achieve within the scp protocol" and "utimes does not fail under normal circumstances.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 7.5
Published: 2020-06-01T16:15Z
Repository: https://github.com/openssh/openssh-portable
Language: C
Files changed: scp.c
Commit date: 2020-05-01 16:40:11 +1000
Commit message: upstream: when receving a file in sink(), be careful to send at

most a single error response after the file has been opened. Otherwise the
source() and sink() can become desyncronised. Reported by Daniel Goujot,
Georges-Axel Jaloyan, Ryan Lahfa, and David Naccache.

ok deraadt@ markus@

OpenBSD-Commit-ID: 6c14d233c97349cb811a8f7921ded3ae7d9e0035

Diff: diff --git a/scp.c b/scp.c
index 812ab53016..4390259804 100644
--- a/scp.c
+++ b/scp.c
@@ -1,6 +1,6 @@
-/* $OpenBSD: scp.c,v 1.208 2020/04/30 17:07:10 markus Exp $ */
+/* $OpenBSD: scp.c,v 1.209 2020/05/01 06:31:42 djm Exp $ */
 /*
  * scp - secure remote copy.  This is basically patched BSD rcp which
  * uses ssh to do the data transfer (instead of using rcmd).
  *
  * NOTE: This version should NOT be suid root.  (This uses ssh to
@@ -372,10 +372,11 @@ typedef struct {
 
 BUF *allocbuf(BUF *, int, int);
 void lostconn(int);
 int okname(char *);
 void run_err(const char *,...);
+int note_err(const char *,...);
 void verifydir(char *);
 
 struct passwd *pwd;
 uid_t userid;
 int errs, remin, remout;
@@ -1229,21 +1230,18 @@ rsource(char *name, struct stat *statp)
 void
 sink(int argc, char **argv, const char *src)
 {
 	static BUF buffer;
 	struct stat stb;
-	enum {
-		YES, NO, DISPLAYED
-	} wrerr;
 	BUF *bp;
 	off_t i;
 	size_t j, count;
 	int amt, exists, first, ofd;
 	mode_t mode, omode, mask;
 	off_t size, statbytes;
 	unsigned long long ull;
-	int setimes, targisdir, wrerrno = 0;
+	int setimes, targisdir, wrerr;
 	char ch, *cp, *np, *targ, *why, *vect[1], buf[2048], visbuf[2048];
 	char **patterns = NULL;
 	size_t n, npatterns = 0;
 	struct timeval tv[2];
 
@@ -1448,12 +1446,17 @@ bad:			run_err("%s: %s", np, strerror(errno));
 		if ((bp = allocbuf(&buffer, ofd, COPY_BUFLEN)) == NULL) {
 			(void) close(ofd);
 			continue;
 		}
 		cp = bp->buf;
-		wrerr = NO;
+		wrerr = 0;
 
+		/*
+		 * NB. do not use run_err() unless immediately followed by
+		 * exit() below as it may send a spurious reply that might
+		 * desyncronise us from the peer. Use note_err() instead.
+		 */
 		statbytes = 0;
 		if (showprogress)
 			start_progress_meter(curfile, size, &statbytes);
 		set_nonblock(remin);
 		for (count = i = 0; i < size; i += bp->cnt) {
@@ -1474,80 +1477,67 @@ bad:			run_err("%s: %s", np, strerror(errno));
 				cp += j;
 			} while (amt > 0);
 
 			if (count == bp->cnt) {
 				/* Keep reading so we stay sync'd up. */
-				if (wrerr == NO) {
+				if (!wrerr) {
 					if (atomicio(vwrite, ofd, bp->buf,
 					    count) != count) {
-						wrerr = YES;
-						wrerrno = errno;
+						note_err("%s: %s", np,
+						    strerror(errno));
+						wrerr = 1;
 					}
 				}
 				count = 0;
 				cp = bp->buf;
 			}
 		}
 		unset_nonblock(remin);
-		if (count != 0 && wrerr == NO &&
+		if (count != 0 && !wrerr &&
 		    atomicio(vwrite, ofd, bp->buf, count) != count) {
-			wrerr = YES;
-			wrerrno = errno;
-		}
-		if (wrerr == NO && (!exists || S_ISREG(stb.st_mode)) &&
-		    ftruncate(ofd, size) != 0) {
-			run_err("%s: truncate: %s", np, strerror(errno));
-			wrerr = DISPLAYED;
+			note_err("%s: %s", np, strerror(errno));
+			wrerr = 1;
 		}
+		if (!wrerr && (!exists || S_ISREG(stb.st_mode)) &&
+		    ftruncate(ofd, size) != 0)
+			note_err("%s: truncate: %s", np, strerror(errno));
 		if (pflag) {
 			if (exists || omode != mode)
 #ifdef HAVE_FCHMOD
 				if (fchmod(ofd, omode)) {
 #else /* HAVE_FCHMOD */
 				if (chmod(np, omode)) {
 #endif /* HAVE_FCHMOD */
-					run_err("%s: set mode: %s",
+					note_err("%s: set mode: %s",
 					    np, strerror(errno));
-					wrerr = DISPLAYED;
 				}
 		} else {
 			if (!exists && omode != mode)
 #ifdef HAVE_FCHMOD
 				if (fchmod(ofd, omode & ~mask)) {
 #else /* HAVE_FCHMOD */
 				if (chmod(np, omode & ~mask)) {
 #endif /* HAVE_FCHMOD */
-					run_err("%s: set mode: %s",
+					note_err("%s: set mode: %s",
 					    np, strerror(errno));
-					wrerr = DISPLAYED;
 				}
 		}
-		if (close(ofd) == -1) {
-			wrerr = YES;
-			wrerrno = errno;
-		}
+		if (close(ofd) == -1)
+			note_err(np, "%s: close: %s", np, strerror(errno));
 		(void) response();
 		if (showprogress)
 			stop_progress_meter();
-		if (setimes && wrerr == NO) {
+		if (setimes && !wrerr) {
 			setimes = 0;
 			if (utimes(np, tv) == -1) {
-				run_err("%s: set times: %s",
+				note_err("%s: set times: %s",
 				    np, strerror(errno));
-				wrerr = DISPLAYED;
 			}
 		}
-		switch (wrerr) {
-		case YES:
-			run_err("%s: %s", np, strerror(wrerrno));
-			break;
-		case NO:
+		/* If no error was noted then signal success for this file */
+		if (note_err(NULL) == 0)
 			(void) atomicio(vwrite, remout, "", 1);
-			break;
-		case DISPLAYED:
-			break;
-		}
 	}
 done:
 	for (n = 0; n < npatterns; n++)
 		free(patterns[n]);
 	free(patterns);
@@ -1631,10 +1621,42 @@ run_err(const char *fmt,...)
 		va_end(ap);
 		fprintf(stderr, "\n");
 	}
 }
 
+/*
+ * Notes a sink error for sending at the end of a file transfer. Returns 0 if
+ * no error has been noted or -1 otherwise. Use note_err(NULL) to flush
+ * any active error at the end of the transfer.
+ */
+int
+note_err(const char *fmt, ...)
+{
+	static char *emsg;
+	va_list ap;
+
+	/* Replay any previously-noted error */
+	if (fmt == NULL) {
+		if (emsg == NULL)
+			return 0;
+		run_err("%s", emsg);
+		free(emsg);
+		emsg = NULL;
+		return -1;
+	}
+
+	errs++;
+	/* Prefer first-noted error */
+	if (emsg != NULL)
+		return -1;
+
+	va_start(ap, fmt);
+	vasnmprintf(&emsg, INT_MAX, NULL, fmt, ap);
+	va_end(ap);
+	return -1;
+}
+
 void
 verifydir(char *cp)
 {
 	struct stat stb;
 

Vulnerable code: /* $OpenBSD: scp.c,v 1.208 2020/04/30 17:07:10 markus Exp $ */
	enum {
		YES, NO, DISPLAYED
	} wrerr;
	int setimes, targisdir, wrerrno = 0;
		wrerr = NO;
				if (wrerr == NO) {
						wrerr = YES;
						wrerrno = errno;
		if (count != 0 && wrerr == NO &&
			wrerr = YES;
			wrerrno = errno;
		}
		if (wrerr == NO && (!exists || S_ISREG(stb.st_mode)) &&
		    ftruncate(ofd, size) != 0) {
			run_err("%s: truncate: %s", np, strerror(errno));
			wrerr = DISPLAYED;
					run_err("%s: set mode: %s",
					wrerr = DISPLAYED;
					run_err("%s: set mode: %s",
					wrerr = DISPLAYED;
		if (close(ofd) == -1) {
			wrerr = YES;
			wrerrno = errno;
		}
		if (setimes && wrerr == NO) {
				run_err("%s: set times: %s",
				wrerr = DISPLAYED;
		switch (wrerr) {
		case YES:
			run_err("%s: %s", np, strerror(wrerrno));
			break;
		case NO:
			break;
		case DISPLAYED:
			break;
		}
Fixed code: /* $OpenBSD: scp.c,v 1.209 2020/05/01 06:31:42 djm Exp $ */
int note_err(const char *,...);
	int setimes, targisdir, wrerr;
		wrerr = 0;
		/*
		 * NB. do not use run_err() unless immediately followed by
		 * exit() below as it may send a spurious reply that might
		 * desyncronise us from the peer. Use note_err() instead.
		 */
				if (!wrerr) {
						note_err("%s: %s", np,
						    strerror(errno));
						wrerr = 1;
		if (count != 0 && !wrerr &&
			note_err("%s: %s", np, strerror(errno));
			wrerr = 1;
		if (!wrerr && (!exists || S_ISREG(stb.st_mode)) &&
		    ftruncate(ofd, size) != 0)
			note_err("%s: truncate: %s", np, strerror(errno));
					note_err("%s: set mode: %s",
					note_err("%s: set mode: %s",
		if (close(ofd) == -1)
			note_err(np, "%s: close: %s", np, strerror(errno));
		if (setimes && !wrerr) {
				note_err("%s: set times: %s",
		/* If no error was noted then signal success for this file */
		if (note_err(NULL) == 0)
/*
 * Notes a sink error for sending at the end of a file transfer. Returns 0 if
 * no error has been noted or -1 otherwise. Use note_err(NULL) to flush
 * any active error at the end of the transfer.
 */
int
note_err(const char *fmt, ...)
{
	static char *emsg;
	va_list ap;

	/* Replay any previously-noted error */
	if (fmt == NULL) {
		if (emsg == NULL)
			return 0;
		run_err("%s", emsg);
		free(emsg);
		emsg = NULL;
		return -1;
	}

	errs++;
	/* Prefer first-noted error */
	if (emsg != NULL)
		return -1;

	va_start(ap, fmt);
	vasnmprintf(&emsg, INT_MAX, NULL, fmt, ap);
	va_end(ap);
	return -1;
}