Merge lp:~mir-team/mir/handle-mis-shaped-cookies into lp:mir

[Brandon Schaefer]

We need to handle mis shaped cookies. If someone creates a cookie authority that returns a Cookie that is too large/small to fit into our own assumption of what a cookie is we need to handle that gracefully.

We need to handle mis shaped cookies. If someone creates a cookie authority that returns a Cookie that is to large/small to fit into our own assumption of what a cookie is we need to handle that gracefully.

If the cookie is NULL. We sigsegv, which will have a meaningful bt.

If the Cookie is smaller then our under the hood version we just copy the size of that.

// This would be a fix me!
If the Cookie is larger then we only copy the max amount we can handle into the internal cookie.

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

PASSED: Continuous integration, rev:3278
https://mir-jenkins.ubuntu.com/job/mir-ci/225/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/225/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3278
http://jenkins.qa.ubuntu.com/job/mir-ci/6201/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5795
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4702
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5751
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/383
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/525
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/525/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/525
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/525/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5748
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5748/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8158
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27301
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/379
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/379/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/233
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27302

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6201/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

The reasoning sounds sensible but how is this related to the linked bug 1536662? Is it that cookies smaller when we can't find entropy?

review: Needs Information

[Brandon Schaefer (brandontschaefer)]

Its related, as its needed for the USC workaround. There will be another branch that will wait to create the secret but that is also a workaround. As theres no real solution right now.

Soo I can remove the link to that bug but since this branch is needed for the USC fix I figured that it was related

[Andreas Pokorny (andreas-pokorny)]

The test names seem to be odd...

Why did you use spin_wait instead of a WaitCondition?

review: Needs Information

[Brandon Schaefer (brandontschaefer)]

Yes the names do sound strange ... I dont know why i named them like that :)

There was a reason... at some point, but yeah no need and ill fix that!

[lp:~mir-team/mir/handle-mis-shaped-cookies]

3279. By Brandon Schaefer on 2016-02-04

* Fix test names
* Use wait cond vs spin wait

[Andreas Pokorny (andreas-pokorny)]

lgtm

review: Approve

[Alan Griffiths (alan-griffiths)]

I'm not sure what we're /really/ trying to support here. (Beyond avoiding a crash.)

