Merge lp:~jamesh/thumbnailer/dbus-aa-credentials into lp:thumbnailer/devel

[James Henstridge]

Track the credentials (user ID, AppArmor label) of clients connecting to the D-Bus service. This is not yet used to make security decisions.

This is the first step of the changing our security policy to rely on aa_query_label().

I've introduced a new step for the request handler to determine the AppArmor security context of the client. At the moment we're only printing it out in a log message, but eventually this information will be pushed down to the ThumbnailRequest where it can be used to make the security decision.

You can test this on the desktop by running thumbnailer-service in one terminal and in another run something like:

    aa-exec -p $profile thumbnailer-admin get $filename outdir/

You can get a list of available profiles on the system with "sudo aa-status". When run on the phone, you should see the labels for confined clients.

To avoid excessive GetConnectionCredentials() calls to the bus daemon, we cache the results in the CredentialsCache class.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:220
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/269/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/78
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/78
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/78/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/78

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/269/rebuild

review: Approve (continuous-integration)

[lp:~jamesh/thumbnailer/dbus-aa-credentials]

221. By James Henstridge on 2015-06-12

A few style changes.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:221
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/270/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/79
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/79
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/79/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/79

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/270/rebuild

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Looks really nice, thank you!

Just to show that I read it, I have to make at least one anal-retentive comment ;-)

qWarning() << "CredentialsCache::received_credentials: error retrieving credentials for" << peer << ":" << reply.error().message();

Should be

... CredentialsCache::received_credentials(): error ...

Splitting the line into two shorter ones would make it a little bit more readable.

review: Needs Fixing

[lp:~jamesh/thumbnailer/dbus-aa-credentials]

222. By James Henstridge on 2015-06-15

Make qml_test depend on thumbnailer-qml.

223. By James Henstridge on 2015-06-15

Fix up warning message, from Michi's review.

[Michi Henning (michihenning)]

Sweet, thank you!

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-06-06 08:39:23 +0000
+++ CMakeLists.txt	2015-06-15 01:16:40 +0000
@@ -97,6 +97,7 @@
 pkg_check_modules(GIO_DEPS REQUIRED gio-2.0 gio-unix-2.0)
 pkg_check_modules(IMG_DEPS REQUIRED gdk-pixbuf-2.0 libexif)
 pkg_check_modules(UNITY_API_DEPS REQUIRED libunity-api)
+pkg_check_modules(APPARMOR_DEPS REQUIRED libapparmor)
 
 include_directories(${GST_DEPS_INCLUDE_DIRS})
 include_directories(${GOBJ_DEPS_INCLUDE_DIRS})

=== modified file 'debian/control'
--- debian/control	2015-05-05 09:19:37 +0000
+++ debian/control	2015-06-15 01:16:40 +0000
@@ -7,6 +7,7 @@
                cmake-extras,
                debhelper (>= 9),
                gstreamer1.0-plugins-good,
+               libapparmor-dev,
                libboost-filesystem-dev,
                libexif-dev,
                libgdk-pixbuf2.0-dev,

=== modified file 'src/service/CMakeLists.txt'
--- src/service/CMakeLists.txt	2015-06-08 03:25:50 +0000
+++ src/service/CMakeLists.txt	2015-06-15 01:16:40 +0000
@@ -1,10 +1,15 @@
-add_definitions(${THUMBNAILER_CFLAGS})
+add_definitions(${APPARMOR_DEPS_CFLAGS})
 
 qt5_add_dbus_adaptor(adaptor_files dbusinterface.xml dbusinterface.h unity::thumbnailer::service::DBusInterface)
 qt5_add_dbus_adaptor(adaptor_files admininterface.xml admininterface.h unity::thumbnailer::service::AdminInterface)
 
+set_source_files_properties(bus.xml PROPERTIES
+  CLASSNAME BusInterface)
+qt5_add_dbus_interface(interface_files bus.xml businterface)
+
 add_executable(thumbnailer-service
   admininterface.cpp
+  credentialscache.cpp
   dbusinterface.cpp
   handler.cpp
   inactivityhandler.cpp
@@ -12,10 +17,11 @@
   ratelimiter.cpp
   stats.cpp
   ${adaptor_files}
+  ${interface_files}
   )
 
 qt5_use_modules(thumbnailer-service DBus Concurrent)
