<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div>This looks like an interesting read.</div><div> </div><div>Stephen Michael Kellat Begin forwarded message: </div><blockquote type="cite"><div>From: Seth Arnold <1203207@<a href="http://bugs.launchpad.net">bugs.launchpad.net</a>> Date: October 7, 2013, 9:36:38 PM EDT To: <a href="mailto:skellat@fastmail.net">skellat@fastmail.net</a> Subject: [Bug 1203207] Re: [MIR] mir Reply-To: Bug 1203207 <1203207@<a href="http://bugs.launchpad.net">bugs.launchpad.net</a>> </div></blockquote><blockquote type="cite"><div>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. <a href="https://bugs.launchpad.net/bugs/1203207">https://bugs.launchpad.net/bugs/1203207</a> 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. </div></blockquote></body></html>