openssh/openssh-7.9p1-scp-name-validator.patch

349 lines
8.5 KiB
Diff
Raw Normal View History

diff --git a/defines.h b/defines.h
index 8f421306..8b4af9b2 100644
--- a/defines.h
+++ b/defines.h
@@ -873,4 +873,10 @@ struct winsize {
# define USE_SYSTEM_GLOB
#endif
+/*
+ * Define to enable additional scp file name validation against
+ * malicious servers.
+ */
+#define USE_SCP_NAME_VALIDATOR 1
+
#endif /* _DEFINES_H */
diff --git a/progressmeter.c b/progressmeter.c
index fe9bf52e..b2a3a38d 100644
--- a/progressmeter.c
+++ b/progressmeter.c
@@ -160,7 +160,10 @@ refresh_progress_meter(void)
buf[0] = '\0';
file_len = win_size - 35;
if (file_len > 0) {
- len = snprintf(buf, file_len + 1, "\r%s", file);
+ char visbuf[MAX_WINSIZE + 1];
+ (void) snmprintf(visbuf, sizeof(visbuf),
+ NULL, "%s", file);
+ len = snprintf(buf, file_len + 1, "\r%s", visbuf);
if (len < 0)
len = 0;
if (len >= file_len + 1)
diff --git a/regress/scp-ssh-wrapper.sh b/regress/scp-ssh-wrapper.sh
index 59f1ff63..dd48a482 100644
--- a/regress/scp-ssh-wrapper.sh
+++ b/regress/scp-ssh-wrapper.sh
@@ -51,6 +51,18 @@ badserver_4)
echo "C755 2 file"
echo "X"
;;
+badserver_5)
+ echo "D0555 0 "
+ echo "X"
+ ;;
+badserver_6)
+ echo "D0555 0 ."
+ echo "X"
+ ;;
+badserver_7)
+ echo "C0755 2 extrafile"
+ echo "X"
+ ;;
*)
set -- $arg
shift
diff --git a/regress/scp.sh b/regress/scp.sh
index 57cc7706..104c89e1 100644
--- a/regress/scp.sh
+++ b/regress/scp.sh
@@ -25,6 +25,7 @@ export SCP # used in scp-ssh-wrapper.scp
scpclean() {
rm -rf ${COPY} ${COPY2} ${DIR} ${DIR2}
mkdir ${DIR} ${DIR2}
+ chmod 755 ${DIR} ${DIR2}
}
verbose "$tid: simple copy local file to local file"
@@ -101,7 +102,7 @@ if [ ! -z "$SUDO" ]; then
$SUDO rm ${DIR2}/copy
fi
-for i in 0 1 2 3 4; do
+for i in 0 1 2 3 4 5 6 7; do
verbose "$tid: disallow bad server #$i"
SCPTESTMODE=badserver_$i
export DIR SCPTESTMODE
@@ -113,6 +114,15 @@ for i in 0 1 2 3 4; do
scpclean
$SCP -r $scpopts somehost:${DATA} ${DIR2} >/dev/null 2>/dev/null
[ -d ${DIR}/dotpathdir ] && fail "allows dir creation outside of subdir"
+
+ scpclean
+ $SCP -pr $scpopts somehost:${DATA} ${DIR2} >/dev/null 2>/dev/null
+ [ ! -w ${DIR2} ] && fail "allows target root attribute change"
+
+ scpclean
+ $SCP $scpopts somehost:${DATA} ${DIR2} >/dev/null 2>/dev/null
+ [ -e ${DIR2}/extrafile ] && fail "allows extranous object creation"
+ rm -f ${DIR2}/extrafile
done
verbose "$tid: detect non-directory target"
diff --git a/scp.c b/scp.c
index eb17c341..da1a3a44 100644
--- a/scp.c
+++ b/scp.c
@@ -17,6 +17,7 @@
/*
* Copyright (c) 1999 Theo de Raadt. All rights reserved.
* Copyright (c) 1999 Aaron Campbell. All rights reserved.
+ * Copyright (c) 2019 Harry Sintonen. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -87,6 +88,14 @@
#ifdef HAVE_SYS_TIME_H
# include <sys/time.h>
#endif
+#ifdef USE_SCP_NAME_VALIDATOR
+# include <libgen.h>
+# ifdef USE_SYSTEM_GLOB
+# include <glob.h>
+# else
+# include "openbsd-compat/glob.h"
+# endif
+#endif
#include <sys/wait.h>
#include <sys/uio.h>
@@ -277,6 +286,18 @@ do_cmd(char *host, char *remuser, int port, char *cmd, int *fdin, int *fdout)
close(pout[0]);
dup2(pin[0], 0);
dup2(pout[1], 1);
+ /*
+ * If we're not expecting output to stderr, redirect it to void.
+ * This helps avoiding output manipulation attacks by malicious
+ * servers.
+ */
+ if (!verbose_mode) {
+ int fd = open("/dev/null", O_WRONLY);
+ if (fd != -1) {
+ dup2(fd, 2);
+ close(fd);
+ }
+ }
close(pin[0]);
close(pout[1]);
@@ -380,9 +401,20 @@ int pflag, iamremote, iamrecursive, targetshouldbedirectory;
#define CMDNEEDS 64
char cmd[CMDNEEDS]; /* must hold "rcp -r -p -d\0" */
+#ifdef USE_SCP_NAME_VALIDATOR
+typedef struct {
+ const char *pattern;
+ int depth;
+} SINKDATA;
+#endif
+
int response(void);
void rsource(char *, struct stat *);
+#ifdef USE_SCP_NAME_VALIDATOR
+void sink(int, char *[], SINKDATA *);
+#else
void sink(int, char *[]);
+#endif
void source(int, char *[]);
void tolocal(int, char *[]);
void toremote(int, char *[]);
@@ -536,7 +568,11 @@ main(int argc, char **argv)
}
if (tflag) {
/* Receive data. */
+#ifdef USE_SCP_NAME_VALIDATOR
+ sink(argc, argv, NULL);
+#else
sink(argc, argv);
+#endif
exit(errs != 0);
}
if (argc < 2)
@@ -750,6 +786,9 @@ tolocal(int argc, char **argv)
char *bp, *host = NULL, *src = NULL, *suser = NULL;
arglist alist;
int i, r, sport = -1;
+#ifdef USE_SCP_NAME_VALIDATOR
+ SINKDATA sinkdata;
+#endif
memset(&alist, '\0', sizeof(alist));
alist.list = NULL;
@@ -793,7 +832,13 @@ tolocal(int argc, char **argv)
continue;
}
free(bp);
+#ifdef USE_SCP_NAME_VALIDATOR
+ sinkdata.pattern = basename(xstrdup(src));
+ sinkdata.depth = 0;
+ sink(1, argv + argc - 1, &sinkdata);
+#else
sink(1, argv + argc - 1);
+#endif
(void) close(remin);
remin = remout = -1;
}
@@ -968,8 +1013,71 @@ rsource(char *name, struct stat *statp)
(sizeof(type) == 8 && (val) > INT64_MAX) || \
(sizeof(type) != 4 && sizeof(type) != 8))
+#ifdef USE_SCP_NAME_VALIDATOR
+#ifdef GLOB_ALTDIRFUNC
+struct fakedir {
+ struct dirent de;
+#ifdef BROKEN_ONE_BYTE_DIRENT_D_NAME
+ char denamebuf[256];
+#endif
+ struct dirent tmpde;
+#ifdef BROKEN_ONE_BYTE_DIRENT_D_NAME
+ char tmpdenamebuf[2]; /* only needs to hold "." or ".." */
+#endif
+ int dirindex;
+};
+static struct fakedir fakedir;
+static void
+g_closedir(void *ptr)
+{
+}
+static struct dirent *
+g_readdir(void *ptr)
+{
+ struct fakedir *fd = ptr;
+ switch (fd->dirindex) {
+ case 1:
+ case 2:
+ strcpy(fd->tmpde.d_name, fd->dirindex == 1 ? "." : "..");
+ fd->tmpde.d_type = DT_DIR;
+ fd->tmpde.d_ino = fd->dirindex++;
+ return &fd->tmpde;
+ case 3:
+ fd->de.d_ino = fd->dirindex++;
+ return &fd->de;
+ }
+ return NULL;
+}
+static void *
+g_opendir(const char *name)
+{
+ if (strcmp(name, ".") != 0) {
+ errno = ENOENT;
+ return NULL;
+ }
+ fakedir.dirindex = 1;
+ return &fakedir;
+}
+static int
+g_stat(const char *name, struct stat *st)
+{
+ if (strcmp(name, fakedir.de.d_name) != 0) {
+ errno = ENOENT;
+ return -1;
+ }
+ memset(st, 0, sizeof(*st));
+ st->st_mode = fakedir.de.d_type == DT_DIR ? S_IFDIR : S_IFREG;
+ return 0;
+}
+#endif
+#endif
+
void
+#ifdef USE_SCP_NAME_VALIDATOR
+sink(int argc, char **argv, SINKDATA *sinkdata)
+#else
sink(int argc, char **argv)
+#endif
{
static BUF buffer;
struct stat stb;
@@ -1113,6 +1221,65 @@ sink(int argc, char **argv)
run_err("error: unexpected filename: %s", cp);
exit(1);
}
+
+#ifdef USE_SCP_NAME_VALIDATOR
+ if (sinkdata) {
+ /*
+ * Validate the item name returned by the server for
+ * attempts to modify the current directory attributes.
+ *
+ * Only allow it on root level and only if it was
+ * explicitly requested by using "host:" or "dirname/."
+ */
+ if (strcmp(cp, ".") == 0 &&
+ (sinkdata->depth != 0 || strcmp(sinkdata->pattern, ".") != 0)) {
+ run_err("error: unexpected filename: %s", cp);
+ exit(1);
+ }
+ }
+#ifdef GLOB_ALTDIRFUNC
+ /* Use glob(3) function to validate the item name against
+ * the last path element (stored in sinkdata->pattern).
+ *
+ * We verify that the items returned at the target
+ * directory level (depth 0) match this pattern.
+ *
+ * While a limited check, it will prevent some of the
+ * potential attacks by a malicious server.
+ */
+ if (sinkdata && sinkdata->depth == 0) {
+ glob_t gl;
+ int rc;
+#ifdef BROKEN_ONE_BYTE_DIRENT_D_NAME
+ if (strlen(cp) >= 256) {
+#else
+ if (strlen(cp) >= sizeof(fakedir.de.d_name)) {
+#endif
+ run_err("error: excessively long filename: %s", cp);
+ exit(1);
+ }
+ fakedir.de.d_type = buf[0] == 'D' ? DT_DIR : DT_REG;
+ strcpy(fakedir.de.d_name, cp);
+
+ memset(&gl, 0, sizeof(gl));
+ gl.gl_closedir = g_closedir;
+ gl.gl_readdir = g_readdir;
+ gl.gl_opendir = g_opendir;
+ gl.gl_lstat = g_stat;
+ gl.gl_stat = g_stat;
+
+ rc = glob(sinkdata->pattern, GLOB_ALTDIRFUNC|GLOB_NOSORT, NULL, &gl);
+ globfree(&gl);
+ if (rc != 0) {
+ if (rc == GLOB_NOMATCH)
+ run_err("error: unexpected filename: %s", cp);
+ else
+ run_err("error: glob error %d", rc);
+ exit(1);
+ }
+ }
+#endif
+#endif
if (targisdir) {
static char *namebuf;
static size_t cursize;
@@ -1150,7 +1317,15 @@ sink(int argc, char **argv)
goto bad;
}
vect[0] = xstrdup(np);
+#ifdef USE_SCP_NAME_VALIDATOR
+ if (sinkdata)
+ sinkdata->depth++;
+ sink(1, vect, sinkdata);
+ if (sinkdata)
+ sinkdata->depth--;
+#else
sink(1, vect);
+#endif
if (setimes) {
setimes = 0;
if (utimes(vect[0], tv) < 0)