xubuntu-dev team mailing list archive

[Seth Arnold <1203207@xxxxxxxxxxxxxxxxxx>]

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
Developers, which is subscribed to the bug report.
https://bugs.launchpad.net/bugs/1203207

Title:
  [MIR] mir

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