If there are "assumptions about cookies" embedded in the wrong code (i.e. code that isn't reconfigurable with the cookie authority), then the problem is the code is in the wrong place!

Almost every time we call cookie->serialize() we have code than immediately copies the result to a cookie::Blob. If cookie::Cookie had a method to populate a blob directly, then that method could be configured by a custom cookie implementation.

Would that address the problem more effectively?

review: Needs Information

[Brandon Schaefer (brandontschaefer)]

Hmm that would expose the cookie::Blob :(. This is mainly a ... fix until we move to a std::vector. Once we can replace the android input transport (what ill be looking at soon!). Though it would be nice to return a cookie::Blob from a cookie::serialize()... ideally once we have a std::vector everywhere this function will be the expected public interface.

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

PASSED: Continuous integration, rev:3279
https://mir-jenkins.ubuntu.com/job/mir-ci/232/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/232/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3279
http://jenkins.qa.ubuntu.com/job/mir-ci/6208/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5805
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4712
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5761
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/388/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/532
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/532/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/532
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/532/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5758
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5758/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8168
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27321
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/384
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/384/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/238/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27329

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6208/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Thanks for the answer.

Next question:
Wouldn't truncating cookie data make it computationally invalid? Like any MAC or checksum would be...

review: Needs Information

[Brandon Schaefer (brandontschaefer)]

Yes! It would, which is why it wouldnt be supported, but our main use case is sending Empty cookies. This case would fail if we created cookies that were to large for our internal storage but that internal storage *should* hopefully go away when i finish going to captnproto in the input layer so its a vector all the way.

For now its safer to handle with out crashing!

[Daniel van Vugt (vanvugt)]

Yeah /not crashing/ is good. Although you can also "not crash" more elegantly by treating oversized data we can't store as invalid, so store that as empty; length zero or something indicating a definite overflow. Which sounds better than storing truncated (and thus confusingly invalid) data.

[Daniel van Vugt (vanvugt)]

What I'm really saying is that we probably want to be able to tell the difference between a failed malicious spoofing attempt (invalid cookie), and non-malicious truncation (invalid cookie).

[Alan Griffiths (alan-griffiths)]

Let me try paraphrasing the case for this MP to see if I've got it:

1. mir::cookie::Blob isn't part of the mircookie, it is a temporary convenience for the event serialization code. (OTOH mir::cookie::default_blob_size from the same private header is used in mircookie.)

2. We don't provide a conversion (ctor, function, etc.) between std::vector<uint8_t> and mir::cookie::Blob but instead repeat the logic wherever we need this conversion.

3. The crash happens because this temporary convenience makes assumptions about serialized cookies that are invalidated by USC's replacement for CookieAuthority.

But...

4. This is a temporary mess to be cleaned up as part of a rework of the aforementioned event serialization code.

~~~~

If I've got that right, isn't a simpler solution to have USC's replacement for Cookie::serialize() return a dummy vector of the expected size?

review: Needs Information

[Brandon Schaefer (brandontschaefer)]

Yes that was one quick fix I thought about ... which im 100% fine with. Just thought this would be better :). Im more then happy to have this quick hack in USC vs this large change/hack here.

[Daniel van Vugt (vanvugt)]

Enough blocking from me.

It certainly seems like an improvement, even if some people want a different improvement.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> Yes that was one quick fix I thought about ... which im 100% fine with. Just
> thought this would be better :). Im more then happy to have this quick hack in
> USC vs this large change/hack here.

+1

The promised changes to event serialization should cover the preexisting encapsulation issues (as discussed).

review: Disapprove

Unmerged revisions

3279. By Brandon Schaefer on 2016-02-04

* Fix test names
* Use wait cond vs spin wait

3278. By Brandon Schaefer on 2016-02-03

* Dont change that space

3277. By Brandon Schaefer on 2016-02-03

* Test we can override a stub cookie factory and accept cookie size thats less then our internal storage size

3276. By Brandon Schaefer on 2016-02-03

* The current test, some strange hangup on a surface release or thread join (if you skip the release)

3275. By Brandon Schaefer on 2016-01-29

* We throw on NULL cookies and handle empty/large cookies now

=== modified file 'src/server/input/default_event_builder.cpp'
--- src/server/input/default_event_builder.cpp	2016-01-29 08:18:22 +0000
+++ src/server/input/default_event_builder.cpp	2016-02-04 13:40:07 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2015 Canonical Ltd.
+ * Copyright © 2015-2016 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 3,
@@ -54,8 +54,10 @@
     if (action == mir_touch_action_up || action == mir_touch_action_down)
     {
         auto const cookie = cookie_authority->make_cookie(event.motion.event_time.count());
+
         auto const serialized_cookie = cookie->serialize();
-        std::copy_n(std::begin(serialized_cookie), event.motion.cookie.size(), std::begin(event.motion.cookie));
+        auto const bytes_to_copy = std::min(serialized_cookie.size(), event.motion.cookie.size());
+        std::copy_n(std::begin(serialized_cookie), bytes_to_copy, std::begin(event.motion.cookie));
     }
 
     me::add_touch(event, touch_id, action, tooltype, x_axis_value, y_axis_value, pressure_value, touch_major_value,

=== modified file 'src/server/input/key_repeat_dispatcher.cpp'
--- src/server/input/key_repeat_dispatcher.cpp	2016-01-29 08:18:22 +0000
+++ src/server/input/key_repeat_dispatcher.cpp	2016-02-04 13:40:07 +0000
@@ -122,7 +122,8 @@
                 ev.key.event_time = std::chrono::steady_clock::now().time_since_epoch();
                 auto const cookie = cookie_authority->make_cookie(ev.key.event_time.count());
                 auto const serialized_cookie = cookie->serialize();
-                std::copy_n(std::begin(serialized_cookie), ev.key.cookie.size(), std::begin(ev.key.cookie));
+                auto const bytes_to_copy = std::min(serialized_cookie.size(), ev.key.cookie.size());
+                std::copy_n(std::begin(serialized_cookie), bytes_to_copy, std::begin(ev.key.cookie));
                 next_dispatcher->dispatch(ev);
 
                 capture_alarm->reschedule_in(repeat_delay);

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2016-01-29 08:18:22 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2016-02-04 13:40:07 +0000
@@ -51,6 +51,7 @@
   test_surface_specification.cpp
   test_system_compositor_window_manager.cpp
   test_session_mediator_report.cpp
+  test_server_cookie_authority.cpp
   test_surface_raise.cpp
   test_client_cookie.cpp
 )

