openssh bugs - May 2013 - [Bug 2105] New: ssh-copy-id leads to rm -f * (local vs. dash)

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

            Bug ID: 2105
           Summary: ssh-copy-id leads to rm -f * (local vs. dash)
    Classification: Unclassified
           Product: Portable OpenSSH
           Version: 6.2p1
          Hardware: Other
               URL: http://bugs.debian.org/708419
                OS: Linux
            Status: NEW
          Severity: major
          Priority: P5
         Component: Miscellaneous
          Assignee: unassigned-bugs at mindrot.org
          Reporter: cjwatson at debian.org

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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

--- Comment #1 from Colin Watson <cjwatson at debian.org> ---
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Colin Watson <cjwatson at debian.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|ssh-copy-id leads to rm -f  |ssh-copy-id can "rm -f
*"
                   |* (local vs. dash)          |if mktemp fails

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
   Attachment #2269|                            |ok?(dtucker at zip.com.au)
              Flags|                            |

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
Created attachment 2269
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2269&action=edit
Fix rm bug

Check exit status of mktemp and that it returned a filename

Be more specific in which files are deleted

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2269|ok?(dtucker at zip.com.au)     |
              Flags|                            |
   Attachment #2270|                            |ok?(dtucker at zip.com.au)
              Flags|                            |
   Attachment #2269|0                           |1
        is obsolete|                            |

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 2270
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2270&action=edit
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"

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |phil at hands.com

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2270|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

--- Comment #4 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2270
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2270
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

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2271|                            |ok?(dtucker at zip.com.au)
              Flags|                            |
   Attachment #2270|0                           |1
        is obsolete|                            |

--- Comment #5 from Damien Miller <djm at mindrot.org> ---
Created attachment 2271
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2271&action=edit
Use more portable test syntax

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au
             Blocks|                            |2035

--- Comment #6 from Darren Tucker <dtucker at zip.com.au> ---
we should put this in 6.2p2

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2271|ok?(dtucker at zip.com.au)     |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
Committed. This will be in openssh-6.2p2, due in an hour or so (lucky
timing...)

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

--- Comment #8 from Philip Hands <phil at hands.com> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

--- Comment #10 from Philip Hands <phil at hands.com> ---
(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).

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2105

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.