I reviewed unity-system-compositor version (0.0.1+13.10.20130926.1-0ubuntu1)
as checked into saucy. This should not be considered a full security
audit, but rather a quick gauge of code quality.
- This package listens on a socket and based on the contents of the
messages on the socket, makes a method call into Mir to ask for a
different "session".
- Build-Depends: cmake, libboost1.48-all-dev, libboost-all-dev,
libmirserver-dev, libgles2-mesa-dev, libprotobuf-dev, python
- Depends: xserver-xorg-xmir
- No cryptography
- No networking, though on-filesystem pipe is wide-open
- Runs alongside Unity and Mir; does not daemonize
- Listens on /tmp/mir_socket, or other configurable socket
- Presumably runs with privileges, does not itself manage privileges
- Postinst requests a reboot from the user
- No initscripts
- No dbus
- No setuid
- No sudo
- No cronjobs
- Provides two binaries, one ELF unity-system-compositor and one /bin/sh
script unity-system-compositor.sleep which sleeps .1 s before exec the
other, seems like a bug being papered over...
- Clean build log
- Test suite doesn't test any of the code in this package
Furthermore, it's only useful for the specific case of hardware in the
testing lab -- not useful for security updates or long-term maintenance
tasks due to assumptions made in the test suite
- No subprocesses are spawned
- Memory management is safe
- File IO happens only via subclassing Mir classes
- Pipe IO relies upon Boost, good choice, but may make unsafe assumptions,
detailed below
- Logging looked excessive, though safe
- Environment variables safely handled
- chmod on control pipe seems misplaced, detailed below
- No cryptography
- No networking
- No privileged portions of code
- Uses fall-back /tmp/mir_socket filename if another isn't configured
- Does not use webkit
- Does not use javascript
Overall code quality is high, idiomatic-looking C++11.
However, I'm concerned about some aspects of the system design:
- SystemCompositor::main() sets mode of socket file to 777:
- Execute privileges shouldn't be granted
- fchmod(2) on an open socket would be safer; on systems with non-Ubuntu
kernels, a symlink or hardlink in /tmp/mir_socket can 777 any file in
the filesystem.
- Best would make the socket owner set the privileges when the socket is
created, rather than have this package modify the socket privileges.
- Everything can write to this file. Firefox. Libreoffice. Cronjobs.
Webservers. Database servers. It would be best to not rely entirely
upon AppArmor for safety, as AppArmor might be disabled in some
environments or policy not yet written for untrusted applications.
Can permissions be locked down better?
- Stream-oriented socket may allow a malicious client to send a message:
0x00 0x00 0x7F 0xFF close()
This ought to gum up future commands...
Maybe should use unix(7) instead to allow OOB packets for framing, or POSIX
message queues, or SCTP if network use is in the future..
- SystemCompositor::set_active_session() linear search, string comparison
is linear -- O(N^2) algorithm here. How many sessions will this manage?
- Manual bit twiddling rather than using htons() and ntohs() may prevent
cross-architecture networking
And some small comments:
- Friendly reminder that SystemCompositor::set_next_session() is // TODO
- Unsatisfiable Build-Dependency libboost1.48-all-dev prevents standard
security toolchain from working
- There is a lintian error for the autopilot package on a local build:
bad-provided-package-name python:any
Please fix the libboost1.48-all-dev dependency before shipping this
package if possible; it'd be nice to avoid this annoyance if an update
is necessary.
Please change 0777 to 0666 for the pipe at your earliest convenience.
Please switch to fchmod(2) if reasonable. If this code can be removed
entirely, all the better.
Ideally, whatever creates the pipe would set permissions appropriately,
and probably logind or other mechanisms could be used to manage access
to this pipe. This is a discussion we can have later.
Please consider testing stability of the system while running a script
like this:
while true ; do dd if=/dev/urandom of=/tmp/mir_socket bs=$RANDOM count=1
; done
Be sure to test if the session switching still works while this script is
running.
Security team ACK for including in main.
** Changed in: unity-system-compositor (Ubuntu)
Assignee: Seth Arnold (seth-arnold) => (unassigned)
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1203588
Title:
[MIR] unity-system-compositor
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/unity-system-compositor/+bug/1203588/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
