Comment 48 for bug 1267393

I partially reviewed juju-core version 1.17.6-0ubuntu1 as checked into
trusty. This shouldn't be considered a full security audit; this review is
even more cursory than usual, since the MIR has been retracted for trusty.

So, here's the notes I've collected thus far in the hopes that they are
useful for the next MIR review:

- release-public-tools.sh doesn't validate downloaded packages

The jujud that is served up through simplestreams or cloud service
provider tools buckets is entirely unchecked against accidental or
malicious replacement. Please use apt-get download to retrieve packages,
it performs all the necessary package hash comparisons and signed package
list comparisons that are currently missing in the shell script.

- utils/trivial.go ShQuote() is broken

The quoting method used is broken -- some clever arrangement of ",
`, and $() will be able to execute unexpected commands. Part of why
the quoting method is broken is because the design of the tool is
trying to accomplish too many things at once. ShQuote() is being used
both for quoting a single argument and for parsing entire scripts. It
cannot be used for both tasks, and in fact trying to "quote" an entire
script is a mistake. It needs to be re-done to focus on a single
argument and use the method of quoting outlined by Florian Weimer at
http://www.openwall.com/lists/oss-security/2014/02/04/3

    The proper way (at least if your shell runs in a UTF-8 or ISO-8859
    locale) to escape shell arguments is to wrap them in '', after replacing
    embedded ' characters with the four character sequence '\''.

I'm pretty new to Go, but it feels like there's a nice opportunity to
use the type system to create a ShellScript type that can only be
constructed from static strings and properly quoted arguments -- and then
the various Run commands could be typed to only accept arguments of the
ShellScript type, to statically discover when a shell injection site may
have been missed.

- juju-backup shell script uses incorrect quoting method needlessly

Another case of trying to quote an entire shell script; the base64 trick
used elsewhere may be a better fit for what is attempted here.

- The local environment hardcodes use of 'sudo' to raise privileges

Some of the functions will execute 'sudo' if they weren't called with
sufficient privileges. I'd rather have operations clearly fail if
permissions are insufficient instead of trying to raise privileges
behind the scenes. If some installations have configured sudo, these
may be unwelcome or raise awkward questions or provide a very unfriendly
user experience.

Using 'sudo' in the units is fine, juju owns those. But sudo on the local
host may be different.

Thanks