Merge lp:~jamesh/mediascanner2/dbus-apparmor into lp:mediascanner2

[James Henstridge]

Limit access to the MediaScanner D-Bus interface from confined processes.

The trust-store API does not exist yet, so it just hard codes support for the "com.ubuntu.music" application.

Limit access to the MediaScanner D-Bus interface from confined processes.

This is roughly based on the equivalent code from media-hub:

http://bazaar.launchpad.net/~phablet-team/media-hub/trunk/view/head:/src/core/media/player_skeleton.cpp#L135

The trust-store API does not exist yet, so it just hard codes support for the "com.ubuntu.music" application.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:256
http://jenkins.qa.ubuntu.com/job/mediascanner2-ci/78/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-amd64-ci/19/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-armhf-ci/19/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-i386-ci/19/console

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

review: Needs Fixing (continuous-integration)

[Jussi Pakkanen (jpakkane)]

Not a security expert, but looks fine to me.

review: Approve

[lp:~jamesh/mediascanner2/dbus-apparmor]

257. By James Henstridge on 2014-05-28

Include a timeout on the QML test.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:257
http://jenkins.qa.ubuntu.com/job/mediascanner2-ci/79/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-amd64-ci/20/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-armhf-ci/20/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-i386-ci/20/console

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

review: Needs Fixing (continuous-integration)

[Tyler Hicks (tyhicks)]

* AppArmor related review comments that need fixing
  - media-hub limits access for the "com.ubuntu.music" application to
    /home/phablet/{Music,Videos}
    + http://bazaar.launchpad.net/~phablet-team/media-hub/media-hub-condensed/view/head:/src/core/media/player_skeleton.cpp#L200
    + MediaScanner should probably do the same thing
    + However, note that mediahub is currently not doing this quite right since
      the path should *equal* /home/phablet/Music or /home/phablet/Vidoes,
      rather than just containing the strings Music/ or Videos/

* AppArmor related review comments that should be fixed but are non-blockers
  - You may want to make the handling of GetConnectionAppArmorSecurityContext()
    more robust
    + When AppArmor is disabled in the kernel or in dbus-daemon,
      GetConnectionAppArmorSecurityContext() will return the
      DBUS_ERROR_APPARMOR_SECURITY_CONTEXT_UNKNOWN error
    + In the current merge proposal, this will always result in the client
      being denied access
    + We may want to link against libapparmor to call aa_is_enabled() and, if
      AppArmor is disabled, always grant access before even calling
      GetConnectionAppArmorSecurityContext()
  - does_client_have_access() is very Ubuntu Touch centric
    + Some confined clients may not be Click Apps, so they may not contains '_'
      chars in their AppArmor confinement context
    + This is fine for now, but in the future we will want to handle
      applications that are applications that are confined by AppArmor but are
      not Click Apps

* Minor things that caught my eye
  - The bus variable in get_client_apparmor_context() is unused
  - The MediaType parameter in does_client_have_access() is unused
    + Because of this, the MediaType parameter of check_access is also unused

review: Needs Fixing

[lp:~jamesh/mediascanner2/dbus-apparmor]

258. By James Henstridge on 2014-05-29

If apparmor is not enabled, treat d-bus peers as unconfined.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:258
http://jenkins.qa.ubuntu.com/job/mediascanner2-ci/80/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-amd64-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-armhf-ci/21
        deb: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-armhf-ci/21/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mediascanner2-utopic-i386-ci/21

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

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

> * AppArmor related review comments that need fixing
> - media-hub limits access for the "com.ubuntu.music" application to
> /home/phablet/{Music,Videos}
> + http://bazaar.launchpad.net/~phablet-team/media-hub/media-hub-
> condensed/view/head:/src/core/media/player_skeleton.cpp#L200
> + MediaScanner should probably do the same thing
> + However, note that mediahub is currently not doing this quite right
> since
> the path should *equal* /home/phablet/Music or /home/phablet/Vidoes,
> rather than just containing the strings Music/ or Videos/

Currently the media scanner is only scanning ~/Music and ~/Videos and external media. Most of the APIs are centred around media types rather than locations though. I'm not sure what the best answer is here.

>
> * AppArmor related review comments that should be fixed but are non-blockers
> - You may want to make the handling of
> GetConnectionAppArmorSecurityContext()
> more robust
> + When AppArmor is disabled in the kernel or in dbus-daemon,
> GetConnectionAppArmorSecurityContext() will return the
> DBUS_ERROR_APPARMOR_SECURITY_CONTEXT_UNKNOWN error
> + In the current merge proposal, this will always result in the client
> being denied access
> + We may want to link against libapparmor to call aa_is_enabled() and, if
> AppArmor is disabled, always grant access before even calling
> GetConnectionAppArmorSecurityContext()
> - does_client_have_access() is very Ubuntu Touch centric
> + Some confined clients may not be Click Apps, so they may not contains
> '_'
> chars in their AppArmor confinement context
> + This is fine for now, but in the future we will want to handle
> applications that are applications that are confined by AppArmor but are
> not Click Apps

