Comment 9 for bug 524226

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.