Comment 8 for bug 1481507

I reviewed lxd version 0.18-0ubuntu2 as checked into wily. This should not
be considered a full security audit, indeed I reviewed significantly less
code than I would have liked due to time constraints.

This is a much larger codebase than I anticipated; it's much more than
"just" wrappers around lxc libraries. The vendorized code common to Go
programs also drastically contributes to the code size and it's quite
difficult to see which portions may be important to the lxd package.
Breaking apart the dependencies would help future reviews.

That said, I did read through the entire API spec (not the implementation,
sadly) and have only limited feedback to give. lxd appears to sidestep
many of the problems that have plagued OpenStack's Glance by not having
any tenants.

The simplicity in the end-user interface is amazing and frankly I wish
lxd would grow support for qemu/kvm as well. No existing VM management
system is anywhere near this elegant.

The one issue I found that I felt required a CVE was handled very well.

The shell scripts feel poorly-reviewed; shellcheck found many issues, the
most troubling are summarized below.

I think the project (and package) would benefit from a pruning phase; I
have the feeling that some things were simply re-implemented because it
was easy, or existing pieces reused because they already existed, but
dragged along their own dependencies. I think a week or two of looking at
the entire package sources with an eye towards removing what could be
removed would be a benefit to the project.

Go seems an odd choice for a systems programming language; much of its
design seems contrary to working directly with POSIX-defined interfaces.
There seemed to be a lot of code dedicated towards handling the boundaries
between the C world and the Go world and while it read much like other
interfaces (e.g. Python modules implemented in C, Erlang NIFs) it does
seem like there's a lot of ancillary code compared to application code.

Some more specific comments:

- loads of vendorized go code, needs to package dependencies, see
  http://launchpad.net/bugs/1267393 comments 62, 63, 68
- I'm scared of the code to automatically add admin users to lxd's
  interface. It seems dangerous compared to "sudo adduser sarnold lxd ;
  newgrp lxd".
- "lxc config set core.trust_password SECRET" -- this exposes password via
  ps, top, etc; examples should use "lxc config edit core.trust_password"
  instead
- idmapset.go ToLxcString() outputs a different format than parse() reads
- getFromMap() enforces UTF-8 encoding on username. How does this handle
  invalid UTF-8 in /etc/subxid files? Do any other utilities enforce UTF-8
  formatting of this file? Which other utilities may perform case folding?
  Do they all perform UTF-8 case folding in this fashion?
- POST to /1.0/images conflates fingerprint with alias -- I think
  fingerprint should always be included for integrity regardless of alias
  presence or absence
- POST to /1.0/certificates has multiple conditionals with "friendly"
  fallbacks. I think this is a mistake, I think the client should supply
  the certificate and name without friendly fallbacks to using the current
  connection's parameters.
- methods.go LinkUnitFiles() -- does this allow setting arbitrary services
  to start?
- container.go Daemonize() feels very misnamed
- container.go WantDaemonize() feels very misnamed
- container.go WantCloseAllFds() error return ErrCloseAllFdsFailed seems
  wrong, the call chain doesn't look like it actually tries to close
  anything, and based on the boolean passed in may not even want to.
- container.go ConfigKeys() is confusing, could someone give it a re-read
  and make sure it does what it's supposed to do? TestConfigKeys() isn't
  very comprehensive.

Shell Issues:
- ./lxd-bridge/lxd-bridge variable LXD_IPV4_NETMASK vs LXD_IPV4_MASK
  mistake. Most variables used in commands need to be "" double-quoted to
  force treating their expansion as a single token.
- goproxy/all.bash most variables used in commands need to be
  double-quoted; *_test.go is unsafe with filenames starting with -;
  should use ./*_test.go
- debian/update-wordlists.sh uses "$string interpolation" in printf,
  should just use printf directly; should use ./*.txt.list rather than
  *.txt.list
- debian/lxd.postinst add_groups() uses variables that are never passed
  in, please add a comment to explain what they are there for

Already raised with stgraber and hallyn:
- doUidshiftIntoContainer() has an unsafe Chmod() call that races against
  the stat in the Filepath.Walk() function. A symbolic link created in
  that window could cause any file on the system to have any mode of the
  attacker's choice. This function cannot be used if any processes from
  the uid ranges in question could be executing on the system.
  The mode change needs to be done after dropping privileges to the target
  user and group.

- container.go CreateSnapshot(), RestoreSnapshot(), DestroySnapshot(),
  DestroyAllSnapshots(), Snapshots(), Freeze(), Unfreeze(),
  Start(), Execute(), Stop(), Reboot(), Shutdown(), Destroy(),
  DestroyWithAllSnapshots(), Clone(), Rename(), races against status
  changes since c.makeSure() isn't called within a lock. Either the
  c.makeSure() locking needs to be handled in each method or the test
  should be removed: racy tests doubles the number of places that
  have to handle errors.

We can support lxd in main conditional upon some concessions
from the lxd team:

- The security team will need help preparing security fixes for all
  supported releases. This may come at the expense of feature development.
  Additional headcount may be necessary to meet all business goals while
  still providing acceptable support for supported releases.

- The security team will need help testing all updates for all supported
  releases; this may include test cases, test frameworks, and infrastructure.

- The lxd team will discuss with the technical board the best way to keep
  lxd updated across all supported releases; this may benefit from being
  kept level across all releases similar to Firefox. This may mandate not
  using new Go language features introduced in future releases until those
  features are on our oldest supported releases.

- The lxd team will break apart the vendorized Go dependencies. Discuss
  with the Juju team which shared dependencies, if any, should be split
  apart first, and aim to have all external dependencies removed from the
  lxd package before 16.04 beta. See the Golang MIR bug for full details:
  https://bugs.launchpad.net/ubuntu/+source/juju-mongodb/+bug/1267393

Please confirm these conditions are acceptable.

Thanks