[Bug 524226] Re: ssh-import-id - retrieve a key from a public keyserver and add to the authorized_keys file

[Jamie Strandboge]

While I can see the utility of this script in certain situations, I'm
not sure it is generally useful enough to put in openssh, or even cloud-
init. It really feels like it should be in its own package. Also, I
think we can assume that someone will one day want to run this as root,
since the idea is to share the VM, and people will likely want to share
all of it. As such, I reviewed the script and feel several improvements
should be made. I don't claim this as an exhaustive review and may have
missed something. These are in no particular order. I hope others will
review it as well.

1. there is no input validation for the '-u' url. Combined with the
'printf' this is a format string vulnerability. I haven't thought about
what all you could do with it, but you really only want %s anyway, so
enforce it.

2. only '$i' is url encoded. Since -u contains untrusted input, you
should url encode it as well

3. there is no input validation for the '-d' option. I can then do -d
'/etc/cron.daily' (or any number of fun things). Note there is no
privilege escalation, but there is also no reason why the script should
write to '/etc/cron.daily'. I suggest rather than using a directory, you
specify a local user. Then use 'getent passwd "$local_user"' to get the
$HOME directory, then tack '.ssh' on to that. You can then do fascist
input validation on $local_user. You can look at adduser for hints on
valid user names

4. http or any cleartext protocol *must not* be allowed otherwise very
serious man in the middle scenarios are easily possible

5. There is no validation for '$pubkey', which could lead to an invalid
entry in the authorized keys file. This is particularly worrisome since
the script trusts the response from the server completely. There are all
kinds of things that can happen here, from a DoS with 'from="..,!<you>"'
to multiple keys being downloaded.

6. there is a TOCTOU issue on the mkdir and then later with file. For the mkdir, use this instead (I left out '-p' intentionally, since if the user's $HOME as seen through 'getent' doesn't exist, it is probably for a reason): 
mkdir -m 0700 "$dir" 2>/dev/null || true
test -d "$dir" || fail "failed to make ${dir}"

For file, I suggest doing something like (notice we can perform validation on the tmp file before committing to authorized keys):
tmp=`mktemp`
trap "rm -f $tmp" EXIT HUP INT QUIT TERM
...
echo "$pubkey" >>  "$tmp" || fail "failed to write to ${file}"
validate_authorized_keys "$tmp" || fail "Invalid entry. Aborting"
mv -f "$tmp" "$file"

7. the script should use 'set -e'

8. this line should probably be broken up, with each failure being reported on its own:
if url_encode "$i" && cururl=$(printf "$url" "${_RET}") &&
           pubkey=$(wget --quiet -O- "$cururl") && [ -n "${pubkey}" ]; then

9. I'd like to see enforcement of valid ssl certs from the server,
otherwise we can be man-in-the-middled

10. Due to the nature of the script, I'd recommend scrubbing the
environment in some way (perhaps 'env -i' before the commands or change
to bash and use 'set -p'). If you used bash, you could maybe even get
away with using '-r' (restricted shell) if you got rid of the '>>' and
used wget to append to the tmp file instead. I've not tested any of
these suggestions.

TBH, after reviewing the code and thinking a little about what could go
wrong, my initial reaction is that this is too scary for openssh and
cloud-init (and potentially main, but I'll let the MIR team comment on
that). Also, since this is being thought of as helpful with the cloud,
it is easy to imagine this command  being used in ways for which the
script is not currently designed to handle, such as in combination with
a web service of some sort. Integration into these types of environments
would require even more scrutiny and careful planning.

-- 
ssh-import-id - retrieve a key from a public keyserver and add to the authorized_keys file
https://bugs.launchpad.net/bugs/524226
You received this bug notification because you are a member of Ubuntu
Server Team, which is subscribed to openssh in ubuntu.