[MIR] unity-system-compositor

[Robert Ancell]

Availability: lp:unity-system-compositor, ppa:mir-team/system-compositor-testing
Rationale: Required for https://blueprints.launchpad.net/ubuntu/+spec/client-1303-mir-system-compositor
Security: No known security issues
Quality assurance: no major bugs
UI standards: N/A
Dependencies: All in main except for mir (MIR bug 1203207)

[Daniel van Vugt (vanvugt)]

What is this bug?

Is it a request for packaging? If so it should have tag "needs-packaging", and of course a better description.

[Daniel van Vugt (vanvugt)]

Oh... (M)ain (I)nclusion (R)equest

:)

[Michael Terry (mterry)]

This should probably be a security-approved MIR. (assigning to upstream part of this bug, since LP won't let me assign to the Ubuntu task, as it isn't in Ubuntu yet)

[Michael Terry (mterry)]

I had some time and did a packaging/maintainership review:
* Please subscribe the dev team to bug reports in Ubuntu.
* Autopilot tests aren't used via dep8, but are used for daily-landing gating, so that's fine.
* Why isn't debian/source_unity-system-compositor.py installed?

Besides that, pretty small, clean package. Just needs a security review (and a bug subscriber).

[Jamie Strandboge (jdstrand)]

The security review is going to take some time and we shouldn't block on it. Assuming everything else is ok, please prepromote and anything the security team finds we'll file as bugs to fix.

[Michael Terry (mterry)]

OK, I'll leave this bug open for the eventual security review, but now that there is a team bug subscriber, this can be promoted.

[Seth Arnold (seth-arnold)]

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 ...