On Sat, 19 Jan 2019, Johnsen XXXXX wrote:
> Hello,
>
> I would like to request an update of the progress regarding fixes
> for the recently disclosed SCP vulnerability (CVE-2018-20685,
> CVE-2019-6111, CVE-2019-6109, CVE-2019-6110)
IMO these are all fairly intrinsic to the way that rcp/scp works. It's
a crappy zombie protocol that shambled out of the 1980s. Don't use
scp with untrusted servers.
We've fixed one of these in git HEAD in 6010c030 and a fix for the
wildcard thing is below. We're a bit wary though because, despite it
being terrible, scp use is common in scripts and we don't want to break
those. Testing wanted and hopefully it will make the openssh-8.0
release.
We don't consider the report relating to stderr to be a vulnerability -
lots of stuff depends on stderr being present (e.g. login warning
banners that some people inexplicably love) and it's impractical for
scp to selectively process them. The machine you just logged into can
print junk to your screen, so what?
We consider the last bug, relating to filename printing to be a
usability problem. The reporters' patch is incorrect though, we had
developed a similar one at https://bugzilla.mindrot.org/b/2434 and
rejected it because it attempts unsafe operations in signal context.
We'll hopefully have a fix for that in 8.0 too.
-d
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..f60112bb 100644
--- a/scp.c
+++ b/scp.c
@@ -94,6 +94,7 @@
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
+#include <fnmatch.h>
#include <limits.h>
#include <locale.h>
#include <pwd.h>
@@ -375,14 +376,14 @@ void verifydir(char *);
struct passwd *pwd;
uid_t userid;
int errs, remin, remout;
-int pflag, iamremote, iamrecursive, targetshouldbedirectory;
+int Tflag, pflag, iamremote, iamrecursive, targetshouldbedirectory;
#define CMDNEEDS 64
char cmd[CMDNEEDS]; /* must hold "rcp -r -p -d\0" */
int response(void);
void rsource(char *, struct stat *);
-void sink(int, char *[]);
+void sink(int, char *[], const char *);
void source(int, char *[]);
void tolocal(int, char *[]);
void toremote(int, char *[]);
@@ -423,8 +424,8 @@ main(int argc, char **argv)
addargs(&args, "-oRemoteCommand=none");
addargs(&args, "-oRequestTTY=no");
- fflag = tflag = 0;
- while ((ch = getopt(argc, argv, "dfl:prtvBCc:i:P:q12346S:o:F:")) !=
-1)
+ fflag = Tflag = tflag = 0;
+ while ((ch = getopt(argc, argv, "dfl:prtTvBCc:i:P:q12346S:o:F:")) !=
-1)
switch (ch) {
/* User-visible flags. */
case '1':
@@ -503,6 +504,9 @@ main(int argc, char **argv)
setmode(0, O_BINARY);
#endif
break;
+ case 'T':
+ Tflag = 1;
+ break;
default:
usage();
}
@@ -536,7 +540,7 @@ main(int argc, char **argv)
}
if (tflag) {
/* Receive data. */
- sink(argc, argv);
+ sink(argc, argv, NULL);
exit(errs != 0);
}
if (argc < 2)
@@ -793,7 +797,7 @@ tolocal(int argc, char **argv)
continue;
}
free(bp);
- sink(1, argv + argc - 1);
+ sink(1, argv + argc - 1, src);
(void) close(remin);
remin = remout = -1;
}
@@ -969,7 +973,7 @@ rsource(char *name, struct stat *statp)
(sizeof(type) != 4 && sizeof(type) != 8))
void
-sink(int argc, char **argv)
+sink(int argc, char **argv, const char *src)
{
static BUF buffer;
struct stat stb;
@@ -985,6 +989,7 @@ sink(int argc, char **argv)
unsigned long long ull;
int setimes, targisdir, wrerrno = 0;
char ch, *cp, *np, *targ, *why, *vect[1], buf[2048], visbuf[2048];
+ char *src_copy = NULL, *restrict_pattern = NULL;
struct timeval tv[2];
#define atime tv[0]
@@ -1009,6 +1014,17 @@ sink(int argc, char **argv)
(void) atomicio(vwrite, remout, "", 1);
if (stat(targ, &stb) == 0 && S_ISDIR(stb.st_mode))
targisdir = 1;
+ if (src != NULL && !iamrecursive && !Tflag) {
+ /*
+ * Prepare to try to restrict incoming filenames to match
+ * the requested destination file glob.
+ */
+ if ((src_copy = strdup(src)) == NULL)
+ fatal("strdup failed");
+ if ((restrict_pattern = strrchr(src_copy, '/')) != NULL) {
+ *restrict_pattern++ = '\0';
+ }
+ }
for (first = 1;; first = 0) {
cp = buf;
if (atomicio(read, remin, cp, 1) != 1)
@@ -1113,6 +1129,9 @@ sink(int argc, char **argv)
run_err("error: unexpected filename: %s", cp);
exit(1);
}
+ if (restrict_pattern != NULL &&
+ fnmatch(restrict_pattern, cp, 0) != 0)
+ SCREWUP("filename does not match request");
if (targisdir) {
static char *namebuf;
static size_t cursize;
@@ -1150,7 +1169,7 @@ sink(int argc, char **argv)
goto bad;
}
vect[0] = xstrdup(np);
- sink(1, vect);
+ sink(1, vect, src);
if (setimes) {
setimes = 0;
if (utimes(vect[0], tv) < 0)