This looks like an interesting read.
Stephen Michael Kellat
Begin forwarded message:
> From: Seth Arnold <1203...@bugs.launchpad.net>
> Date: October 7, 2013, 9:36:38 PM EDT
> To: skel...@fastmail.net
> Subject: [Bug 1203207] Re: [MIR] mir
> Reply-To: Bug 1203207 <1203...@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.
>
--
xubuntu-devel mailing list
xubuntu-devel@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/xubuntu-devel