=== added file 'tests/acceptance-tests/test_server_cookie_authority.cpp'
--- tests/acceptance-tests/test_server_cookie_authority.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_server_cookie_authority.cpp	2016-02-04 13:40:07 +0000
@@ -0,0 +1,196 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Brandon Schaefer <[e-mail]>
+ */
+
+#include "mir_test_framework/stub_server_platform_factory.h"
+#include "mir_test_framework/connected_client_with_a_surface.h"
+#include "mir/test/wait_condition.h"
+
+#include "mir/input/input_device_info.h"
+#include "mir_test_framework/fake_input_device.h"
+
+#include "mir/cookie/cookie.h"
+#include "mir/cookie/authority.h"
+
+#include <linux/input.h>
+
+namespace mtf = mir_test_framework;
+namespace mt = mir::test;
+namespace mi = mir::input;
+namespace mis = mir::input::synthesis;
+
+namespace
+{
+std::chrono::seconds const max_wait{4};
+void event_capturing_callback(MirSurface*, MirEvent const* ev, void* ctx);
+}
+
+class EmptyCookie : public mir::cookie::Cookie
+{
+public:
+    virtual uint64_t timestamp() const override
+    {
+        return 0;
+    }
+
+    virtual std::vector<uint8_t> serialize() const override
+    {
+        return std::vector<uint8_t>();
+    }
+};
+
+class EmptyCookieAuthority : public mir::cookie::Authority
+{
+public:
+    virtual std::unique_ptr<mir::cookie::Cookie> make_cookie(uint64_t const& /*timestamp*/) override
+    {
+        return std::make_unique<EmptyCookie>();
+    }
+
+    virtual std::unique_ptr<mir::cookie::Cookie> make_cookie(std::vector<uint8_t> const& /*raw_cookie*/) override
+    {
+        return std::make_unique<EmptyCookie>();
+    }
+};
+
+struct FocusExposeSurface : mtf::ConnectedClientWithASurface
+{
+    void SetUp() override
+    {
+        mtf::ConnectedClientWithASurface::SetUp();
+
+        // Need fullscreen for the cursor events
+        auto const spec = mir_connection_create_spec_for_changes(connection);
+        mir_surface_spec_set_fullscreen_on_output(spec, 1);
+        mir_surface_apply_spec(surface, spec);
+        mir_surface_spec_release(spec);
+
+        mir_surface_set_event_handler(surface, &event_capturing_callback, this);
+        mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(surface));
+
+        ready_to_accept_events.wait_for_at_most_seconds(max_wait);
+        if (!ready_to_accept_events.woken())
+            BOOST_THROW_EXCEPTION(std::runtime_error("Timeout waiting for surface to become focused and exposed"));
+    }
+
+    std::unique_ptr<mtf::FakeInputDevice> fake_keyboard{
+        mtf::add_fake_input_device(mi::InputDeviceInfo{"keyboard", "keyboard-uid" , mi::DeviceCapability::keyboard})
+        };
+    std::unique_ptr<mtf::FakeInputDevice> fake_pointer{
+        mtf::add_fake_input_device(mi::InputDeviceInfo{"mouse", "mouse-uid" , mi::DeviceCapability::pointer})
+        };
+    std::unique_ptr<mtf::FakeInputDevice> fake_touch_screen{
+       mtf::add_fake_input_device(mi::InputDeviceInfo{
+       "touch screen", "touch-screen-uid", mi::DeviceCapability::touchscreen | mi::DeviceCapability::multitouch})
+       };
+
+    mir::test::WaitCondition ready_to_accept_events;
+    mir::test::WaitCondition cookie_event_received;
+    mutable std::mutex mutex;
+    bool exposed{false};
+    bool focused{false};
+};
+
+struct EmptyCookieAuthorityOverride : FocusExposeSurface
+{
+    EmptyCookieAuthorityOverride()
+    {
+        server.override_the_cookie_authority([this] ()
+            { return std::make_unique<EmptyCookieAuthority>(); });
+    }
+};
+
+namespace
+{
+void event_capturing_callback(MirSurface* /*surface*/, MirEvent const* ev, void* ctx)
+{
+    auto const event_type = mir_event_get_type(ev);
+    auto focused_surface = static_cast<FocusExposeSurface*>(ctx);
+
+    if (event_type == mir_event_type_surface)
+    {
+        auto event = mir_event_get_surface_event(ev);
+        auto const attrib = mir_surface_event_get_attribute(event);
+        auto const value = mir_surface_event_get_attribute_value(event);
+
+        std::lock_guard<std::mutex> lk(focused_surface->mutex);
+
+        if (attrib == mir_surface_attrib_visibility &&
+            value == mir_surface_visibility_exposed)
+        {
+            focused_surface->exposed = true;
+        }
+
+        if (attrib == mir_surface_attrib_focus &&
+            value == mir_surface_focused)
+        {
+            focused_surface->focused = true;
+        }
+
+        if (focused_surface->exposed && focused_surface->focused && !focused_surface->ready_to_accept_events.woken())
+        {
+            focused_surface->ready_to_accept_events.wake_up_everyone();
+        }
+    }
+    else if (event_type == mir_event_type_input)
+    {
+        auto const* iev = mir_event_get_input_event(ev);
+
+        std::lock_guard<std::mutex> lk(focused_surface->mutex);
+        if (mir_input_event_has_cookie(iev))
+        {
+            focused_surface->cookie_event_received.wake_up_everyone();
+        }
+    }
+}
+}
+
+TEST_F(EmptyCookieAuthorityOverride, touch_events_handle_empty_cookie)
+{ 
+    fake_touch_screen->emit_event(
+         mis::a_touch_event()
+        .at_position({0, 0})
+        );
+
+    fake_touch_screen->emit_event(
+         mis::a_touch_event()
+        .with_action(mis::TouchParameters::Action::Move)
+        .at_position({1, 1})
+        );
+
+    cookie_event_received.wait_for_at_most_seconds(max_wait);
+    EXPECT_TRUE(cookie_event_received.woken());
+}
+
+TEST_F(EmptyCookieAuthorityOverride, pointer_events_handle_empty_cookie)
+{
+    fake_pointer->emit_event(mis::a_button_down_event().of_button(BTN_LEFT).with_action(mis::EventAction::Down));
+    fake_pointer->emit_event(mis::a_button_up_event().of_button(BTN_LEFT));
+
+    cookie_event_received.wait_for_at_most_seconds(max_wait);
+    EXPECT_TRUE(cookie_event_received.woken());
+}
+
+TEST_F(EmptyCookieAuthorityOverride, key_events_handle_empty_cookie)
+{
+    fake_keyboard->emit_event(mis::a_key_down_event().of_scancode(KEY_M));
+
+    cookie_event_received.wait_for_at_most_seconds(max_wait);
+    EXPECT_TRUE(cookie_event_received.woken());
+}
+
+