[Bug 509647] Re: [MIR] lxc

[Seth Arnold]

I audited lxc version 0.9.0~rc1-0ubuntu3 as checked into Raring.

This should not be considered a complete security audit, but rather
a quick gauge of maintainability.

- lxc provides userspace convenience wrappers around the Linux kernel's
  containers implementation to make using containers convenient
- There's no cryptography
- No traditional networking, though extensive control of network
  interfaces, bridges, and routing
- Some Unix domain sockets and Netlink domain sockets
- Uses libapparmor, libcap, libc, liblxc, libpthread, libseccomp, libutil
- Can start containers and dnsmasq at boot
- dnsmasq runs as specific lxc-dnsmasq user
- Carefully daemonizes, with nice mechanism to report errors to
  grandparent for good human-based interaction
- postrm does not remove user created during postinst
- No DBus services
- No setuid
- No sudo fragments
- No cron jobs
- Many "failed to load external entity" errors in build logs, probably
  just an incorrect docbook option
- No PIE, no immediate binding
- Variable stack protection (likely false negatives, small binary)
- Variable Foritify (likely false negatives, small binary)
- Consistent read-only bindings

Code quality varied; the Python bindings seem least mature, though the
average quality was otherwise good.


Python bindings:

- convert_tuple_to_char_pointer_array() appears to write to unallocated memory
- convert_tuple_to_char_pointer_array() error-case memory leak
- Container_get_cgroup_item() error-case memory leak
- Container_get_config_item() error-case memory leak
- Container_get_keys() error-case memory leak

convert_tuple_to_char_pointer_array() is serious, if I've properly
understood the function. This must be reviewed by someone else (or
valgrind) before the Python bindings can reasonably be promoted to main.
The presence of this problem in the Python bindings makes me wonder if
they can be split apart and left in universe for the time being.


Everything else:

- Calling lxc_container_free() _after_ releasing the privlock feels wrong
   - lxc_container_get() may follow stale pointers while locking
   - lxc_container_put() could be freeing an object that was acquired

Probably lxc_container_free() should be called with the privlock still
held, and the lock destroyed after the object no longer exists. If
any existing multiprocess or multithreaded code relies upon shared
lxc_container objects, this must be addressed before being promoted
to main. If no current code relies upon shared lxc_container objects,
this can be handled as a simple bug report.

- run_script() shouldn't build string for popen(3) from argv array; this
  should do the difficult pipe2(2) and dup2(2) itself and use execve(2), 
  execv(3), or execl(3) directly
- run_script() shouldn't check length against INT_MAX, the argument size
  limit should be roughly two megabytes (see xargs --show-limits for
  size..) -- ideally, this would be removed entirely when replaced with
  an execve(2) version

I didn't follow the callers all the way to the configuration variables
that were used in building these strings; if they can only be set by root,
then this can be handled as a simple bug. If they can be set by a user
account or from within a container, this should be fixed before promoting
to main.

- lxc_newlock() does not use O_EXCL
- lxc_container_free() closes but doesn't unlink named semaphores 

These two function appear designed to work together this way intentionally.
But it would be nice to check ownership of the semaphore after creation,
to ensure an untrusted user account didn't create the semaphore.

- lxclock_name() does not restrict length of semaphore name
- run_buffer() could use stack-allocated rather than malloc(3)
- config_network_ipv4() several error-case memory leaks
- config_network_ipv4_gateway() several error-case memory leaks
- config_network_ipv6() several error-case memory leaks
- config_network_ipv6_gateway() several error-case memory leaks
- Several config_*() functions use atoi(3), which cannot report errors
- config_mount() error-case memory leak
- caps.c appears to have three different ways of finding valid caps,
  it feels like this code hasn't been cleaned up in a while
- ifa_get_local_ip() has an unchecked malloc(3)


Just assorted minor annoyances. Please address in whatever manner seems
appropriate, in whatever time frame is convenient.


I believe the increased exposure to lxc due to 13.04 main inclusion would
help polish lxc enough that a full five-year support promise in 14.04 LTS
would be viable. So, despite the issues raised here, lxc gets a
provisional ACK, conditional upon:

- Either a review of Convert_tuple_to_char_pointer_array() to prove
  correctness _or_ provide a solution that does not write to unallocated
  memory
- Either a review that no currently shipping code relies upon the
  concurrent use of lxc_container objects _or_ a review to prove
  correctness of the locking code _or_ a solution for the locking code.
- Either a review that no untrusted input is used in run_script()'s
  arguments _or_ replacing the implementation with one based on exec()
  rather than popen().
- Enable PIE for the binaries.

Thanks


** Changed in: lxc (Ubuntu)
     Assignee: Seth Arnold (seth-arnold) => MIR approval team (ubuntu-mir)

-- 
You received this bug notification because you are a member of Ubuntu
Server Team, which is subscribed to lxc in Ubuntu.
https://bugs.launchpad.net/bugs/509647

Title:
  [MIR] lxc

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/lxc/+bug/509647/+subscriptions