On Fri, Feb 19, 2010 at 8:47 AM, Jamie Strandboge <email address hidden> wrote:
> 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.

Thanks for the review, Jamie.

> 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.

Actually, I would like to drop the -u parameter altogether for the
first cut of the utility.  Localize the variable entirely, forcing
Launchpad.net as the only valid URL.  Users wanting another URL would
need to copy the script and edit it locally.

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

Agreed.  Moot point, though, if we drop -u.

> 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

Good suggestion.  But I'd also like to drop -d for now too, in the
interest of simplicity.  I was only using it for testing.  I think the
assumption should be that only the user running the script can write
to their own $HOME/.ssh/authorized_keys file (which presumably they
can do already).

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

Agreed.  The wget I used enforces certificates, and fails if
certificate is invalid.  Again, if we drop the -u parameter, and force
the https://launchpad.net... URL, I think we're safe here.

> 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.

Great point.  This should be fixed.

> 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}"

Thanks, agreed.

> 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"

Agreed.

> 7. the script should use 'set -e'

Ah, now I see...  You're looking at Scott's script, and not mine.  I
used "set -e".  He used a bunch of fancier stuff, including the -u and
-d parameters, and did not 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

Right.

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

Hmm, I thought this was already done?  The wget will fail without a
valid cert, and thus it should not be written to auth_keys.

> 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.

Yes, good call.

> 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.

Okay.