scp currently implements local-to-local copy by constructing a command line using 'cp' in a string and then using system(). It has the problem that the file name is exposed twice to shell expansion. The file name could contain characters which need quoting, like $ or spaces. This second expansion must be avoided. Steps to Reproduce: 1.touch foo\ bar 2.mkdir somedir 3.scp foo\ bar somedir Actual results: cp: cannot stat `foo': No such file or directory cp: cannot stat `bar': No such file or directory This can be even a security issue although with a fairly low severity: bress@link:/tmp/josh% ls -l total 4 drwxrwxr-x 2 bress bress 4096 Sep 19 14:51 a -rw-rw-r-- 1 bress bress 0 Sep 19 14:51 `touch feh` bress@link:/tmp/josh% scp * a cp: omitting directory `a' cp: missing destination file Try `cp --help' for more information. zsh: exit 1 scp * a bress@link:/tmp/josh% ls -l total 4 drwxrwxr-x 2 bress bress 4096 Sep 19 14:51 a -rw-rw-r-- 1 bress bress 0 Sep 19 14:52 feh -rw-rw-r-- 1 bress bress 0 Sep 19 14:51 `touch feh` Proposed solution: replace system() with fork+exec() I'll attach a patch later.
Created attachment 971 [details] Proposed patch This patch replaces system with fork+exec+waitpid.
Created attachment 1053 [details] rework patch for OpenBSD, with djm.
Created attachment 1054 [details] Same patch as #1053 for OpenSSH 4.2p1
Created attachment 1055 [details] Regress test for this bug
Created attachment 1056 [details] regress test for normal local/local copies Add some regress tests for local -> local copies
Created attachment 1057 [details] Updated patch for OpenBSD Fix a compile problems (bad memset in sftp) and use vasprintf() instead of snprintf/xstrdup
There are two xfree(bp) calls left in your patch (lines 459 an 475 of scp.c) - they must be removed.
Created attachment 1058 [details] OpenBSD patch v.3 doh, yes. patch updated
Comment on attachment 1055 [details] Regress test for this bug scpclean should clean up *metachar* too i think
(In reply to comment #9) > scpclean should clean up *metachar* too i think Not necessary: it's created in a scratch directory that's deleted entirely by scpclean.
Comment on attachment 1055 [details] Regress test for this bug oh yes, ok by me to commit after the fix is
fix and regress test committed, will be in 4.3 (real soon now)
For the record, this was CVE-2006-0225.
Change all RESOLVED bug to CLOSED with the exception of the ones fixed post-4.4.