-target_link_libraries(thumbnailer-service thumbnailer ${CMAKE_THREAD_LIBS_INIT})
+target_link_libraries(thumbnailer-service thumbnailer ${CMAKE_THREAD_LIBS_INIT} ${APPARMOR_DEPS_LDFLAGS})
 set_target_properties(thumbnailer-service PROPERTIES AUTOMOC TRUE)
 add_dependencies(thumbnailer-service vs-thumb)
 

=== added file 'src/service/bus.xml'
--- src/service/bus.xml	1970-01-01 00:00:00 +0000
+++ src/service/bus.xml	2015-06-15 01:16:40 +0000
@@ -0,0 +1,9 @@
+<node>
+  <interface name="org.freedesktop.DBus">
+    <method name="GetConnectionCredentials">
+      <arg direction="in" type="s" name="bus_name" />
+      <arg direction="out" type="a{sv}" name="credentials" />
+      <annotation name="org.qtproject.QtDBus.QtTypeName.Out0" value="QVariantMap" />
+    </method>
+  </interface>
+</node>

=== added file 'src/service/credentialscache.cpp'
--- src/service/credentialscache.cpp	1970-01-01 00:00:00 +0000
+++ src/service/credentialscache.cpp	2015-06-15 01:16:40 +0000
@@ -0,0 +1,179 @@
+/*
+ * Copyright (C) 2015 Canonical, Ltd.
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of version 3 of the GNU General Public License as published
+ * by the Free Software Foundation.
+ *
+ * This library 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/>.
+ *
+ * Authors:
+ *    James Henstridge <[e-mail]>
+ */
+
+#include "credentialscache.h"
+
+#include <QDBusPendingCallWatcher>
+
+#include <assert.h>
+#include <vector>
+#include <sys/apparmor.h>
+
+using namespace std;
+
+namespace {
+
+char const DBUS_BUS_NAME[] = "org.freedesktop.DBus";
+char const DBUS_BUS_PATH[] = "/org/freedesktop/DBus";
+
+char const UNIX_USER_ID[] = "UnixUserID";
+char const LINUX_SECURITY_LABEL[] = "LinuxSecurityLabel";
+
+int const MAX_CACHE_SIZE = 50;
+
+}
+
+namespace unity
+{
+
+namespace thumbnailer
+{
+
+namespace service
+{
+
+struct CredentialsCache::Request
+{
+    QDBusPendingCallWatcher watcher;
+    std::vector<CredentialsCache::Callback> callbacks;
+
+    Request(QDBusPendingReply<QVariantMap> call) : watcher(call) {}
+};
+
+CredentialsCache::CredentialsCache(QDBusConnection const& bus)
+    : bus_daemon_(DBUS_BUS_NAME, DBUS_BUS_PATH, bus)
+    , apparmor_enabled_(aa_is_enabled())
+{
+}
+
+CredentialsCache::~CredentialsCache() = default;
+
+void CredentialsCache::get(QString const& peer, Callback callback)
+{
+    // Return the credentials directly if they are cached
+    try
+    {
+        Credentials const& credentials = cache_.at(peer);
+        callback(credentials);
+        return;
+    }
+    catch (std::out_of_range const &)
+    {
+        // ignore
+    }
+
+    // If the credentials exist in the previous generation of the
+    // cache, move them to the current generation.
+    try
+    {
+        Credentials& credentials = old_cache_.at(peer);
+        cache_.emplace(peer, std::move(credentials));
+        old_cache_.erase(peer);
+        callback(cache_.at(peer));
+        return;
+    }
+    catch (std::out_of_range const &)
+    {
+        // ignore
+    }
+
+    // If the credentials are already being requested, add ourselves
+    // to the callback list.
+    try
+    {
+        unique_ptr<Request>& request = pending_.at(peer);
+        request->callbacks.push_back(callback);
+        return;
+    }
+    catch (std::out_of_range const &)
+    {
+        // ignore
+    }
+
+    // Ask the bus daemon for the peer's credentials
+    unique_ptr<Request> request(
+        new Request(bus_daemon_.GetConnectionCredentials(peer)));
+    QObject::connect(&request->watcher, &QDBusPendingCallWatcher::finished,
+                     [this, peer](QDBusPendingCallWatcher *watcher)
+                     {
+                         this->received_credentials(peer, *watcher);
+                     });
+    request->callbacks.push_back(callback);
+    pending_.emplace(peer, std::move(request));
+}
+
+void CredentialsCache::received_credentials(QString const& peer, QDBusPendingReply<QVariantMap> reply)
+{
+    Credentials credentials;
+    if (reply.isError())
+    {
+        qWarning() << "CredentialsCache::received_credentials(): "
+            "error retrieving credentials for" << peer <<
+            ":" << reply.error().message();
+    }
+    else
+    {
+        credentials.valid = true;
+        // The contents of this map are described in the specification here:
+        // http://dbus.freedesktop.org/doc/dbus-specification.html#bus-messages-get-connection-credentials
+        credentials.user = reply.value().value(UNIX_USER_ID).value<uint32_t>();
+        if (apparmor_enabled_)
+        {
+            QByteArray label = reply.value().value(LINUX_SECURITY_LABEL).value<QByteArray>();
+            if (label.size() > 0) {
+                // The label is null terminated.
+                assert(label[label.size()-1] == '\0');
+                label.truncate(label.size() - 1);
+                // Trim the mode off the end of the label.
+                int pos = label.lastIndexOf(' ');
+                if (pos > 0 && label.endsWith(')') && label[pos+1] == '(')
+                {
+                    label.truncate(pos);
+                }
+                credentials.label = string(label.constData(), label.size());
+            }
+        }
+        else
+        {
+            // If AppArmor is not enabled, treat peer as unconfined.
+            credentials.label = "unconfined";
+        }
+    }
+
+    // If we've hit our maximum cache size, start a new generation.
+    if (cache_.size() >= MAX_CACHE_SIZE)
+    {
+        old_cache_ = std::move(cache_);
+        cache_.clear();
+    }
+    cache_.emplace(peer, credentials);
+
+    // Notify anyone waiting on the request and remove it from the map:
+    for (auto& callback : pending_.at(peer)->callbacks)
+    {
+        callback(credentials);
+    }
+    pending_.erase(peer);
+}
+
+}  // namespace service
+
+}  // namespace thumbnailer
+
+}  // namespace unity

