nhyatt/putty-source
1
0
Fork 0
mirror of https://git.tartarus.org/simon/putty.git synced 2025-01-09 17:38:00 +00:00
Code Issues Projects Releases Wiki Activity

File transfer tools: sanitise remote filenames and stderr.

Browse Source 
This commit adds sanitisation to PSCP and PSFTP in the same style as
I've just put it into Plink. This time, standard error is sanitised
without reference to whether it's redirected (at least unless you give
an override option), on the basis that where Plink is _sometimes_ an
SSH transport for some other protocol, PSCP and PSFTP _always_ are.

But also, the sanitiser is run over any remote filename sent by the
server, substituting ? for any control characters it finds. That
removes another avenue for the server to deliberately confuse the
display.

This commit fixes our bug 'pscp-unsanitised-server-output', aka the
two notional 'vulnerabilities' CVE-2019-6109 and CVE-2019-6110.
(Although we regard those in isolation as only bugs, not serious
vulnerabilities, because their main threat was in hiding the evidence
of a server having exploited other more serious vulns that we never
had.)
This commit is contained in:
Simon Tatham 2019-02-20 07:09:10 +00:00
parent 91cf47dd0d
commit 2675f9578d
5 changed files with 186 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
Recipe
View File

@ -281,7 +281,7 @@ WINSSH = SSH winnoise wincapi winpgntc wingss winshare winnps winnpc
UXSSH = SSH uxnoise uxagentc uxgss uxshare
# SFTP implementation (pscp, psftp).
SFTP = sftp sftpcommon logging cmdline
SFTP = sftp sftpcommon logging cmdline stripctrl
# Miscellaneous objects appearing in all the utilities, or all the
# network ones, or the Unix or Windows subsets of those in turn.

10
doc/pscp.but
View File

@ -63,6 +63,7 @@ use PSCP:
\c -hostkey aa:bb:cc:...
\c manually specify a host key (may be repeated)
\c -batch disable all interactive prompts
\c -no-sanitise-stderr don't strip control chars from standard error
\c -proxycmd command
\c use 'command' as local proxy
\c -unsafe allow server-side wildcards (DANGEROUS)
@ -281,6 +282,15 @@ The \c{-sftp} option forces PSCP to use the SFTP protocol or quit.
When this option is specified, PSCP looks harder for an SFTP server,
which may allow use of SFTP with SSH-1 depending on server setup.
\S2{pscp-option-sanitise} \I{-sanitise-stderr}\I{-no-sanitise-stderr}\c{-no-sanitise-stderr}: control error message sanitisation
The \c{-no-sanitise-stderr} option will cause PSCP to pass through the
server's standard-error stream literally, without stripping control
characters from it first. This might be useful if the server were
sending coloured error messages, but it also gives the server the
ability to have unexpected effects on your terminal display. For more
discussion, see \k{plink-option-sanitise}.
\S{pscp-retval} \ii{Return value}
PSCP returns an \i\cw{ERRORLEVEL} of zero (success) only if the files

9
doc/psftp.but
View File

@ -135,6 +135,15 @@ This may help PSFTP's behaviour when it is used in automated
scripts: using \c{-batch}, if something goes wrong at connection
time, the batch job will fail rather than hang.
\S2{psftp-option-sanitise} \I{-sanitise-stderr}\I{-no-sanitise-stderr}\c{-no-sanitise-stderr}: control error message sanitisation
The \c{-no-sanitise-stderr} option will cause PSFTP to pass through the
server's standard-error stream literally, without stripping control
characters from it first. This might be useful if the server were
sending coloured error messages, but it also gives the server the
ability to have unexpected effects on your terminal display. For more
discussion, see \k{plink-option-sanitise}.
\H{psftp-commands} Running PSFTP
Once you have started your PSFTP session, you will see a \c{psftp>}

109
pscp.c
View File