I've updated the code to call aa_is_enabled(), and treat peers as unconfined if it returns false. That seems to help get the code through Jenkins (and presumably also Launchpad's builder).

I agree that it would be good to externalise the security policy, but this seems to be the kind of thing media-hub is also doing for now. I'd be happy to change this once we've got an idea of how to do that.

>
> * Minor things that caught my eye
> - The bus variable in get_client_apparmor_context() is unused
> - The MediaType parameter in does_client_have_access() is unused
> + Because of this, the MediaType parameter of check_access is also unused

I've switched the policy over to allowing com.ubuntu.music to access AudioMedia, so the MediaType parameter is now used (I guess I missed this when I got distracted by the Jenkins failure). I've removed the unused bus variable in get_client_apparmor_context().

[Tyler Hicks (tyhicks)]

Everything looks good to me. Thanks!

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-05-29 07:30:29 +0000
+++ CMakeLists.txt	2014-05-29 07:30:30 +0000
@@ -16,6 +16,7 @@
 )
 pkg_check_modules(GST gstreamer-1.0 gstreamer-pbutils-1.0 REQUIRED)
 pkg_check_modules(DBUSCPP dbus-cpp REQUIRED)
+pkg_check_modules(APPARMOR libapparmor REQUIRED)
 find_package(Threads REQUIRED)
 find_package(Qt5Core REQUIRED)
 
 
=== modified file 'debian/control'
--- debian/control	2014-05-29 07:30:29 +0000
+++ debian/control	2014-05-29 07:30:30 +0000
@@ -6,6 +6,7 @@
 Build-Depends: cmake,
                dbus,
                debhelper (>= 9),
+               libapparmor-dev,
                libdbus-1-dev,
                libdbus-cpp-dev (>= 3.0.0),
                libglib2.0-dev,
 
=== modified file 'src/ms-dbus/CMakeLists.txt'
--- src/ms-dbus/CMakeLists.txt	2014-05-29 07:30:29 +0000
+++ src/ms-dbus/CMakeLists.txt	2014-05-29 07:30:30 +0000
@@ -1,5 +1,5 @@
 include_directories(..)
-add_definitions(${MEDIASCANNER_CFLAGS} ${DBUSCPP_CFLAGS})
+add_definitions(${MEDIASCANNER_CFLAGS} ${DBUSCPP_CFLAGS} ${APPARMOR_CFLAGS})
 
 add_library(ms-dbus STATIC
   dbus-codec.cc
@@ -7,7 +7,10 @@
   service-stub.cc
 )
 
-target_link_libraries(ms-dbus mediascanner ${DBUSCPP_LDFLAGS})
+target_link_libraries(ms-dbus
+  mediascanner
+  ${DBUSCPP_LDFLAGS}
+  ${APPARMOR_LDFLAGS})
 # Compile with -fPIC, since this code is linked into the QML plugin.
 set_target_properties(ms-dbus PROPERTIES COMPILE_FLAGS "-fPIC")
 
 
=== modified file 'src/ms-dbus/dbus-interface.hh'
--- src/ms-dbus/dbus-interface.hh	2014-05-29 07:30:29 +0000
+++ src/ms-dbus/dbus-interface.hh	2014-05-29 07:30:30 +0000
@@ -41,6 +41,13 @@
                 return s;
             }
         };
