The security audit on Mir

[Stephen Michael Kellat]

This looks like an interesting read.

Stephen Michael Kellat 


Begin forwarded message:

> From: Seth Arnold <1203207 at bugs.launchpad.net>
> Date: October 7, 2013, 9:36:38 PM EDT
> To: skellat at fastmail.net
> Subject: [Bug 1203207] Re: [MIR] mir
> Reply-To: Bug 1203207 <1203207 at bugs.launchpad.net>
> 
> I reviewed Mir version 0.0.12+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.
> 
> - Mir is a new display server, intending to replace X11, to rely upon the
>  features of high-powered modern graphics hardware available in both
>  traditional computers and newfangled handheld devices. By starting over
>  with higher demands on hardware and reduced demands on legacy features,
>  the intention is to provide a display server that is faster and more
>  secure (e.g., preventing mouse grabs and keyboard grabs from preventing
>  screen saver lock, or client keypress sniffing, or other annoyances from
>  the X11 legacy codebase).
> - Build-Depends upon cmake, doxygen, xsltproc, graphviz, boost, protobuf,
>  libdrm, libegl1-mesa, libgles2-mesa, libgdm, libglm, libhardware,
>  libgoogle-glog, liblttns-ust, libxkbcommon, umockdev, libudev,
>  google-mock, valgrind
> - No cryptography
> - Extensive local networking, no off-machine networking
> - Not exactly the usual daemon; does not double-fork(2), setpgid(2) and
>  setsid(2) happen via mgg::LinuxVirtualTerminal::open_vt() rather than at
>  startup.
> - No initscripts
> - No dbus
> - No setuid
> - No sudo
> - No cron
> - Binaries in /usr/bin/:
>  mir_demo_client_flicker
>  mir_demo_server_basic
>  mir_demo_standalone_input_filter
>  mir_stress
>  mir_demo_server_shell
>  mir_demo_client_multiwin
>  mir_demo_client_scroll
>  mir_demo_client_fingerpaint
>  mir_demo_client_eglplasma
>  mir_demo_client_basic
>  mir_demo_client_eglflash
>  mir_demo_client_egltriangle
> - Good test suite
> - Fairly messy build logs, several instances of:
>  - warning: format '%d' expects argument of type 'int', but argument 4
>    has type 'size_t {aka long unsigned int}'
> - Many instances of:
>  - Warning: no uniquely matching class member found for ...
>  - Warning: no matching class member found for
> - Lintian errors:
>  - E: libmirplatform: postinst-must-call-ldconfig
>    usr/lib/x86_64-linux-gnu/libmirplatformgraphics.so
>  - E: mir-test-tools: arch-dependent-file-not-in-arch-specific-directory
>    usr/bin/mir_stress
> - Lintian warnings:
>  - (two) W: libmirplatform: shlib-without-versioned-soname
>    usr/lib/x86_64-linux-gnu/libmirplatformgraphics.so
>    libmirplatformgraphics.so
>  - (twelve) W: mir-demos: binary-without-manpage
>    usr/bin/mir_demo_client_basic
>  - (one) W: mir-doc: embedded-javascript-library
>    usr/share/doc/mir-doc/html/jquery.js
> 
> 
> - One instance of spawning a subprocess, examples/basic_server.cpp, just
>  passes along a command-line argument to system(3); not itself unsafe,
>  but might be unsafe in some potential uses of this example.
> - Most memory management is handled via C++ safe pointers
> - File IO is largely two types: socket parsing and device ioctls, looked safe
> - Logging looked safe, lttng toolkit provides nice tracing tools
> - Environment variables used: MIR_CLIENT_RPC_REPORT, MIR_SOCKET,
>  XDG_CONFIG_HOME, HOME, XDG_CONFIG_DIRS, MIR_BYPASS,
>  MIR_SERVER_HOST_SOCKET, ANDROID_ROOT, ANDROID_DATA
> - Environment variable use looked safe
> - Extensive ioctl use, possible mistake detailed below
> - Expects to run with sufficient privileges to manipulate hardware
>  devices, does not separate into high-privileged and low-privileged
>  portions during execution
> - No cryptography
> - Extensive Unix-domain networking, Google protobufs used in some cases to
>  provide structure-over-socket support, RPC support
> - Does not use WebKit
> - Does not use qtjsbackend
> 
> 
> Mir is professionally-written C++11 code; while it is well-written by
> disciplined programmers, maintenance tasks on Mir will require experts
> knowledgeable in its design and construction. The security team will
> largely be reliant upon "upstream" for any fixes that may be needed
> in the future, and it is vital that we retain in-house expertise on
> this codebase for as long as we commit to supporting it.
> 
> I'm concerned about the instructions to "chmod 777 /tmp/mir_socket" that
> appear in the documentation, and baked into another package: a Unix domain
> socket should not have execute permissions, and such wide-open permissions
> makes me worried about the reliability of this socket.
> 
> (Please forgive a small aside: normally, when I see instructions on
> web sites along the lines of "chmod 777 /path/to/something", I usually
> ignore the rest of that person's advice on the basis that they probably
> didn't apply scientific thinking to their problem solving. We should
> not encourage "chmod 777" as a solution to anything, even if the only
> change is "chmod 666".)
> 
> Setting the permissions after the socket is created and a name is bound
> to it is a race condition, one that might end up with users accidentally
> executing content written by another user (if non-Ubuntu kernels are used
> that lack our symlink and hardlink protections). Perhaps fchmod(2) can be
> used to set the permissions after the socket has been bound into the
> filesystem. If not, perhaps the umask(2) needs to be configured before the
> socket is bound into the filesystem.
> 
> I'd love to see a test case like this added to the test suite, ideally it
> would run concurrently with all the other tests:
> 
> while true do ; dd if=/dev/random of=/tmp/mir_socket bs=$RANDOM count=1
> ; done
> 
> Most methods assume input validation is handled elsewhere. This is
> okay in the early stages of a project, where the demarcation lines are
> clear and well-known to all members of the team, but in the long run I
> fear that lacking internal defense may become a reliability problem. Some
> methods have encouraging assert() macros to test preconditions; I would
> like to see more use of defensive assert()s.
> 
> I took some assorted notes while reading the source; some are duplicates
> of prose above (but the details seemed worth keeping). Feel free to handle
> most of these whenever it is convenient.
> 
> - Includes copy of input.h from Linux kernel sources
>  The version included in Mir shares the Android definitions of two
>  ioctls:
>    #define EVIOCGSUSPENDBLOCK  _IOR('E', 0x91, int)      /* get suspend block enable */
>    #define EVIOCSSUSPENDBLOCK  _IOW('E', 0x91, int)      /* set suspend block enable */
>  The version in the mainline Linux kernel has different symbols and
>  meanings for these specific ioctls:
>    #define EVIOCGRAB               _IOW('E', 0x90, int) /* Grab/Release device */
>    #define EVIOCREVOKE             _IOW('E', 0x91, int) /* Revoke device access */
> 
>  This is used in
>  3rd_party/android-input/android/frameworks/base/services/input/EventHub.cpp
> 
> - EventHub.cpp still uses usleep(2), which was removed from POSIX in 2008.
>  usleep(2) may fail around clock skews; nanosleep(2) uses the MONOTONIC
>  clock and thus won't fail around clock skews.
> 
> - progressbar.c still uses usleep(2)
> - progressbar.c uses unsafe routines in signal handlers, assume it's a
>  toy, no further investigation :)
> 
> - ./3rd_party/android-deps/std/properties.h property_get() uses
>  default_value if it is given, regardless of value. No length checks are
>  performed, and the handful of callers in the source base all give a
>  value parameter that could not be overwritten if default_value weren't
>  NULL. It's not broken within this package, but it is also not pretty.
> 
> - Missing required O_RDONLY, O_WRONLY, O_RDWR in:
>  ./tests/integration-tests/test_swapinterval.cpp
>  ./tests/unit-tests/frontend/test_protobuf_sends_fds.cpp
>  ./tests/unit-tests/client/test_client_mir_surface.cpp
>  ./tests/unit-tests/client/input/test_android_input_receiver_thread.cpp
>  ./tests/acceptance-tests/test_server_shutdown.cpp
> 
> - tests/mir-stress/src/threading.cpp run_mir_test() double-counts some
>  tests, if (!p->create_surface()) is true, both false and true are added
> 
> - src/server/frontend/published_socket_connector.cpp
>  mf::BasicConnector::client_socket_fd() creates a socketpair for
>  communication between clients and Mir, but I don't see the corresponding
>  bind(2) to give it a name in the filesystem, nor permissions setting,
>  nor options to use abstract sockets (@foo in netstat output..)
> 
> - ./src/client/rpc/mir_socket_rpc_channel.cpp send_message() uses manual
>  bit-fiddling rather than ntohs() and htons(), this might restrict client
>  and server to running on same endianness architecture, may complicate
>  emulator construction with qemu.
> 
> - Other methods use manual bit-fiddling for socket communications.
> 
> 
> SUMMARY
> =======
> 
> Please change "chmod 777" to "chmod 666" soon. I don't want "chmod 777" to
> be seen as reasonable advice. :)
> 
> Please investigate better approaches to change the permissions on the
> bound socket than chmod(2). Suggested replacements are fchmod(2) and
> then umask(2). Since the socket is created in this package, I'd recommend
> keeping permissions management of the socket in this package, rather than
> in unity-system-compositor.
> 
> Please investigate lintian warnings and C++ compiler warnings.
> 
> None of the above blocks including Mir into Ubuntu Saucy.
> 
> Security team ACK for including Mir in main.
> 
> Thanks
> 
> 
> ** Changed in: mir (Ubuntu)
>     Assignee: Seth Arnold (seth-arnold) => (unassigned)
> 
> -- 
> You received this bug notification because you are a member of Xubuntu
> Bugs Team, which is subscribed to the bug report.
> https://bugs.launchpad.net/bugs/1203207
> 
> Title:
>  [MIR] mir
> 
> Status in “mir” package in Ubuntu:
>  Triaged
> 
> Bug description:
>  Availability: lp:mir, ppa:mir-team/system-compositor-testing
>  Rationale: Required for Unity System Compositor (MIR bug 1203588)
>  Security: No known security issues
>  Quality assurance: no major bugs, one open for consideration of MIR - bug 1203070
>  UI standards: N/A
>  Dependencies: All in main except for libglm-dev (MIR bug 1176083), libgoogle-glog-dev (MIR bug 1151844), liblttng-ust-dev (MIR bug 1203589)
>  Background:
> 
>  Mir is the new display server planned for introduction into 13.10
>  unity-system-compositor is a component needed for the xmir configuration to work and leverage Mir for 13.10
> 
>  These components have been available and are currently being tested as part of the ppa:mir-team/system-compositor-testing
>  there are no major bugs and its introduction is expected not to interfere with saucy.
>  the XMir functionality being introduced will allow the X to continue in standalone mode if there is a failure.
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/xubuntu-devel/attachments/20131007/8e97fa25/attachment.html>