@ -145,17 +145,16 @@ void agent_schedule_callback(void (*callback)(void *, void *, int),
*/
static bufchain received_data;
static BinarySink *stderr_bs;
static size_t pscp_output(
Seat *seat, bool is_stderr, const void *data, size_t len)
{
/*
* stderr data is just spouted to local stderr and otherwise
* ignored.
* stderr data is just spouted to local stderr (optionally via a
* sanitiser) and otherwise ignored.
*/
if (is_stderr) {
if (len > 0)
if (fwrite(data, 1, len, stderr) < len)
/* oh well */;
put_data(stderr_bs, data, len);
return 0;
}
@ -251,6 +250,15 @@ static NORETURN void bump(const char *fmt, ...)
cleanup_exit(1);
}
/*
* A nasty loop macro that lets me get an escape-sequence sanitised
* version of a string for display, and free it automatically
* afterwards.
*/
#define with_stripctrl(varname, input) \
for (char *varname = stripctrl_string(input); varname; \
sfree(varname), varname = NULL)
/*
* Wait for the reply to a single SFTP request. Parallels the same
* function in psftp.c (but isn't centralised into sftp.c because the
@ -664,8 +672,10 @@ void scp_sftp_listdir(const char *dirname)
/*
* And print them.
*/
for (i = 0; i < nnames; i++)
printf("%s\n", ournames[i].longname);
for (i = 0; i < nnames; i++) {
with_stripctrl(san, ournames[i].longname)
printf("%s\n", san);
}
sfree(ournames);
}
@ -1175,7 +1185,8 @@ int scp_get_sink_action(struct scp_sink_action *act)
ret = fxp_stat_recv(pktin, req, &attrs);
if (!ret || !(attrs.flags & SSH_FILEXFER_ATTR_PERMISSIONS)) {
tell_user(stderr, "unable to identify %s: %s", fname,
with_stripctrl(san, fname)
tell_user(stderr, "unable to identify %s: %s", san,
ret ? "file type not supplied" : fxp_error());
if (must_free_fname) sfree(fname);
errs++;
@ -1202,7 +1213,8 @@ int scp_get_sink_action(struct scp_sink_action *act)
* things matching the wildcard.
*/
if (!scp_sftp_recursive && !scp_sftp_wildcard) {
tell_user(stderr, "pscp: %s: is a directory", fname);
with_stripctrl(san, fname)
tell_user(stderr, "pscp: %s: is a directory", san);
errs++;
if (must_free_fname) sfree(fname);
if (scp_sftp_dirstack_head) {
@ -1231,8 +1243,9 @@ int scp_get_sink_action(struct scp_sink_action *act)
dirhandle = fxp_opendir_recv(pktin, req);
if (!dirhandle) {
with_stripctrl(san, fname)
tell_user(stderr, "pscp: unable to open directory %s: %s",
fname, fxp_error());
san, fxp_error());
if (must_free_fname) sfree(fname);
errs++;
return 1;
@ -1249,8 +1262,9 @@ int scp_get_sink_action(struct scp_sink_action *act)
if (names == NULL) {
if (fxp_error_type() == SSH_FX_EOF)
break;
with_stripctrl(san, fname)
tell_user(stderr, "pscp: reading directory %s: %s",
fname, fxp_error());
san, fxp_error());
req = fxp_close_send(dirhandle);
pktin = sftp_wait_for_reply(req);
@ -1278,9 +1292,9 @@ int scp_get_sink_action(struct scp_sink_action *act)
* complaining about.
*/
} else if (!vet_filename(names->names[i].filename)) {
tell_user(stderr, "ignoring potentially dangerous server-"
"supplied filename '%s'",
names->names[i].filename);
with_stripctrl(san, names->names[i].filename)
tell_user(stderr, "ignoring potentially dangerous "
"server-supplied filename '%s'", san);
} else
ournames[nnames++] = names->names[i];
}
@ -1382,11 +1396,13 @@ int scp_get_sink_action(struct scp_sink_action *act)
act->buf[i - 1] = '\0';
switch (action) {
case '\01': /* error */
tell_user(stderr, "%s", act->buf);
with_stripctrl(san, act->buf)
tell_user(stderr, "%s", san);
errs++;
continue; /* go round again */
case '\02': /* fatal error */
bump("%s", act->buf);
with_stripctrl(san, act->buf)
bump("%s", san);
case 'E':
backend_send(backend, "", 1);
act->action = SCP_SINK_ENDDIR;
@ -1442,8 +1458,9 @@ int scp_accept_filexfer(void)
scp_sftp_filehandle = fxp_open_recv(pktin, req);
if (!scp_sftp_filehandle) {
with_stripctrl(san, scp_sftp_currentname)
tell_user(stderr, "pscp: unable to open %s: %s",
scp_sftp_currentname, fxp_error());
san, fxp_error());
errs++;
return 1;
}
@ -1787,10 +1804,14 @@ static void sink(const char *targ, const char *src)
striptarget = stripslashes(act.name, true);
if (striptarget != act.name) {
tell_user(stderr, "warning: remote host sent a compound"
" pathname '%s'", act.name);
tell_user(stderr, " renaming local file to '%s'",
striptarget);
with_stripctrl(sanname, act.name) {
with_stripctrl(santarg, act.name) {
tell_user(stderr, "warning: remote host sent a"
" compound pathname '%s'", sanname);
tell_user(stderr, " renaming local",
" file to '%s'", santarg);
}
}
}
/*
@ -1807,8 +1828,9 @@ static void sink(const char *targ, const char *src)
stripsrc = stripslashes(src, true);
if (strcmp(striptarget, stripsrc) &&
!using_sftp && !scp_unsafe_mode) {
tell_user(stderr, "warning: remote host tried to write "
"to a file called '%s'", striptarget);
with_stripctrl(san, striptarget)
tell_user(stderr, "warning: remote host tried to "
"write to a file called '%s'", san);
tell_user(stderr, " when we requested a file "
"called '%s'.", stripsrc);
tell_user(stderr, " If this is a wildcard, "
@ -1837,13 +1859,15 @@ static void sink(const char *targ, const char *src)
if (act.action == SCP_SINK_DIR) {
if (exists && attr != FILE_TYPE_DIRECTORY) {
run_err("%s: Not a directory", destfname);
with_stripctrl(san, destfname)
run_err("%s: Not a directory", san);
sfree(destfname);
continue;
}
if (!exists) {
if (!create_directory(destfname)) {
run_err("%s: Cannot create directory", destfname);
with_stripctrl(san, destfname)
run_err("%s: Cannot create directory", san);
sfree(destfname);
continue;
}
@ -1856,7 +1880,8 @@ static void sink(const char *targ, const char *src)
f = open_new_file(destfname, act.permissions);
if (f == NULL) {
run_err("%s: Cannot create file", destfname);
with_stripctrl(san, destfname)
run_err("%s: Cannot create file", san);
sfree(destfname);
continue;
}
@ -1870,7 +1895,7 @@ static void sink(const char *targ, const char *src)
stat_bytes = 0;
stat_starttime = time(NULL);
stat_lasttime = 0;
stat_name = stripslashes(destfname, true);
stat_name = stripctrl_string(stripslashes(destfname, true));
received = 0;
while (received < act.size) {
@ -1914,11 +1939,13 @@ static void sink(const char *targ, const char *src)
close_wfile(f);
if (wrerror) {
run_err("%s: Write error", destfname);
with_stripctrl(san, destfname)
run_err("%s: Write error", san);
sfree(destfname);
continue;
}
(void) scp_finish_filerecv();
sfree(stat_name);
sfree(destfname);
sfree(act.buf);
}
@ -2133,8 +2160,13 @@ static void get_dir_list(int argc, char *argv[])
if (using_sftp) {
scp_sftp_listdir(src);
} else {
stdio_sink ss;
stdio_sink_init(&ss, stdout);
StripCtrlChars *scc = stripctrl_new(
BinarySink_UPCAST(&ss), false, L'\0');
while (ssh_scp_recv(&c, 1))
tell_char(stdout, c);
put_byte(scc, c);
stripctrl_free(scc);
}
}
@ -2169,9 +2201,13 @@ static void usage(void)
printf(" -hostkey aa:bb:cc:...\n");
printf(" manually specify a host key (may be repeated)\n");
printf(" -batch disable all interactive prompts\n");
printf(" -no-sanitise-stderr don't strip control chars from"
" standard error\n");
printf(" -proxycmd command\n");
printf(" use 'command' as local proxy\n");
printf(" -unsafe allow server-side wildcards (DANGEROUS)\n");
printf(" -no-sanitise-stderr allow escape sequences in error "
"messages\n");
printf(" -sftp force use of SFTP protocol\n");
printf(" -scp force use of SCP protocol\n");
printf(" -sshlog file\n");
@ -2202,6 +2238,9 @@ void cmdline_error(const char *p, ...)
const bool share_can_be_downstream = true;
const bool share_can_be_upstream = false;
static stdio_sink stderr_ss;
static StripCtrlChars *stderr_scc;
/*
* Main program. (Called `psftp_main' because it gets called from
* *sftp.c; bit silly, I know, but it had to be called _something_.)
@ -2209,6 +2248,7 @@ const bool share_can_be_upstream = false;
int psftp_main(int argc, char *argv[])
{
int i;
bool sanitise_stderr = true;
default_protocol = PROT_TELNET;
@ -2264,6 +2304,10 @@ int psftp_main(int argc, char *argv[])
try_scp = false; try_sftp = true;
} else if (strcmp(argv[i], "-scp") == 0) {
try_scp = true; try_sftp = false;
} else if (strcmp(argv[i], "-sanitise-stderr") == 0) {
sanitise_stderr = true;
} else if (strcmp(argv[i], "-no-sanitise-stderr") == 0) {
sanitise_stderr = false;
} else if (strcmp(argv[i], "--") == 0) {
i++;
break;
@ -2275,6 +2319,13 @@ int psftp_main(int argc, char *argv[])
argv += i;
backend = NULL;
stdio_sink_init(&stderr_ss, stderr);
stderr_bs = BinarySink_UPCAST(&stderr_ss);
if (sanitise_stderr) {
stderr_scc = stripctrl_new(stderr_bs, false, L'\0');
stderr_bs = BinarySink_UPCAST(stderr_scc);
}
if (list) {
if (argc != 1)
usage();

108
psftp.c
View File

@ -64,6 +64,15 @@ static const SeatVtable psftp_seat_vt = {
};
static Seat psftp_seat[1] = {{ &psftp_seat_vt }};
/* ----------------------------------------------------------------------
* A nasty loop macro that lets me get an escape-sequence sanitised
* version of a string for display, and free it automatically
* afterwards.
*/
#define with_stripctrl(varname, input) \
for (char *varname = stripctrl_string(input); varname; \
sfree(varname), varname = NULL)
/* ----------------------------------------------------------------------
* Manage sending requests and waiting for replies.
*/
@ -266,7 +275,8 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
*/
if (file_type(outfname) != FILE_TYPE_DIRECTORY &&
!create_directory(outfname)) {
printf("%s: Cannot create directory\n", outfname);
with_stripctrl(san, outfname)
printf("%s: Cannot create directory\n", san);
return false;
}
@ -279,8 +289,9 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
dirhandle = fxp_opendir_recv(pktin, req);
if (!dirhandle) {
with_stripctrl(san, fname)
printf("%s: unable to open directory: %s\n",
fname, fxp_error());
san, fxp_error());
return false;
}
nnames = namesize = 0;
@ -295,7 +306,9 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
if (names == NULL) {
if (fxp_error_type() == SSH_FX_EOF)
break;
printf("%s: reading directory: %s\n", fname, fxp_error());
with_stripctrl(san, fname)
printf("%s: reading directory: %s\n",
san, fxp_error());
req = fxp_close_send(dirhandle);
pktin = sftp_wait_for_reply(req);
@ -316,9 +329,9 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
if (strcmp(names->names[i].filename, ".") &&
strcmp(names->names[i].filename, "..")) {
if (!vet_filename(names->names[i].filename)) {
with_stripctrl(san, names->names[i].filename)
printf("ignoring potentially dangerous server-"
"supplied filename '%s'\n",
names->names[i].filename);
"supplied filename '%s'\n", san);
} else {
ournames[nnames++] =
fxp_dup_name(&names->names[i]);
@ -414,7 +427,8 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
fh = fxp_open_recv(pktin, req);
if (!fh) {
printf("%s: open for read: %s\n", fname, fxp_error());
with_stripctrl(san, fname)
printf("%s: open for read: %s\n", san, fxp_error());
return false;
}
@ -425,7 +439,8 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
}
if (!file) {
printf("local: unable to open %s\n", outfname);
with_stripctrl(san, outfname)
printf("local: unable to open %s\n", san);
req = fxp_close_send(fh);
pktin = sftp_wait_for_reply(req);
@ -437,8 +452,8 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
if (restart) {
if (seek_file(file, 0, FROM_END) == -1) {
close_wfile(file);
printf("reget: cannot restart %s - file too large\n",
outfname);
with_stripctrl(san, outfname)
printf("reget: cannot restart %s - file too large\n", san);
req = fxp_close_send(fh);
pktin = sftp_wait_for_reply(req);
fxp_close_recv(pktin, req);
@ -452,7 +467,10 @@ bool sftp_get_file(char *fname, char *outfname, bool recurse, bool restart)
offset = 0;
}
printf("remote:%s => local:%s\n", fname, outfname);
with_stripctrl(san, fname) {
with_stripctrl(sano, outfname)
printf("remote:%s => local:%s\n", san, sano);
}
/*
* FIXME: we can use FXP_FSTAT here to get the file size, and
@ -839,9 +857,11 @@ char *sftp_wildcard_get_filename(SftpWildcardMatcher *swcm)
swcm->names = fxp_readdir_recv(pktin, req);
if (!swcm->names) {
if (fxp_error_type() != SSH_FX_EOF)
printf("%s: reading directory: %s\n", swcm->prefix,
fxp_error());
if (fxp_error_type() != SSH_FX_EOF) {
with_stripctrl(san, swcm->prefix)
printf("%s: reading directory: %s\n",
san, fxp_error());
}
return NULL;
} else if (swcm->names->nnames == 0) {
/*
@ -866,8 +886,9 @@ char *sftp_wildcard_get_filename(SftpWildcardMatcher *swcm)
continue; /* expected bad filenames */
if (!vet_filename(name->filename)) {
with_stripctrl(san, name->filename)
printf("ignoring potentially dangerous server-"
"supplied filename '%s'\n", name->filename);
"supplied filename '%s'\n", san);
continue; /* unexpected bad filename */
}
@ -1060,7 +1081,8 @@ int sftp_cmd_ls(struct sftp_command *cmd)
cdir = canonify(dir);
printf("Listing directory %s\n", cdir);
with_stripctrl(san, cdir)
printf("Listing directory %s\n", san);
req = fxp_opendir_send(cdir);
pktin = sftp_wait_for_reply(req);
@ -1118,7 +1140,8 @@ int sftp_cmd_ls(struct sftp_command *cmd)
* And print them.
*/
for (i = 0; i < nnames; i++) {
printf("%s\n", ournames[i]->longname);
with_stripctrl(san, ournames[i]->longname)
printf("%s\n", san);
fxp_free_name(ournames[i]);
}
sfree(ournames);
@ -1157,7 +1180,8 @@ int sftp_cmd_cd(struct sftp_command *cmd)
dirh = fxp_opendir_recv(pktin, req);
if (!dirh) {
printf("Directory %s: %s\n", dir, fxp_error());
with_stripctrl(san, dir)
printf("Directory %s: %s\n", san, fxp_error());
sfree(dir);
return 0;
}
@ -1168,7 +1192,8 @@ int sftp_cmd_cd(struct sftp_command *cmd)
sfree(pwd);
pwd = dir;
printf("Remote directory is now %s\n", pwd);
with_stripctrl(san, pwd)
printf("Remote directory is now %s\n", san);
return 1;
}
@ -1183,7 +1208,8 @@ int sftp_cmd_pwd(struct sftp_command *cmd)
return 0;
}
printf("Remote directory is %s\n", pwd);
with_stripctrl(san, pwd)
printf("Remote directory is %s\n", san);
return 1;
}
@ -1423,10 +1449,12 @@ int sftp_cmd_mkdir(struct sftp_command *cmd)
result = fxp_mkdir_recv(pktin, req);
if (!result) {
printf("mkdir %s: %s\n", dir, fxp_error());
with_stripctrl(san, dir)
printf("mkdir %s: %s\n", san, fxp_error());
ret = 0;
} else
printf("mkdir %s: OK\n", dir);
with_stripctrl(san, dir)
printf("mkdir %s: OK\n", san);
sfree(dir);
}
@ -1571,10 +1599,12 @@ static bool sftp_action_mv(void *vctx, char *srcfname)
error = result ? NULL : fxp_error();
if (error) {
printf("mv %s %s: %s\n", srcfname, finalfname, error);
with_stripctrl(san, finalfname)
printf("mv %s %s: %s\n", srcfname, san, error);
toret = false;
} else {
printf("%s -> %s\n", srcfname, finalfname);
with_stripctrl(san, finalfname)
printf("%s -> %s\n", srcfname, san);
toret = true;
}
@ -2332,7 +2362,8 @@ static int do_sftp_init(void)
fxp_error());
homedir = dupstr(".");
} else {
printf("Remote working directory is %s\n", homedir);
with_stripctrl(san, homedir)
printf("Remote working directory is %s\n", san);
}
pwd = dupstr(homedir);
return 0;
@ -2444,19 +2475,17 @@ void agent_schedule_callback(void (*callback)(void *, void *, int),
* own psftp_output() function to catch the data that comes back. We
* do this until we have enough data.
*/
static bufchain received_data;
static BinarySink *stderr_bs;
static size_t psftp_output(
Seat *seat, bool is_stderr, const void *data, size_t len)
{
/*
* stderr data is just spouted to local stderr and otherwise
* ignored.
* stderr data is just spouted to local stderr (optionally via a
* sanitiser) and otherwise ignored.
*/
if (is_stderr) {
if (len > 0)
if (fwrite(data, 1, len, stderr) < len)
/* oh well */;
put_data(stderr_bs, data, len);
return 0;
}
@ -2532,8 +2561,12 @@ static void usage(void)
printf(" -hostkey aa:bb:cc:...\n");
printf(" manually specify a host key (may be repeated)\n");
printf(" -batch disable all interactive prompts\n");
printf(" -no-sanitise-stderr don't strip control chars from"
" standard error\n");
printf(" -proxycmd command\n");
printf(" use 'command' as local proxy\n");
printf(" -no-sanitise-stderr allow escape sequences in error "
"messages\n");
printf(" -sshlog file\n");
printf(" -sshrawlog file\n");
printf(" log protocol details to a file\n");
@ -2754,6 +2787,9 @@ void cmdline_error(const char *p, ...)
const bool share_can_be_downstream = true;
const bool share_can_be_upstream = false;
static stdio_sink stderr_ss;
static StripCtrlChars *stderr_scc;
/*
* Main program. Parse arguments etc.
*/
@ -2764,6 +2800,7 @@ int psftp_main(int argc, char *argv[])
char *userhost, *user;
int mode = 0;
int modeflags = 0;
bool sanitise_stderr = true;
char *batchfile = NULL;
flags = FLAG_INTERACTIVE
@ -2818,6 +2855,10 @@ int psftp_main(int argc, char *argv[])
modeflags = modeflags | 1;
} else if (strcmp(argv[i], "-be") == 0) {
modeflags = modeflags | 2;
} else if (strcmp(argv[i], "-sanitise-stderr") == 0) {
sanitise_stderr = true;
} else if (strcmp(argv[i], "-no-sanitise-stderr") == 0) {
sanitise_stderr = false;
} else if (strcmp(argv[i], "--") == 0) {
i++;
break;
@ -2829,6 +2870,13 @@ int psftp_main(int argc, char *argv[])
argv += i;
backend = NULL;
stdio_sink_init(&stderr_ss, stderr);
stderr_bs = BinarySink_UPCAST(&stderr_ss);
if (sanitise_stderr) {
stderr_scc = stripctrl_new(stderr_bs, false, L'\0');
stderr_bs = BinarySink_UPCAST(stderr_scc);
}
/*
* If the loaded session provides a hostname, and a hostname has not
* otherwise been specified, pop it in `userhost' so that