=== added file 'src/service/credentialscache.h'
--- src/service/credentialscache.h	1970-01-01 00:00:00 +0000
+++ src/service/credentialscache.h	2015-06-15 01:16:40 +0000
@@ -0,0 +1,81 @@
+/*
+ * Copyright (C) 2015 Canonical, Ltd.
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of version 3 of the GNU General Public License as published
+ * by the Free Software Foundation.
+ *
+ * This library 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/>.
+ *
+ * Authors:
+ *    James Henstridge <[e-mail]>
+ */
+
+#pragma once
+
+#include "businterface.h"
+
+#include <QDBusConnection>
+#include <QDBusPendingCall>
+#include <QString>
+
+#include <functional>
+#include <map>
+#include <memory>
+#include <string>
+#include <sys/types.h>
+
+namespace unity
+{
+
+namespace thumbnailer
+{
+
+namespace service
+{
+
+
+class CredentialsCache final {
+public:
+    struct Credentials
+    {
+        bool valid = false;
+        uid_t user = 0;
+        // Not using QString, because this is not necessarily unicode.
+        std::string label;
+    };
+    typedef std::function<void(Credentials const&)> Callback;
+
+    CredentialsCache(QDBusConnection const& bus);
+    ~CredentialsCache();
+
+    CredentialsCache(CredentialsCache const&) = delete;
+    CredentialsCache& operator=(CredentialsCache const&) = delete;
+
+    // Retrieve the security credentials for the given D-Bus peer.
+    void get(QString const& peer, Callback callback);
+
+private:
+    struct Request;
+
+    BusInterface bus_daemon_;
+    bool apparmor_enabled_;
+
+    std::map<QString,Credentials> cache_;
+    std::map<QString,Credentials> old_cache_;
+    std::map<QString,std::unique_ptr<Request>> pending_;
+
+    void received_credentials(QString const& peer, QDBusPendingReply<QVariantMap> reply);
+};
+
+}  // namespace service
+
+}  // namespace thumbnailer
+
+}  // namespace unity

