Hi Attached the patch and debdiff for unstable which fixes this issue.
Colin, but please double check if this is enough. A server which sends an additional malicious file is blocked by that (and the patch is not following git-dpm workflow as I'm unfamiliar with it). dummy@sid:~$ scp -P 2222 foo@localhost:test.txt . The authenticity of host '[localhost]:2222 ([127.0.0.1]:2222)' can't be established. RSA key fingerprint is SHA256:BCYLeKMU5zuQ/Xd2Xc8sur4Mp7pQTMHcpwQkfAAmeXM. Are you sure you want to continue connecting (yes/no)? yes Warning: Permanently added '[localhost]:2222' (RSA) to the list of known hosts. foo@localhost's password: test.txt 100% 32 0.7KB/s 00:00 protocol error: filename does not match request dummy@sid:~$ Regards, Salvatore
diff -Nru openssh-7.9p1/debian/changelog openssh-7.9p1/debian/changelog --- openssh-7.9p1/debian/changelog 2019-02-28 20:31:49.000000000 +0100 +++ openssh-7.9p1/debian/changelog 2019-02-28 22:45:36.000000000 +0100 @@ -1,3 +1,11 @@ +openssh (1:7.9p1-8.1) unstable; urgency=medium + + * Non-maintainer upload. + * When checking that filenames sent by the server side match what the client + requested (Closes: #923486) + + -- Salvatore Bonaccorso <car...@debian.org> Thu, 28 Feb 2019 22:45:36 +0100 + openssh (1:7.9p1-8) unstable; urgency=medium [ Colin Watson ] diff -Nru openssh-7.9p1/debian/patches/series openssh-7.9p1/debian/patches/series --- openssh-7.9p1/debian/patches/series 2019-02-28 11:37:30.000000000 +0100 +++ openssh-7.9p1/debian/patches/series 2019-02-28 22:45:36.000000000 +0100 @@ -30,3 +30,4 @@ check-filenames-in-scp-client.patch fix-key-type-check.patch request-rsa-sha2-cert-signatures.patch +upstream-when-checking-that-filenames-sent-by-the-se.patch diff -Nru openssh-7.9p1/debian/patches/upstream-when-checking-that-filenames-sent-by-the-se.patch openssh-7.9p1/debian/patches/upstream-when-checking-that-filenames-sent-by-the-se.patch --- openssh-7.9p1/debian/patches/upstream-when-checking-that-filenames-sent-by-the-se.patch 1970-01-01 01:00:00.000000000 +0100 +++ openssh-7.9p1/debian/patches/upstream-when-checking-that-filenames-sent-by-the-se.patch 2019-02-28 22:45:36.000000000 +0100 @@ -0,0 +1,351 @@ +From: "d...@openbsd.org" <d...@openbsd.org> +Date: Sun, 10 Feb 2019 11:15:52 +0000 +Subject: upstream: when checking that filenames sent by the server side +Origin: https://anongit.mindrot.org/openssh.git/commit/?id=3d896c157c722bc47adca51a58dca859225b5874 +Bug-Debian: https://bugs.debian.org/923486 + +match what the client requested, be prepared to handle shell-style brace +alternations, e.g. "{foo,bar}". + +"looks good to me" millert@ + in snaps for the last week courtesy +deraadt@ + +OpenBSD-Commit-ID: 3b1ce7639b0b25b2248e3a30f561a548f6815f3e +--- + scp.c | 282 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--- + 1 file changed, 270 insertions(+), 12 deletions(-) + +diff --git a/scp.c b/scp.c +index 96fc246cd9ba..80bc0e8b16d1 100644 +--- a/scp.c ++++ b/scp.c +@@ -630,6 +630,253 @@ parse_scp_uri(const char *uri, char **userp, char **hostp, int *portp, + return r; + } + ++/* Appends a string to an array; returns 0 on success, -1 on alloc failure */ ++static int ++append(char *cp, char ***ap, size_t *np) ++{ ++ char **tmp; ++ ++ if ((tmp = reallocarray(*ap, *np + 1, sizeof(*tmp))) == NULL) ++ return -1; ++ tmp[(*np)] = cp; ++ (*np)++; ++ *ap = tmp; ++ return 0; ++} ++ ++/* ++ * Finds the start and end of the first brace pair in the pattern. ++ * returns 0 on success or -1 for invalid patterns. ++ */ ++static int ++find_brace(const char *pattern, int *startp, int *endp) ++{ ++ int i; ++ int in_bracket, brace_level; ++ ++ *startp = *endp = -1; ++ in_bracket = brace_level = 0; ++ for (i = 0; i < INT_MAX && *endp < 0 && pattern[i] != '\0'; i++) { ++ switch (pattern[i]) { ++ case '\\': ++ /* skip next character */ ++ if (pattern[i + 1] != '\0') ++ i++; ++ break; ++ case '[': ++ in_bracket = 1; ++ break; ++ case ']': ++ in_bracket = 0; ++ break; ++ case '{': ++ if (in_bracket) ++ break; ++ if (pattern[i + 1] == '}') { ++ /* Protect a single {}, for find(1), like csh */ ++ i++; /* skip */ ++ break; ++ } ++ if (*startp == -1) ++ *startp = i; ++ brace_level++; ++ break; ++ case '}': ++ if (in_bracket) ++ break; ++ if (*startp < 0) { ++ /* Unbalanced brace */ ++ return -1; ++ } ++ if (--brace_level <= 0) ++ *endp = i; ++ break; ++ } ++ } ++ /* unbalanced brackets/braces */ ++ if (*endp < 0 && (*startp >= 0 || in_bracket)) ++ return -1; ++ return 0; ++} ++ ++/* ++ * Assembles and records a successfully-expanded pattern, returns -1 on ++ * alloc failure. ++ */ ++static int ++emit_expansion(const char *pattern, int brace_start, int brace_end, ++ int sel_start, int sel_end, char ***patternsp, size_t *npatternsp) ++{ ++ char *cp; ++ int o = 0, tail_len = strlen(pattern + brace_end + 1); ++ ++ if ((cp = malloc(brace_start + (sel_end - sel_start) + ++ tail_len + 1)) == NULL) ++ return -1; ++ ++ /* Pattern before initial brace */ ++ if (brace_start > 0) { ++ memcpy(cp, pattern, brace_start); ++ o = brace_start; ++ } ++ /* Current braced selection */ ++ if (sel_end - sel_start > 0) { ++ memcpy(cp + o, pattern + sel_start, ++ sel_end - sel_start); ++ o += sel_end - sel_start; ++ } ++ /* Remainder of pattern after closing brace */ ++ if (tail_len > 0) { ++ memcpy(cp + o, pattern + brace_end + 1, tail_len); ++ o += tail_len; ++ } ++ cp[o] = '\0'; ++ if (append(cp, patternsp, npatternsp) != 0) { ++ free(cp); ++ return -1; ++ } ++ return 0; ++} ++ ++/* ++ * Expand the first encountered brace in pattern, appending the expanded ++ * patterns it yielded to the *patternsp array. ++ * ++ * Returns 0 on success or -1 on allocation failure. ++ * ++ * Signals whether expansion was performed via *expanded and whether ++ * pattern was invalid via *invalid. ++ */ ++static int ++brace_expand_one(const char *pattern, char ***patternsp, size_t *npatternsp, ++ int *expanded, int *invalid) ++{ ++ int i; ++ int in_bracket, brace_start, brace_end, brace_level; ++ int sel_start, sel_end; ++ ++ *invalid = *expanded = 0; ++ ++ if (find_brace(pattern, &brace_start, &brace_end) != 0) { ++ *invalid = 1; ++ return 0; ++ } else if (brace_start == -1) ++ return 0; ++ ++ in_bracket = brace_level = 0; ++ for (i = sel_start = brace_start + 1; i < brace_end; i++) { ++ switch (pattern[i]) { ++ case '{': ++ if (in_bracket) ++ break; ++ brace_level++; ++ break; ++ case '}': ++ if (in_bracket) ++ break; ++ brace_level--; ++ break; ++ case '[': ++ in_bracket = 1; ++ break; ++ case ']': ++ in_bracket = 0; ++ break; ++ case '\\': ++ if (i < brace_end - 1) ++ i++; /* skip */ ++ break; ++ } ++ if (pattern[i] == ',' || i == brace_end - 1) { ++ if (in_bracket || brace_level > 0) ++ continue; ++ /* End of a selection, emit an expanded pattern */ ++ ++ /* Adjust end index for last selection */ ++ sel_end = (i == brace_end - 1) ? brace_end : i; ++ if (emit_expansion(pattern, brace_start, brace_end, ++ sel_start, sel_end, patternsp, npatternsp) != 0) ++ return -1; ++ /* move on to the next selection */ ++ sel_start = i + 1; ++ continue; ++ } ++ } ++ if (in_bracket || brace_level > 0) { ++ *invalid = 1; ++ return 0; ++ } ++ /* success */ ++ *expanded = 1; ++ return 0; ++} ++ ++/* Expand braces from pattern. Returns 0 on success, -1 on failure */ ++static int ++brace_expand(const char *pattern, char ***patternsp, size_t *npatternsp) ++{ ++ char *cp, *cp2, **active = NULL, **done = NULL; ++ size_t i, nactive = 0, ndone = 0; ++ int ret = -1, invalid = 0, expanded = 0; ++ ++ *patternsp = NULL; ++ *npatternsp = 0; ++ ++ /* Start the worklist with the original pattern */ ++ if ((cp = strdup(pattern)) == NULL) ++ return -1; ++ if (append(cp, &active, &nactive) != 0) { ++ free(cp); ++ return -1; ++ } ++ while (nactive > 0) { ++ cp = active[nactive - 1]; ++ nactive--; ++ if (brace_expand_one(cp, &active, &nactive, ++ &expanded, &invalid) == -1) { ++ free(cp); ++ goto fail; ++ } ++ if (invalid) ++ fatal("%s: invalid brace pattern \"%s\"", __func__, cp); ++ if (expanded) { ++ /* ++ * Current entry expanded to new entries on the ++ * active list; discard the progenitor pattern. ++ */ ++ free(cp); ++ continue; ++ } ++ /* ++ * Pattern did not expand; append the finename component to ++ * the completed list ++ */ ++ if ((cp2 = strrchr(cp, '/')) != NULL) ++ *cp2++ = '\0'; ++ else ++ cp2 = cp; ++ if (append(xstrdup(cp2), &done, &ndone) != 0) { ++ free(cp); ++ goto fail; ++ } ++ free(cp); ++ } ++ /* success */ ++ *patternsp = done; ++ *npatternsp = ndone; ++ done = NULL; ++ ndone = 0; ++ ret = 0; ++ fail: ++ for (i = 0; i < nactive; i++) ++ free(active[i]); ++ free(active); ++ for (i = 0; i < ndone; i++) ++ free(done[i]); ++ free(done); ++ return ret; ++} ++ + void + toremote(int argc, char **argv) + { +@@ -993,7 +1240,8 @@ sink(int argc, char **argv, const char *src) + 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; ++ char **patterns = NULL; ++ size_t n, npatterns = 0; + struct timeval tv[2]; + + #define atime tv[0] +@@ -1023,16 +1271,13 @@ sink(int argc, char **argv, const char *src) + * 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'; +- } ++ if (brace_expand(src, &patterns, &npatterns) != 0) ++ fatal("%s: could not expand pattern", __func__); + } + for (first = 1;; first = 0) { + cp = buf; + if (atomicio(read, remin, cp, 1) != 1) +- return; ++ goto done; + if (*cp++ == '\n') + SCREWUP("unexpected <newline>"); + do { +@@ -1058,7 +1303,7 @@ sink(int argc, char **argv, const char *src) + } + if (buf[0] == 'E') { + (void) atomicio(vwrite, remout, "", 1); +- return; ++ goto done; + } + if (ch == '\n') + *--cp = 0; +@@ -1133,9 +1378,14 @@ sink(int argc, char **argv, const char *src) + 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 (npatterns > 0) { ++ for (n = 0; n < npatterns; n++) { ++ if (fnmatch(patterns[n], cp, 0) == 0) ++ break; ++ } ++ if (n >= npatterns) ++ SCREWUP("filename does not match request"); ++ } + if (targisdir) { + static char *namebuf; + static size_t cursize; +@@ -1294,7 +1544,15 @@ bad: run_err("%s: %s", np, strerror(errno)); + break; + } + } ++done: ++ for (n = 0; n < npatterns; n++) ++ free(patterns[n]); ++ free(patterns); ++ return; + screwup: ++ for (n = 0; n < npatterns; n++) ++ free(patterns[n]); ++ free(patterns); + run_err("protocol error: %s", why); + exit(1); + } +-- +2.20.1 +
From: "d...@openbsd.org" <d...@openbsd.org> Date: Sun, 10 Feb 2019 11:15:52 +0000 Subject: upstream: when checking that filenames sent by the server side Origin: https://anongit.mindrot.org/openssh.git/commit/?id=3d896c157c722bc47adca51a58dca859225b5874 Bug-Debian: https://bugs.debian.org/923486 match what the client requested, be prepared to handle shell-style brace alternations, e.g. "{foo,bar}". "looks good to me" millert@ + in snaps for the last week courtesy deraadt@ OpenBSD-Commit-ID: 3b1ce7639b0b25b2248e3a30f561a548f6815f3e --- scp.c | 282 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 270 insertions(+), 12 deletions(-) diff --git a/scp.c b/scp.c index 96fc246cd9ba..80bc0e8b16d1 100644 --- a/scp.c +++ b/scp.c @@ -630,6 +630,253 @@ parse_scp_uri(const char *uri, char **userp, char **hostp, int *portp, return r; } +/* Appends a string to an array; returns 0 on success, -1 on alloc failure */ +static int +append(char *cp, char ***ap, size_t *np) +{ + char **tmp; + + if ((tmp = reallocarray(*ap, *np + 1, sizeof(*tmp))) == NULL) + return -1; + tmp[(*np)] = cp; + (*np)++; + *ap = tmp; + return 0; +} + +/* + * Finds the start and end of the first brace pair in the pattern. + * returns 0 on success or -1 for invalid patterns. + */ +static int +find_brace(const char *pattern, int *startp, int *endp) +{ + int i; + int in_bracket, brace_level; + + *startp = *endp = -1; + in_bracket = brace_level = 0; + for (i = 0; i < INT_MAX && *endp < 0 && pattern[i] != '\0'; i++) { + switch (pattern[i]) { + case '\\': + /* skip next character */ + if (pattern[i + 1] != '\0') + i++; + break; + case '[': + in_bracket = 1; + break; + case ']': + in_bracket = 0; + break; + case '{': + if (in_bracket) + break; + if (pattern[i + 1] == '}') { + /* Protect a single {}, for find(1), like csh */ + i++; /* skip */ + break; + } + if (*startp == -1) + *startp = i; + brace_level++; + break; + case '}': + if (in_bracket) + break; + if (*startp < 0) { + /* Unbalanced brace */ + return -1; + } + if (--brace_level <= 0) + *endp = i; + break; + } + } + /* unbalanced brackets/braces */ + if (*endp < 0 && (*startp >= 0 || in_bracket)) + return -1; + return 0; +} + +/* + * Assembles and records a successfully-expanded pattern, returns -1 on + * alloc failure. + */ +static int +emit_expansion(const char *pattern, int brace_start, int brace_end, + int sel_start, int sel_end, char ***patternsp, size_t *npatternsp) +{ + char *cp; + int o = 0, tail_len = strlen(pattern + brace_end + 1); + + if ((cp = malloc(brace_start + (sel_end - sel_start) + + tail_len + 1)) == NULL) + return -1; + + /* Pattern before initial brace */ + if (brace_start > 0) { + memcpy(cp, pattern, brace_start); + o = brace_start; + } + /* Current braced selection */ + if (sel_end - sel_start > 0) { + memcpy(cp + o, pattern + sel_start, + sel_end - sel_start); + o += sel_end - sel_start; + } + /* Remainder of pattern after closing brace */ + if (tail_len > 0) { + memcpy(cp + o, pattern + brace_end + 1, tail_len); + o += tail_len; + } + cp[o] = '\0'; + if (append(cp, patternsp, npatternsp) != 0) { + free(cp); + return -1; + } + return 0; +} + +/* + * Expand the first encountered brace in pattern, appending the expanded + * patterns it yielded to the *patternsp array. + * + * Returns 0 on success or -1 on allocation failure. + * + * Signals whether expansion was performed via *expanded and whether + * pattern was invalid via *invalid. + */ +static int +brace_expand_one(const char *pattern, char ***patternsp, size_t *npatternsp, + int *expanded, int *invalid) +{ + int i; + int in_bracket, brace_start, brace_end, brace_level; + int sel_start, sel_end; + + *invalid = *expanded = 0; + + if (find_brace(pattern, &brace_start, &brace_end) != 0) { + *invalid = 1; + return 0; + } else if (brace_start == -1) + return 0; + + in_bracket = brace_level = 0; + for (i = sel_start = brace_start + 1; i < brace_end; i++) { + switch (pattern[i]) { + case '{': + if (in_bracket) + break; + brace_level++; + break; + case '}': + if (in_bracket) + break; + brace_level--; + break; + case '[': + in_bracket = 1; + break; + case ']': + in_bracket = 0; + break; + case '\\': + if (i < brace_end - 1) + i++; /* skip */ + break; + } + if (pattern[i] == ',' || i == brace_end - 1) { + if (in_bracket || brace_level > 0) + continue; + /* End of a selection, emit an expanded pattern */ + + /* Adjust end index for last selection */ + sel_end = (i == brace_end - 1) ? brace_end : i; + if (emit_expansion(pattern, brace_start, brace_end, + sel_start, sel_end, patternsp, npatternsp) != 0) + return -1; + /* move on to the next selection */ + sel_start = i + 1; + continue; + } + } + if (in_bracket || brace_level > 0) { + *invalid = 1; + return 0; + } + /* success */ + *expanded = 1; + return 0; +} + +/* Expand braces from pattern. Returns 0 on success, -1 on failure */ +static int +brace_expand(const char *pattern, char ***patternsp, size_t *npatternsp) +{ + char *cp, *cp2, **active = NULL, **done = NULL; + size_t i, nactive = 0, ndone = 0; + int ret = -1, invalid = 0, expanded = 0; + + *patternsp = NULL; + *npatternsp = 0; + + /* Start the worklist with the original pattern */ + if ((cp = strdup(pattern)) == NULL) + return -1; + if (append(cp, &active, &nactive) != 0) { + free(cp); + return -1; + } + while (nactive > 0) { + cp = active[nactive - 1]; + nactive--; + if (brace_expand_one(cp, &active, &nactive, + &expanded, &invalid) == -1) { + free(cp); + goto fail; + } + if (invalid) + fatal("%s: invalid brace pattern \"%s\"", __func__, cp); + if (expanded) { + /* + * Current entry expanded to new entries on the + * active list; discard the progenitor pattern. + */ + free(cp); + continue; + } + /* + * Pattern did not expand; append the finename component to + * the completed list + */ + if ((cp2 = strrchr(cp, '/')) != NULL) + *cp2++ = '\0'; + else + cp2 = cp; + if (append(xstrdup(cp2), &done, &ndone) != 0) { + free(cp); + goto fail; + } + free(cp); + } + /* success */ + *patternsp = done; + *npatternsp = ndone; + done = NULL; + ndone = 0; + ret = 0; + fail: + for (i = 0; i < nactive; i++) + free(active[i]); + free(active); + for (i = 0; i < ndone; i++) + free(done[i]); + free(done); + return ret; +} + void toremote(int argc, char **argv) { @@ -993,7 +1240,8 @@ sink(int argc, char **argv, const char *src) 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; + char **patterns = NULL; + size_t n, npatterns = 0; struct timeval tv[2]; #define atime tv[0] @@ -1023,16 +1271,13 @@ sink(int argc, char **argv, const char *src) * 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'; - } + if (brace_expand(src, &patterns, &npatterns) != 0) + fatal("%s: could not expand pattern", __func__); } for (first = 1;; first = 0) { cp = buf; if (atomicio(read, remin, cp, 1) != 1) - return; + goto done; if (*cp++ == '\n') SCREWUP("unexpected <newline>"); do { @@ -1058,7 +1303,7 @@ sink(int argc, char **argv, const char *src) } if (buf[0] == 'E') { (void) atomicio(vwrite, remout, "", 1); - return; + goto done; } if (ch == '\n') *--cp = 0; @@ -1133,9 +1378,14 @@ sink(int argc, char **argv, const char *src) 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 (npatterns > 0) { + for (n = 0; n < npatterns; n++) { + if (fnmatch(patterns[n], cp, 0) == 0) + break; + } + if (n >= npatterns) + SCREWUP("filename does not match request"); + } if (targisdir) { static char *namebuf; static size_t cursize; @@ -1294,7 +1544,15 @@ bad: run_err("%s: %s", np, strerror(errno)); break; } } +done: + for (n = 0; n < npatterns; n++) + free(patterns[n]); + free(patterns); + return; screwup: + for (n = 0; n < npatterns; n++) + free(patterns[n]); + free(patterns); run_err("protocol error: %s", why); exit(1); } -- 2.20.1