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 #endif +#ifdef USE_SCP_NAME_VALIDATOR +# include +# ifdef USE_SYSTEM_GLOB +# include +# else +# include "openbsd-compat/glob.h" +# endif +#endif #include #include @@ -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)