+
+        struct Unauthorized {
+            inline static const std::string& name() {
+                static std::string s = "com.canonical.MediaScanner2.Error.Unauthorized";
+                return s;
+            }
+        };
     };
 
     struct Lookup {
 
=== modified file 'src/ms-dbus/service-skeleton.cc'
--- src/ms-dbus/service-skeleton.cc	2014-05-29 07:30:29 +0000
+++ src/ms-dbus/service-skeleton.cc	2014-05-29 07:30:30 +0000
@@ -3,6 +3,7 @@
 #include <core/dbus/message.h>
 #include <core/dbus/object.h>
 #include <core/dbus/types/object_path.h>
+#include <sys/apparmor.h>
 
 #include <mediascanner/Album.hh>
 #include <mediascanner/MediaFile.hh>
@@ -17,6 +18,26 @@
 namespace mediascanner {
 namespace dbus {
 
+struct Apparmor {
+    static const std::string &name() {
+        static std::string s = "org.freedesktop.DBus";
+        return s;
+    }
+
+    struct GetConnectionAppArmorSecurityContext {
+        typedef Apparmor Interface;
+
+        static const std::string &name() {
+            static std::string s = "GetConnectionAppArmorSecurityContext";
+            return s;
+        }
+
+        static const std::chrono::milliseconds default_timeout() {
+            return std::chrono::seconds{1};
+        }
+    };
+};
+
 struct ServiceSkeleton::Private {
     ServiceSkeleton *impl;
     std::shared_ptr<MediaStore> store;
@@ -69,7 +90,69 @@
                 std::placeholders::_1));
     }
 
+    std::string get_client_apparmor_context(const Message::Ptr &message) {
+        if (!aa_is_enabled()) {
+            return "unconfined";
+        }
+        auto service = core::dbus::Service::use_service(
+            impl->access_bus(), "org.freedesktop.DBus");
+        auto obj = service->object_for_path(
+            core::dbus::types::ObjectPath("/org/freedesktop/DBus"));
+
+        core::dbus::Result<std::string> result;
+        try {
+            result = obj->invoke_method_synchronously<Apparmor::GetConnectionAppArmorSecurityContext, std::string>(message->sender());
+        } catch (const std::runtime_error &e) {
+            fprintf(stderr, "Error getting apparmor context: %s\n", e.what());
+            return std::string();
+        }
+        if (result.is_error()) {
+            fprintf(stderr, "Error getting apparmor context: %s\n", result.error().print().c_str());
+            return std::string();
+        }
+        return result.value();
+    }
+
+    bool does_client_have_access(const std::string &context, MediaType type) {
+        if (context.empty()) {
+            // Deny access if we don't have a context
+            return false;
+        }
+        if (context == "unconfined") {
+            // Unconfined
+            return true;
+        }
+
+        auto pos = context.find_first_of('_');
+        if (pos == std::string::npos) {
+            fprintf(stderr, "Badly formed AppArmor context: %s\n", context.c_str());
+            return false;
+        }
+        const std::string pkgname = context.substr(0, pos);
+
+        // TODO: when the trust store lands, check it to see if this
+        // app can access the index.
+        if (type == AudioMedia && pkgname == "com.ubuntu.music") {
+            return true;
+        }
+        return false;
+    }
+
+    bool check_access(const Message::Ptr &message, MediaType type) {
+        const std::string context = get_client_apparmor_context(message);
+        bool have_access = does_client_have_access(context, type);
+        if (!have_access) {
+            auto reply = Message::make_error(
+                message, MediaStoreInterface::Errors::Unauthorized::name(), "Unauthorized");
+            impl->access_bus()->send(reply);
+        }
+        return have_access;
+    }
+
     void handle_lookup(const Message::Ptr &message) {
+        if (!check_access(message, AllMedia))
+            return;
+
         std::string filename;
         message->reader() >> filename;
         Message::Ptr reply;
@@ -89,8 +172,11 @@
         std::string query;
         int32_t type;
         int32_t limit;
-
         message->reader() >> query >> type >> limit;
+
+        if (!check_access(message, (MediaType)type))
+            return;
+
         Message::Ptr reply;
         try {
             auto results = store->query(query, (MediaType)type, limit);
@@ -105,9 +191,11 @@
     }
 
     void handle_query_albums(const Message::Ptr &message) {
+        if (!check_access(message, AudioMedia))
+            return;
+
         std::string query;
         int32_t limit;
-
         message->reader() >> query >> limit;
         Message::Ptr reply;
         try {
@@ -123,8 +211,10 @@
     }
 
     void handle_get_album_songs(const Message::Ptr &message) {
+        if (!check_access(message, AudioMedia))
+            return;
+
         Album album("", "");
-
         message->reader() >> album;
         Message::Ptr reply;
         try {
@@ -140,6 +230,9 @@
     }
 
     void handle_get_etag(const Message::Ptr &message) {
+        if (!check_access(message, AllMedia))
+            return;
+
         std::string filename;
         message->reader() >> filename;
 
@@ -157,9 +250,11 @@
     }
 
     void handle_list_songs(const Message::Ptr &message) {
+        if (!check_access(message, AudioMedia))
+            return;
+
         std::string artist, album, album_artist;
         int32_t limit;
-
         message->reader() >> artist >> album >> album_artist >> limit;
         Message::Ptr reply;
         try {
@@ -175,9 +270,11 @@
     }
 
     void handle_list_albums(const Message::Ptr &message) {
+        if (!check_access(message, AudioMedia))
+            return;
+
         std::string artist, album_artist;
         int32_t limit;
-
         message->reader() >> artist >> album_artist >> limit;
         Message::Ptr reply;
         try {
@@ -193,9 +290,11 @@
     }
 
     void handle_list_artists(const Message::Ptr &message) {
+        if (!check_access(message, AudioMedia))
+            return;
+
         bool album_artists;
         int32_t limit;
-
         message->reader() >> album_artists >> limit;
         Message::Ptr reply;
         try {
 
=== modified file 'test/CMakeLists.txt'
--- test/CMakeLists.txt	2014-05-29 07:30:29 +0000
+++ test/CMakeLists.txt	2014-05-29 07:30:30 +0000
@@ -51,8 +51,9 @@
 qt5_use_modules(test_qml QuickTest)
 target_link_libraries(test_qml mediascanner ${DBUSCPP_LDFLAGS})
 add_test(test_qml test_qml -input ${CMAKE_CURRENT_SOURCE_DIR}/qml -import ${CMAKE_BINARY_DIR}/src/qml)
-set_tests_properties(test_qml
-  PROPERTIES ENVIRONMENT "QT_QPA_PLATFORM=minimal")
+set_tests_properties(test_qml PROPERTIES
+  ENVIRONMENT "QT_QPA_PLATFORM=minimal"
+  TIMEOUT 600)
 
 add_executable(test_util test_util.cc ../src/daemon/util.cc)
 target_link_libraries(test_util gtest)