=== modified file 'src/service/dbusinterface.cpp'
--- src/service/dbusinterface.cpp	2015-06-08 03:59:53 +0000
+++ src/service/dbusinterface.cpp	2015-06-15 01:16:40 +0000
@@ -52,6 +52,15 @@
 {
 }
 
+CredentialsCache& DBusInterface::credentials()
+{
+    if (!credentials_)
+    {
+        credentials_.reset(new CredentialsCache(connection()));
+    }
+    return *credentials_.get();
+}
+
 QDBusUnixFileDescriptor DBusInterface::GetAlbumArt(QString const& artist,
                                                    QString const& album,
                                                    QSize const& requestedSize)
@@ -64,7 +73,8 @@
         auto request = thumbnailer_->get_album_art(artist.toStdString(), album.toStdString(), requestedSize);
         queueRequest(new Handler(connection(), message(),
                                  check_thread_pool_, create_thread_pool_,
-                                 download_limiter_, std::move(request), details));
+                                 download_limiter_, credentials(),
+                                 std::move(request), details));
     }
     // LCOV_EXCL_START
     catch (exception const& e)
@@ -89,7 +99,8 @@
         auto request = thumbnailer_->get_artist_art(artist.toStdString(), album.toStdString(), requestedSize);
         queueRequest(new Handler(connection(), message(),
                                  check_thread_pool_, create_thread_pool_,
-                                 download_limiter_, std::move(request), details));
+                                 download_limiter_, credentials(),
+                                 std::move(request), details));
     }
     // LCOV_EXCL_START
     catch (exception const& e)
@@ -107,6 +118,7 @@
                                                     QSize const& requestedSize)
 {
     std::unique_ptr<ThumbnailRequest> request;
+
     try
     {
         QString details;
@@ -115,7 +127,8 @@
         auto request = thumbnailer_->get_thumbnail(filename.toStdString(), filename_fd.fileDescriptor(), requestedSize);
         queueRequest(new Handler(connection(), message(),
                                  check_thread_pool_, create_thread_pool_,
-                                 extraction_limiter_, std::move(request), details));
+                                 extraction_limiter_, credentials(),
+                                 std::move(request), details));
     }
     catch (exception const& e)
     {

=== modified file 'src/service/dbusinterface.h'
--- src/service/dbusinterface.h	2015-06-05 08:27:20 +0000
+++ src/service/dbusinterface.h	2015-06-15 01:16:40 +0000
@@ -19,6 +19,7 @@
 
 #pragma once
 
+#include "credentialscache.h"
 #include "handler.h"
 #include "ratelimiter.h"
 
@@ -65,9 +66,12 @@
     void startInactivity();
 
 private:
+    CredentialsCache& credentials();
+
     std::shared_ptr<unity::thumbnailer::internal::Thumbnailer> const& thumbnailer_;
     std::shared_ptr<QThreadPool> check_thread_pool_;
     std::shared_ptr<QThreadPool> create_thread_pool_;
+    std::unique_ptr<CredentialsCache> credentials_;
     std::map<Handler*, std::unique_ptr<Handler>> requests_;
     std::map<std::string, std::vector<Handler*>> request_keys_;
     unity::thumbnailer::internal::Settings settings_;

=== modified file 'src/service/handler.cpp'
--- src/service/handler.cpp	2015-06-08 03:25:50 +0000
+++ src/service/handler.cpp	2015-06-15 01:16:40 +0000
@@ -122,6 +122,7 @@
     shared_ptr<QThreadPool> check_pool;
     shared_ptr<QThreadPool> create_pool;
     RateLimiter& limiter;
+    CredentialsCache& creds;
     unique_ptr<ThumbnailRequest> request;
     chrono::system_clock::time_point start_time;            // Overall start time
     chrono::system_clock::time_point finish_time;           // Overall finish time
@@ -139,6 +140,7 @@
                    shared_ptr<QThreadPool> check_pool,
                    shared_ptr<QThreadPool> create_pool,
                    RateLimiter& limiter,
+                   CredentialsCache& creds,
                    unique_ptr<ThumbnailRequest>&& request,
                    QString const& details)
         : bus(bus)
