Bug 1094 - Local to local copy (and also remote to remote) uses shell expansion twice
Summary: Local to local copy (and also remote to remote) uses shell expansion twice
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: scp (show other bugs)
Version: 4.2p1
Hardware: All Linux
: P2 security
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-09-28 23:03 AEST by Tomas Mraz
Modified: 2023-01-13 13:57 AEDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (4.37 KB, patch)
2005-09-29 17:44 AEST, Tomas Mraz
no flags Details | Diff
rework patch for OpenBSD, with djm. (8.45 KB, patch)
2006-01-26 12:01 AEDT, Darren Tucker
no flags Details | Diff
Same patch as #1053 for OpenSSH 4.2p1 (8.67 KB, patch)
2006-01-26 12:02 AEDT, Darren Tucker
no flags Details | Diff
Regress test for this bug (719 bytes, patch)
2006-01-26 12:03 AEDT, Darren Tucker
djm: ok+
Details | Diff
regress test for normal local/local copies (1.44 KB, patch)
2006-01-26 20:50 AEDT, Damien Miller
dtucker: ok+
Details | Diff
Updated patch for OpenBSD (8.52 KB, patch)
2006-01-26 20:54 AEDT, Damien Miller
no flags Details | Diff
OpenBSD patch v.3 (8.65 KB, patch)
2006-01-26 21:11 AEDT, Damien Miller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Mraz 2005-09-28 23:03:32 AEST
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.
Comment 1 Tomas Mraz 2005-09-29 17:44:15 AEST
Created attachment 971 [details]
Proposed patch

This patch replaces system with fork+exec+waitpid.
Comment 2 Darren Tucker 2006-01-26 12:01:25 AEDT
Created attachment 1053 [details]
rework patch for OpenBSD, with djm.
Comment 3 Darren Tucker 2006-01-26 12:02:42 AEDT
Created attachment 1054 [details]
Same patch as #1053 for OpenSSH 4.2p1
Comment 4 Darren Tucker 2006-01-26 12:03:47 AEDT
Created attachment 1055 [details]
Regress test for this bug
Comment 5 Damien Miller 2006-01-26 20:50:18 AEDT
Created attachment 1056 [details]
regress test for normal local/local copies

Add some regress tests for local -> local copies
Comment 6 Damien Miller 2006-01-26 20:54:23 AEDT
Created attachment 1057 [details]
Updated patch for OpenBSD

Fix a compile problems (bad memset in sftp) and use vasprintf() instead of snprintf/xstrdup
Comment 7 Tomas Mraz 2006-01-26 21:05:41 AEDT
There are two xfree(bp) calls left in your patch (lines 459 an 475 of scp.c) - they must be removed.
Comment 8 Damien Miller 2006-01-26 21:11:05 AEDT
Created attachment 1058 [details]
OpenBSD patch v.3

doh, yes. patch updated
Comment 9 Damien Miller 2006-01-26 21:13:35 AEDT
Comment on attachment 1055 [details]
Regress test for this bug

scpclean should clean up *metachar* too i think
Comment 10 Darren Tucker 2006-01-26 23:07:26 AEDT
(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 11 Damien Miller 2006-01-27 17:48:56 AEDT
Comment on attachment 1055 [details]
Regress test for this bug

oh yes, ok by me to commit after the fix is
Comment 12 Damien Miller 2006-01-31 21:35:32 AEDT
fix and regress test committed, will be in 4.3 (real soon now)
Comment 13 Darren Tucker 2006-02-02 18:28:06 AEDT
For the record, this was CVE-2006-0225.
Comment 14 Darren Tucker 2006-10-07 11:42:27 AEST
Change all RESOLVED bug to CLOSED with the exception of the ones fixed post-4.4.