Bug 2105 - ssh-copy-id can "rm -f *" if mktemp fails
Summary: ssh-copy-id can "rm -f *" if mktemp fails
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: Miscellaneous (show other bugs)
Version: 6.2p1
Hardware: Other Linux
: P5 major
Assignee: Damien Miller
URL: http://bugs.debian.org/708419
Keywords:
Depends on:
Blocks: V_6_2
  Show dependency treegraph
 
Reported: 2013-05-16 03:10 AEST by Colin Watson
Modified: 2015-08-11 23:02 AEST (History)
2 users (show)

See Also:


Attachments
Fix rm bug (811 bytes, patch)
2013-05-16 10:23 AEST, Damien Miller
no flags Details | Diff
Fix usage message (1.02 KB, patch)
2013-05-16 10:31 AEST, Damien Miller
dtucker: ok+
Details | Diff
Use more portable test syntax (1.02 KB, patch)
2013-05-16 11:06 AEST, Damien Miller
dtucker: ok+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Watson 2013-05-16 03:10:53 AEST
Cyril Brulebois filed this bug in Debian:

"""
ssh-copy-id uses "local" while pretending to be POSIX compliant (/bin/sh
as a shebang). Unfortunately, lack of error handling meands that this
isn't caught:
  local L_TMP_ID_FILE=$(mktemp ~/.ssh/ssh-copy-id_id.XXXXXXXXXX)
  trap "rm -f $L_TMP_ID_FILE*" EXIT TERM INT QUIT

mktemp succeeds but dash doesn't like local:
| $ dash
| $ local foo=bar
| dash: 1: local: not in a function
| $ echo $foo
|
| $

Which leads to:
  rm -f *

=> Nasty!

Traced back to 1:6.2p1-1 due to:
     - Update ssh-copy-id to Phil Hands' greatly revised version
       (closes: #99785, #322228, #620428; LP: #518883, #835901, #1074798).

(Tagging "upstream" as it's also mention in upstream's ChangeLog.)

debdiffing both 6.1p1-4 and 1:6.2p1-1 confirms that the regression
was introduced in the latter (there was no "local" before).
"""

I think this may be a misdiagnosis; the "local" here is in a function.  But, regardless of the exact cause, I wanted to make sure this was forwarded to you in time for 6.2p2.
Comment 1 Colin Watson 2013-05-16 06:42:33 AEST
Cyril continues:

OK, trying locally, adding "echo" before the two "rm" (see both lines
with "trap"), and moving ~/.ssh away, I'm getting a mktemp failure,
and "echo rm" prints all the files which would be otherwise deleted.

Running ssh-copy-id from the wrong machine can lead to such an ENOENT
issue; running with the wrong user, or with wrong permissions
(e.g. after a reinstall/restoration from a backup), EPERM is another
likely cause.

Sorry for jumping to the wrong cause, but the conclusion stands.
Comment 2 Damien Miller 2013-05-16 10:23:44 AEST
Created attachment 2269 [details]
Fix rm bug

Check exit status of mktemp and that it returned a filename

Be more specific in which files are deleted
Comment 3 Damien Miller 2013-05-16 10:31:09 AEST
Created attachment 2270 [details]
Fix usage message

Check exit status of mktemp and that it returned a filename

Be more specific in which files are deleted

Print usage only if no arguments specified, rather than "too many arguments"
Comment 4 Darren Tucker 2013-05-16 11:06:06 AEST
Comment on attachment 2270 [details]
Fix usage message

>+  if test $? -ne 0 -o "x$L_TMP_ID_FILE" = "x" ; then

I think there are some shells that don't do -o.  We avoid it in configure and instead use something like

test $? -ne 0 || test "x$L_TMP_ID_FILE" = "x"

other than that, ok
Comment 5 Damien Miller 2013-05-16 11:06:28 AEST
Created attachment 2271 [details]
Use more portable test syntax
Comment 6 Darren Tucker 2013-05-16 11:07:26 AEST
we should put this in 6.2p2
Comment 7 Damien Miller 2013-05-16 11:35:58 AEST
Committed. This will be in openssh-6.2p2, due in an hour or so (lucky timing...)
Comment 8 Philip Hands 2013-05-16 12:32:22 AEST
For consistency with the rest of the script, would it be OK to use the following for the error message:

  printf '%s: ERROR: mktemp failed\n' "$0" >&2


Perhaps the error code should also be put into that message, if that's going to be useful for diagnosing the cause of the problem.

Cheers, Phil.
Comment 9 Damien Miller 2013-05-16 14:30:42 AEST
I've already committed the "echo" version for the 6.2p2 release. 

Phil, if you update your git tree with printf then I'll pull the fix from you for CVS HEAD.
Comment 10 Philip Hands 2013-07-27 23:28:00 AEST
(In reply to Damien Miller from comment #9)
> I've already committed the "echo" version for the 6.2p2 release. 
> 
> Phil, if you update your git tree with printf then I'll pull the fix
> from you for CVS HEAD.

Sorry for the delay in getting round to that (took me a while to sort out a git cvsimport so that I could cherry-pick your changes nicely).

That's now done, and the echo converted to a printf on the master branch of my repo.

I don't think this is even slightly urgent, so should perhaps be ignored until after the current release cycle (on the other hand, it is trivial, so could go in if you fancy it).
Comment 11 Damien Miller 2015-08-11 23:02:55 AEST
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1