@@ -146,6 +148,7 @@
         , check_pool(check_pool)
         , create_pool(create_pool)
         , limiter(limiter)
+        , creds(creds)
         , request(move(request))
         , details(details)
     {
@@ -158,9 +161,10 @@
                  shared_ptr<QThreadPool> check_pool,
                  shared_ptr<QThreadPool> create_pool,
                  RateLimiter& limiter,
+                 CredentialsCache& creds,
                  unique_ptr<ThumbnailRequest>&& request,
                  QString const& details)
-    : p(new HandlerPrivate(bus, message, check_pool, create_pool, limiter, move(request), details))
+    : p(new HandlerPrivate(bus, message, check_pool, create_pool, limiter, creds, move(request), details))
 {
     connect(&p->checkWatcher, &QFutureWatcher<FdOrError>::finished, this, &Handler::checkFinished);
     connect(p->request.get(), &ThumbnailRequest::downloadFinished, this, &Handler::downloadFinished);
@@ -182,6 +186,22 @@
 
 void Handler::begin()
 {
+    p->creds.get(p->message.service(),
+                 [this](CredentialsCache::Credentials const& credentials)
+                 {
+                     gotCredentials(credentials);
+                 });
+}
+
+void Handler::gotCredentials(CredentialsCache::Credentials const& credentials)
+{
+    if (!credentials.valid)
+    {
+        sendError("gotCredentials(): " + details() + ": could not retrieve peer credentials");
+        return;
+    }
+    qDebug() << "Peer" << p->message.service() << "has uid =" << credentials.user << "label =" << QString::fromStdString(credentials.label);
+
     auto do_check = [this]() -> FdOrError
     {
         try

=== modified file 'src/service/handler.h'
--- src/service/handler.h	2015-06-08 02:34:13 +0000
+++ src/service/handler.h	2015-06-15 01:16:40 +0000
@@ -19,6 +19,7 @@
 
 #pragma once
 
+#include "credentialscache.h"
 #include "ratelimiter.h"
 #include <internal/thumbnailer.h>
 
@@ -51,6 +52,7 @@
             std::shared_ptr<QThreadPool> check_pool,
             std::shared_ptr<QThreadPool> create_pool,
             RateLimiter& limiter,
+            CredentialsCache& creds,
             std::unique_ptr<internal::ThumbnailRequest>&& request,
             QString const& details);
     ~Handler();
@@ -78,6 +80,7 @@
 private:
     void sendThumbnail(QDBusUnixFileDescriptor const& unix_fd);
     void sendError(QString const& error);
+    void gotCredentials(CredentialsCache::Credentials const& credentials);
     QDBusUnixFileDescriptor check();
     QDBusUnixFileDescriptor create();
 

=== modified file 'tests/qml/CMakeLists.txt'
--- tests/qml/CMakeLists.txt	2015-06-04 08:45:20 +0000
+++ tests/qml/CMakeLists.txt	2015-06-15 01:16:40 +0000
@@ -1,5 +1,5 @@
 add_executable(qml_test qml_test.cpp)
 qt5_use_modules(qml_test Qml DBus QuickTest)
 target_link_libraries(qml_test testutils)
-add_dependencies(qml_test thumbnailer-service)
+add_dependencies(qml_test thumbnailer-service thumbnailer-qml)
 add_test(qml xvfb-run -a -s "-screen 0 800x600x24" ./qml_test -import ${CMAKE_BINARY_DIR}/plugins)