Merge lp:~vanvugt/mir/run-without-entropy into lp:mir

[Daniel van Vugt]

Avoid hanging and/or crashing the Mir server when gathering entropy
(LP: #1536662 and LP: #1541188)

Hanging or crashing a visual system like a display server is
unacceptable. And I'm fairly confident we're not doing anything
sufficiently security-critical with "cookies" that could justify hanging
or crashing the system.

Unless you're trying to protect your data from a hardened professional
attacker, there is no justification for blocking indefinitely on
/dev/random, when the non-blocking /dev/urandom is sufficiently secure.

And even if you think you need /dev/random, you should gather data from
it _offline_ only, like ssh-keygen does. Because there's no telling
just how long it might take to read /dev/random. It can and will often
take more than the 30 seconds we were willing to wait (which was too
long anyway).

[Mir CI Bot (mir-ci-bot)]

FAILED: Continuous integration, rev:3347
https://mir-jenkins.ubuntu.com/job/mir-ci/411/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/212/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/236
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/228
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/228
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/219
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/219/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/219
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/219/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/219
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/219/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/219
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/219/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/219/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/411/rebuild

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Seems sensible (but of course I am not a security expert).

(ignoring the build failure in this approval)

review: Approve

[Alan Griffiths (alan-griffiths)]

I am not a security expert either, but I know this won't pass audit

review: Disapprove

[Brandon Schaefer (brandontschaefer)]

We cant do this with out a security team member approving this.

As this would RUIN our entire reasoning for even doing cookies to begin with. If we are having entropy issues to begin with on boot, then this is pretty much accepting that we will be generating a predictable secret. The best we can do is an attempted lazy create the secret we something is first requested vs doing it on boot.

review: Disapprove

[Brandon Schaefer (brandontschaefer)]

The other thing we can do, is check the kernel version, and based on that use get_random(). Wont solve our issues for prev versions or the phone. This could fix the issue on the desktop though (which seems to be the only place we are getting hit by this so far)

[Brandon Schaefer (brandontschaefer)]

http://man7.org/linux/man-pages/man2/getrandom.2.html

[Alexandros Frantzis (afrantzis)]

An interesting read (but once again I note that I don't have the expertise to judge):

http://www.2uo.de/myths-about-urandom/

[Brandon Schaefer (brandontschaefer)]

Well i am far from a security expert :). If we can get a security review saying this is the fine way to go, then im 100% fine with it. Im just being overly cautious because I dont want to break security!

[Daniel van Vugt (vanvugt)]

The logic is simple:

No other display server (or app?) hangs and/or crashes waiting for entropy. It's a bad user experience and obviously unacceptable.

I don't care if it "would fail audit", that misses the point. The bigger problem is that we currently fail basic usability. Mir needs to start up quickly and without crashing. Presently it does not do that.

Furthermore, Mir needs to keep responding quickly and without crashing. So to defer entropy collection only defers the problem without solving it.

This branch is the only way forward I can tell. If we've made heavy crypto a priority over a usable system, we've failed. It's not that the current design needs to pass security audit, but the current design is a failure if it can't pass a security audit without hanging indefinitely or crashing.

[Chris Halse Rogers (raof)]

Mir - or something - needs to generate a key, because we're securing copy/paste with a secret-key system.

Using /dev/urandom is unsafe for something that'll run in early-boot, before the random pool is initialised; if /dev/urandom is used at that time there's no entropy so it'd relatively simple to break the key and hence the copy/paste security.

Since USC doesn't require copy/paste we can avoid the most likely trigger for the problem by USC not providing cookies.

If this is insufficient, we could not provide cookies until a source of randomness has been initialised.

We could also rework the copy/paste API to not require a secret key.

We *shouldn't* handle this by throwing away the check. If this check is failing, then we're in precisely the situation where urandom has insufficient entropy to provide security guarantees!

review: Disapprove

=== modified file 'src/cookie/authority.cpp'
--- src/cookie/authority.cpp	2016-01-29 08:18:22 +0000
+++ src/cookie/authority.cpp	2016-02-26 07:35:45 +0000
@@ -41,9 +41,6 @@
 
 namespace
 {
-std::string const random_device_path{"/dev/random"};
-std::string const urandom_device_path{"/dev/urandom"};
-uint32_t const wait_seconds{30};
 uint32_t const mac_byte_size{20};
 
 size_t cookie_size_from_format(mir::cookie::Format const& format)
@@ -62,55 +59,19 @@
 
 static mir::cookie::Secret get_random_data(unsigned size)
 {
+    int fd = open("/dev/urandom", O_RDONLY);
+    if (fd < 0)
+        BOOST_THROW_EXCEPTION(std::system_error(errno, std::system_category(),
+                                                "open failed on urandom"));
+
     mir::cookie::Secret buffer(size);
-    int random_fd;
-    int retval;
-    fd_set rfds;
-
-    struct timeval tv;
-    tv.tv_sec  = wait_seconds;
-    tv.tv_usec = 0;
-
-    if ((random_fd = open(random_device_path.c_str(), O_RDONLY)) == -1)
-    {
-        int error = errno;
-        BOOST_THROW_EXCEPTION(std::system_error(error, std::system_category(),
-                                                "open failed on device " + random_device_path));
-    }
-
-    FD_ZERO(&rfds);
-    FD_SET(random_fd, &rfds);
-
-    /* We want to block until *some* entropy exists on boot, then use urandom once we have some */
-    retval = select(random_fd + 1, &rfds, NULL, NULL, &tv);
-
-    /* We are done with /dev/random at this point, and it is either an error or ready to be read */
-    if (close(random_fd) == -1)
-    {
-        int error = errno;
-        BOOST_THROW_EXCEPTION(std::system_error(error, std::system_category(),
-                                                "close failed on device " + random_device_path));
-    }
-
-    if (retval == -1)
-    {
-        int error = errno;
-        BOOST_THROW_EXCEPTION(std::system_error(error, std::system_category(),
-                                                "select failed on file descriptor " + std::to_string(random_fd) +
-                                                " from device " + random_device_path));
-    }
-    else if (retval && FD_ISSET(random_fd, &rfds))
-    {
-        std::uniform_int_distribution<uint8_t> dist;
-        std::random_device rand_dev(urandom_device_path);
-
-        std::generate(std::begin(buffer), std::end(buffer), [&]() { return dist(rand_dev); });
-    }
-    else
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to read from device: " + random_device_path +
-                                                 " after: " + std::to_string(wait_seconds) + " seconds"));
-    }
+    auto got = read(fd, buffer.data(), size);
+    int error = errno;
+    close(fd);
+    
+    if (got != size)
+        BOOST_THROW_EXCEPTION(std::system_error(error, std::system_category(),
+                                                "read failed on urandom"));
 
     return buffer;
 }

=== modified file 'tests/acceptance-tests/test_server_startup.cpp'
--- tests/acceptance-tests/test_server_startup.cpp	2016-02-26 03:37:46 +0000
+++ tests/acceptance-tests/test_server_startup.cpp	2016-02-26 07:35:45 +0000
@@ -68,7 +68,7 @@
         });
 }
 
-TEST(ServerStartupReliability, DISABLED_can_start_with_low_entropy)
+TEST(ServerStartupReliability, can_start_with_low_entropy)
 {   // Regression test for LP: #1536662 and LP: #1541188
     using namespace ::testing;
 

=== modified file 'tests/unit-tests/test_mir_cookie.cpp'
--- tests/unit-tests/test_mir_cookie.cpp	2016-02-26 03:37:46 +0000
+++ tests/unit-tests/test_mir_cookie.cpp	2016-02-26 07:35:45 +0000
@@ -119,7 +119,7 @@
         Ge(mir::cookie::Authority::minimum_secret_size));
 }
 
-TEST(MirCookieAuthority, DISABLED_given_low_entropy_does_not_hang_or_crash)
+TEST(MirCookieAuthority, given_low_entropy_does_not_hang_or_crash)
 {   // Regression test for LP: #1536662 and LP: #1541188
     using namespace testing;