On Sat, Oct 10, 2015 at 04:42:46AM -0000, Seth Arnold wrote: > 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 didn't know of shellcheck, I'll be sure to run it against the LXD shell scripts (basically just the lxd-bridge init script + the LXD testsuite) and fix anything that seems relevant. > 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. We have some of that planned (see below). > 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. I can certainly agree with at least some of that. Though ultimately the choice of programming language wasn't up to us and it's not something we can really change at this point. Since LXD is built on a fair amount of C libraries (which is unusual for Go projects), we do have a fair amount of binding code (go-lxc, utils_linux.go, ...), it's pretty repetitive code which is usually pretty self-contained. The rest tends to be pure Go as is the norm in this ecosystem... > Some more specific comments: > > - loads of vendorized go code, needs to package dependencies, see > http://launchpad.net/bugs/1267393 comments 62, 63, 68 We have the list of those, a good bunch are already packaged, we'll have to package at least two new ones and move away/replace a few more. > - 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". That code was basically copy/paste from what's being done in libvirt and was a requirement to have a good first user experience (though the newgrp part is still annoying...). > - "lxc config set core.trust_password SECRET" -- this exposes password via > ps, top, etc; examples should use "lxc config edit core.trust_password" > instead Note that the "set" command as since grown support for reading from stdin, so "echo password | lxc config set core.trust_password -", or directly reading from a file (to avoid echo showing up), would now work. > - idmapset.go ToLxcString() outputs a different format than parse() reads I'd have to re-check the code, but I'm assuming parse() is meant to parse the /etc/sub{uid,gid} format, whereas .ToLxcString outputs a string compatible with lxc's lxc.id_map option. > - 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? All good questions. I do know from experience that UTF-8 usernames are a thing and things usually work fine, but I'm pretty sure we've never stress tested this particular bit of code :) > - POST to /1.0/images conflates fingerprint with alias -- I think > fingerprint should always be included for integrity regardless of alias > presence or absence You are referring to the POST to /1.0/images which triggers a cross-server copy. Requiring the client to pass the fingerprint would force the client to do a couple more REST calls to the source server to do something which the user didn't actually requested. Specifically, the user requests that the target of a given alias on the source server be copied to the remote server. Having the client resolve the fingerprint and send it to the server may race with the target changing in between which would end up with the incorrect image being copied. The target LXD server will already get the image info from the source (including fingerprint) and hash the content as it's transfered over the network, so I don't see the need for extra roundtrips in the client and an added race condition there. > - 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. This is to make it easy to script the API using curl or similar tools. Can you point out a security issue with those fallbacks? > - methods.go LinkUnitFiles() -- does this allow setting arbitrary services > to start? I have no idea, this is go-systemd's code and as far as I can tell, not part of the code which we actually use. LXD only uses the socket activation code from this project. Note that in the future, it'd be helpful to indicate the fullpath since I had to dig around a bit as this isn't part of the LXD upstream source tree. > - 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. I'm assuming all of the above refer to go-lxc and not to LXD's own container.go, again, giving full paths in such cases would save me valuable time. I'm not terribly familiar with this code since it's an external project maintained by Caglar, however those misnamed function as you call them are consistently named with the other LXC bindings and with LXC's own API, so I prefer consistency with C API over renaming everything to try and better describe what they do. want_daemonize and want_close_all_fds both just set a flag and return 0 if succesful, so the Go wrapping code looks right to me in that Err* will only be raised if the C API fails for some reason. As for ConfigKeys, the implementation looks correct, though a check for more than one argument could probably be added. ConfigKeys in the LXC C API (and its wrapping here) will return a list of config keys when passed NULL or a list of sub-keys when passed a key. The wrapper looks correct to me in that regard. > 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. Will fix where applicable (paths). > - 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 Unused by LXD. We'll probably switch to an in-house HTTP proxy for 16.04 as the go-proxy dependency is rather massive for a pretty trivial feature (non-logging, non-caching, http proxy with CONNECT support). > - debian/update-wordlists.sh uses "$string interpolation" in printf, > should just use printf directly; should use ./*.txt.list rather than > *.txt.list LXD itself never calls that code. I however believe Dustin is already following this bug report and I'm sure he'll be fixing this very soon :) > - debian/lxd.postinst add_groups() uses variables that are never passed > in, please add a comment to explain what they are there for You misread the script I'm affraid, however it does look like the code is wrongly ordered, the check for $1, $2, $3 should be after the set -- $line so that it catches invalid entries before the calculation is done rather than after :). Fixed in our packaging branch. > 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. We actually fixed it in a different way (no priv dropping), but yes, this has been fixed already. > - 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. Again, assuming you're talking about container.go from go-lxc and not from LXD as I said on IRC, LXC itself (liblxc) does the actual locking and proper checks before performing the actual action, it doesn't export any of the locking primitives to downstream. The checks done in go-lxc are only cosmetic, any actual action against the container will be done by liblxc with the proper locking and will, if needed, return an error back which the wrapper will return in a slightly less user friendly way. Would it be great to absolutely always catch those races and return the same error string, sure. Is it practical for every single layer to re-do the locking and checks, absolutely not. This is consistent with what's done in the other bindings and I don't expect us to do any major rework around this. Any actual race condition that could lead to a security vulnerability will be fixed in liblxc itself rather than trying to prevent such an issue in the downstream bindings (python3-lxc, python2-lxc, ruby-lxc, go-lxc, lua-lxc, haskell-lxc, java-lxc, ...) and binding users (in this case, lxd). > 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 LXC team already does so for all the other LXC related projects (lxc, cgmanager and lxcfs), so I don't expect this to be a problem. > - The security team will need help testing all updates for all supported > releases; this may include test cases, test frameworks, and infrastructure. As with LXC, LXD has an upstream Jenkins instance that can test any branch and make sure we don't have regressions. The same tests are also run in autopkgtest and can easily be run locally. > - 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 current plan (and this may change based on management request) is to do it LXC-style, with LXD 1.0 being tagged with 16.04 and then going into bugfix-only mode with a micro release exception. It's a model which has been working great for LXC and the LXC related projects (combined with PPA and backports for new versions) and one that I hope we'll be able to go with for LXD. > - 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 This is planned, current status is: DEPENDENCY PACKAGE or PLAN --- --- dist/src/code.google.com/p/go-charset/ TODO: Implement tiny HTTP proxy? dist/src/github.com/chai2010/gettext-go/ golang-gettext-dev dist/src/github.com/dustinkirkland/golang-petname/ golang-petname-dev dist/src/github.com/elazarl/goproxy/ TODO: Implement tiny HTTP proxy? dist/src/github.com/godbus/dbus/ golang-dbus-dev dist/src/github.com/golang/protobuf/ golang-goprotobuf-dev dist/src/github.com/gorilla/context/ golang-context-dev dist/src/github.com/gorilla/mux/ golang-mux-dev dist/src/github.com/gorilla/websocket/ golang-websocket-dev dist/src/github.com/inconshreveable/go-vhost/ golang-vhost-dev dist/src/github.com/mattn/go-colorable/ TODO: Change logging system? dist/src/github.com/mattn/go-sqlite3/ TODO: Is golang-gosqlite-dev good enough? dist/src/github.com/olekukonko/tablewriter/ TODO: Alternatives? dist/src/github.com/satori/go.uuid/ golang-uuid-dev dist/src/github.com/stgraber/lxd-go-systemd/ golang-go-systemd-dev dist/src/github.com/stretchr/objx/ golang-objx-dev dist/src/github.com/stretchr/testify/ golang-testify-dev dist/src/github.com/syndtr/gocapability/ golang-gocapability-dev dist/src/golang.org/x/crypto/ golang-go.crypto-dev dist/src/gopkg.in/check.v1/ golang-check.v1-dev dist/src/gopkg.in/flosch/pongo2.v3/ NEEDS PACKAGING dist/src/gopkg.in/inconshreveable/log15.v2/ TODO: Change logging system? dist/src/gopkg.in/lxc/go-lxc.v2/ NEEDS PACKAGING dist/src/gopkg.in/tomb.v2/ golang-gopkg-tomb.v2-dev dist/src/gopkg.in/yaml.v2/ golang-yaml.v2-dev My plan is to open 16.04 with LXD switching to all the existing packaged ones, note that this means that 17 MIRs will be filed and sent your way. We'll then be packaging go-lxc and pongo2 and look at the remaining 6 to see whether we can drop them somehow and if not, package them. > Please confirm these conditions are acceptable. That's pretty much what we expected so I don't have any problem with that. So long as your team can handle dealing with 19 extra MIRs in a timely fashion, we should be fine. > > Thanks > > -- > You received this bug notification because you are a member of Ubuntu > containers team, which is subscribed to lxd in Ubuntu. > Matching subscriptions: lxd > https://bugs.launchpad.net/bugs/1481507 > > Title: > [MIR] lxd > > Status in lxd package in Ubuntu: > Confirmed > > Bug description: > [Availability] > In universe since 15.04 and available for all architectures. Using golang for those architectures it supports and gccgo for the rest. > > [Rationale] > LXD is a new container manager based on liblxc which offers a REST API to manage containers and container images across multiple hosts. > > It's developed by Canonical and the LinuxContainers community and > meant to become the new one to interact with LXC containers, both > locally and at scale. As a result, we want it as widely available and > supported as LXC itself. > > It's planned for LXD to become pre-installed in cloud images as well > as snappy images. In this configuration, LXD will be socket activated > to limit resource usage on systems that don't actively use it and it > will not be listening on any network port by default. > > [Security] > LXD hasn't had any security issue so far. > > The LXD daemon runs as root, containers spawned by it are then > typically running unprivileged with apparmor, seccomp, capabilities > and cgroup restrictions through the use of LXC. > > The LXD daemon listens to a local unix socket, only accessible to > members of the lxd group. Through that unix socket it's then possible > to get the daemon to bind a tcp port for network operations. > > [Quality assurance] > LXD basically just works when it's installed, the daemon is auto-started through socket activation and any member of the admin or sudo group is granted access to lxd upon installation. > > There are no debconf prompts in the lxd packages. > > Upstream is pretty much only made of Ubuntu developers so we expect a > very good relationship here. As it stands, there aren't any serious > bug with LXD in Ubuntu. > > The package is actively maintained in Ubuntu by upstream, it's not in > Debian. > > LXD isn't tied to any specific hardware. > > The testsuite cannot be run at package build time due to strict > requirements on kernel, network and root access. > > A debian watch file is included to track new releases. > > [Dependencies] > > LXD build-depends on golang which is currently subject to a separate > MIR. > > [Standards compliance] > > LXD complies with the current Debian standards. > > [Maintenance] > > Upstream is maintaining the Ubuntu packages as well as various daily > builds and backports. > > [Background information] > Nothing special to report. > > To manage notifications about this bug go to: > https://bugs.launchpad.net/ubuntu/+source/lxd/+bug/1481507/+subscriptions -- Stéphane Graber Ubuntu developer http://www.ubuntu.com