Merge lp:~ken-vandine/content-hub/lp1456628 into lp:content-hub

[Ken VanDine]

* SECURITY UPDATE: file disclosure via unchecked AppArmor profile
    (LP: #1456628)
  - Don't allow exporting of files that aren't allowed by the source apparmor profile
  - CVE-2015-1327

Verify the source app has read access to local files being transferred

Debs can be found at http://people.canonical.com/~kenvandine/lp1456628.tar

This can be tested by installing the content-hub-testability package and running these commands checking their exit value.

"content-hub-test-exporter content-hub-test-importer file:///etc/issue content-hub-test-ok"

Should exit 0

"content-hub-test-exporter content-hub-test-importer file:///etc/issue content-hub-test-bad"

Should exit 1

[Michael Sheldon (michael-sheldon)]

Haven't tested yet, but the code looks good (with one small query about some potential redundancy in diff comments)

[Ken VanDine (ken-vandine)]

Are there any related MPs required for this MP to build/function as expected? Please list.

 * No

Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)

 * Yes

Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?

 * Yes

Did you successfully run all tests found in your component's Test Plan (https://wiki.ubuntu.com/Process/Merges/TestPlan/content-hub) on device or emulator?

 * Yes

If you changed the UI, was the change specified/approved by design?

 * No change

If you changed UI labels, did you update the pot file?

 * No change

If you changed the packaging (debian), did you add a core-dev as a reviewer to this MP?

 * I'm a core-dev, added autopkgtests, apparmor profile for testing and build depends for apparmor

[Michael Sheldon (michael-sheldon)]

Did you perform an exploratory manual test run of the code change and any related functionality on device or emulator?

 * Yes

Did CI run pass? If not, please explain why.

 * No CI due to private MR

Have you checked that submitter has accurately filled out the submitter checklist and has taken no shortcut?

 * Yes

review: Approve

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-04-20 21:21:19 +0000
+++ CMakeLists.txt	2015-06-02 12:46:00 +0000
@@ -68,6 +68,7 @@
 pkg_check_modules(DBUS REQUIRED dbus-1)
 pkg_check_modules(UBUNTU_DOWNLOAD_MANAGER REQUIRED ubuntu-download-manager-client)
 pkg_check_modules(NOTIFY REQUIRED libnotify)
+pkg_check_modules(APPARMOR REQUIRED libapparmor)
 
 add_definitions(-DDEBUG_ENABLED)
 
 
=== added directory 'debian/apparmor'
=== added file 'debian/apparmor/content-hub-testability'
--- debian/apparmor/content-hub-testability	1970-01-01 00:00:00 +0000
+++ debian/apparmor/content-hub-testability	2015-06-02 12:46:00 +0000
@@ -0,0 +1,15 @@
+# vim:syntax=apparmor
+# Author: Ken VanDine <[e-mail]>
+#
+#include <tunables/global>
+
+profile "content-hub-test-ok" {
+    #include <abstractions/base>
+    #include <abstractions/dbus-session>
+    /etc/issue r,
+}
+profile "content-hub-test-bad" {
+    #include <abstractions/base>
+    #include <abstractions/dbus-session>
+    audit deny /etc/issue r,
+}
 
=== modified file 'debian/content-hub-testability.install'
--- debian/content-hub-testability.install	2014-08-07 19:35:30 +0000
+++ debian/content-hub-testability.install	2015-06-02 12:46:00 +0000
@@ -3,3 +3,4 @@
 usr/share/applications/content-hub-test*
 usr/share/content-hub/testability/data
 usr/share/icons/hicolor/512x512/apps/content-hub-test*
+debian/apparmor/content-hub-testability etc/apparmor.d
 
=== modified file 'debian/control'
--- debian/control	2015-04-22 14:27:27 +0000
+++ debian/control	2015-06-02 12:46:00 +0000
@@ -5,10 +5,12 @@
                click-dev,
                dbus-test-runner,
                debhelper (>= 9),
+               dh-apparmor,
                dh-translations,
                doxygen,
                google-mock,
                lcov,
+               libapparmor-dev,
                libglib2.0-dev,
                libgsettings-qt-dev,
                libnih-dbus-dev,
@@ -28,6 +30,7 @@
 Vcs-Bzr: https://code.launchpad.net/~phablet-team/content-hub/trunk
 Vcs-Browser: https://bazaar.launchpad.net/~phablet-team/content-hub/trunk/files
 X-Ubuntu-Use-Langpack: yes
+XS-Testsuite: autopkgtest
 
 Package: content-hub
 Architecture: any
@@ -88,5 +91,6 @@
 Architecture: any
 Depends: ${misc:Depends},
          ${shlibs:Depends},
+         content-hub,
 Description: content sharing testability
  Files and utilities needed for automated testing of content-hub
 
=== modified file 'debian/rules'
--- debian/rules	2014-10-10 16:13:03 +0000
+++ debian/rules	2015-06-02 12:46:00 +0000
@@ -6,6 +6,10 @@
 %:
 	dh $@ --with click,translations --fail-missing -- -B build
 
+override_dh_auto_install:
+	dh_auto_install
+	dh_apparmor -pcontent-hub-testability --profile-name=content-hub-testability
+
 override_dh_auto_test:
 	dbus-test-runner -t make -p "-C" -p "build/tests/acceptance-tests" -p test
 	
 
=== added directory 'debian/tests'
=== added file 'debian/tests/aa-check'
--- debian/tests/aa-check	1970-01-01 00:00:00 +0000
+++ debian/tests/aa-check	2015-06-02 12:46:00 +0000
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+# start X
+(Xvfb :5 >/dev/null 2>&1 &)
+XVFB_PID=$!
+export DISPLAY=:5
+
+# start local session D-BUS
+eval `dbus-launch`
+trap "kill $DBUS_SESSION_BUS_PID $XVFB_PID" 0 TERM QUIT INT
+export DBUS_SESSION_BUS_ADDRESS
+export XAUTHORITY=/dev/null
+
+oktests="content-hub-test-ok"
+badtests="content-hub-test-bad"
+FAILED=""
+
+for a in $oktests; do
+    content-hub-test-importer &2>/dev/null
+    content-hub-test-exporter content-hub-test-importer file:///etc/issue $a 2>/dev/null
+    if [ $? -ne 0 ]; then
+        FAILED="$FAILED $a"
+    fi
+done
+
+for b in $badtests; do
+    content-hub-test-importer &2>/dev/null
+    content-hub-test-exporter content-hub-test-importer file:///etc/issue $b 2>/dev/null
+    if [ $? -eq 0 ]; then
+        FAILED="$FAILED $b"
+    fi
+done
+
+if [ -z "$FAILED" ]; then
+    echo "All tests passed"
+    exit 0
+else
+    echo "$FAILED failed"
+    exit 1
+fi
+
 
=== added file 'debian/tests/control'
--- debian/tests/control	1970-01-01 00:00:00 +0000
+++ debian/tests/control	2015-06-02 12:46:00 +0000
@@ -0,0 +1,3 @@
+Tests: aa-check
+Depends: content-hub-testability, dbus-x11, xvfb
+Restrictions: allow-stderr
 
=== modified file 'src/com/ubuntu/content/CMakeLists.txt'
--- src/com/ubuntu/content/CMakeLists.txt	2014-05-26 06:45:57 +0000
+++ src/com/ubuntu/content/CMakeLists.txt	2015-06-02 12:46:00 +0000
@@ -28,6 +28,7 @@
   ${UBUNTU_LAUNCH_INCLUDE_DIRS}
   ${UBUNTU_DOWNLOAD_MANAGER_INCLUDE_DIRS}
   ${NOTIFY_INCLUDE_DIRS}
+  ${APPARMOR_INCLUDE_DIRS}
 )
 
 qt5_add_dbus_interface(
@@ -105,6 +106,7 @@
     ${GIO_LIBRARIES}
     ${UBUNTU_DOWNLOAD_MANAGER_LIBRARIES}
     ${NOTIFY_LIBRARIES}
+    ${APPARMOR_LDFLAGS}
 )
 
 install(
 
=== modified file 'src/com/ubuntu/content/detail/service.h'
--- src/com/ubuntu/content/detail/service.h	2014-11-19 18:37:26 +0000
+++ src/com/ubuntu/content/detail/service.h	2015-06-02 12:46:00 +0000
@@ -79,7 +79,7 @@
     void handle_exports(int);
     void handler_unregistered(const QString&);
     QDBusObjectPath CreateTransfer(const QString&, const QString&, int, const QString&);
-    void download_notify (com::ubuntu::content::detail::Transfer*);
+    void download_notify(com::ubuntu::content::detail::Transfer*);
 
 };
 }
 
=== modified file 'src/com/ubuntu/content/detail/transfer.cpp'
--- src/com/ubuntu/content/detail/transfer.cpp	2015-04-21 14:46:25 +0000
+++ src/com/ubuntu/content/detail/transfer.cpp	2015-06-02 12:46:00 +0000
@@ -36,7 +36,7 @@
             const QString& source,
             const QString& destination,
             const int direction,
-            const QString& content_type)         :
+            const QString& content_type):
         state(cuc::Transfer::created),
             id(id),
             source(source),
@@ -166,12 +166,28 @@
         return;
     } 
 
+    QString profile = aa_profile(message().service());
+    TRACE() << Q_FUNC_INFO << "PROFILE:" << profile;
+
     QVariantList ret;
     Q_FOREACH(QVariant iv, items) {
         cuc::Item item = qdbus_cast<Item>(iv);
         if (item.url().isEmpty()) {
             ret.append(QVariant::fromValue(item));
         } else {
+            TRACE() << Q_FUNC_INFO;
+            if (profile.toStdString() != QString("unconfined").toStdString() &&
+                item.url().isLocalFile()) {
+                TRACE() << Q_FUNC_INFO << "IS LOCAL FILE";
+                QString file(item.url().toLocalFile());
+                TRACE() << Q_FUNC_INFO << "FILE:" << file;
+                // Verify app has read access to local file before transfer
+                if (not check_profile_read(profile, file)) {
+                    // If failed to access file, abort
+                    ret.clear();
+                    goto abort;
+                }
+            }
             QString newUrl = copy_to_store(item.url().toString(), d->store);
             if (!newUrl.isEmpty()) {
                 item.setUrl(QUrl(newUrl));
@@ -179,11 +195,12 @@
                 ret.append(QVariant::fromValue(item));
             } else {
                 ret.clear();
-                break;
+                goto abort;
             }
         }
     }
 
+abort:
     if (ret.count() <= 0)
     {
         qWarning() << "Failed to charge items, aborting";
 
=== modified file 'src/com/ubuntu/content/detail/transfer.h'
--- src/com/ubuntu/content/detail/transfer.h	2014-09-26 10:34:39 +0000
+++ src/com/ubuntu/content/detail/transfer.h	2015-06-02 12:46:00 +0000
@@ -18,9 +18,11 @@
 #ifndef TRANSFER_H_
 #define TRANSFER_H_
 
+#include <QDir>
 #include <QObject>
 #include <QStringList>
-#include <QDir>
+#include <QtDBus/QDBusMessage>
+#include <QtDBus/QDBusContext>
 #include <ubuntu/download_manager/error.h>
 
 namespace com
@@ -31,7 +33,7 @@
 {
 namespace detail
 {
-class Transfer : public QObject
+class Transfer : public QObject, protected QDBusContext
 {
     Q_OBJECT
     Q_PROPERTY(int State READ State NOTIFY StateChanged)
 
=== modified file 'src/com/ubuntu/content/utils.cpp'
--- src/com/ubuntu/content/utils.cpp	2015-03-16 16:57:15 +0000
+++ src/com/ubuntu/content/utils.cpp	2015-06-02 12:46:00 +0000
@@ -16,12 +16,14 @@
  * Authored by: Ken VanDine <[e-mail]>
  */
 
+#include <QCoreApplication>
+#include <QDir>
+#include <QFile>
+#include <QFileInfo>
+#include <QProcess>
 #include <QtCore>
 #include <QtDBus/QDBusMessage>
 #include <QtDBus/QDBusConnection>
-#include <QFile>
-#include <QDir>
-#include <QFileInfo>
 #include <QUrl>
 #include <nih/alloc.h>
 #include <nih-dbus/dbus_util.h>
@@ -31,6 +33,11 @@
 #include "com/ubuntu/content/type.h"
 #include <unistd.h>
 
+#include <sys/apparmor.h>
+/* need to be exposed in libapparmor but for now ... */
+#define AA_CLASS_FILE 2
+#define AA_MAY_READ (1 << 2)
+
 namespace cuc = com::ubuntu::content;
 
 namespace {
@@ -102,10 +109,6 @@
             reply.errorMessage();
     }
 
-    if (aaProfile.toStdString() == QString("unconfined").toStdString())
-    {
-        return QString("");
-    }
     return aaProfile;
 }
 
@@ -175,4 +178,45 @@
     return false;
 }
 
+int query_file(const char *label, const char *path, int *allowed)
+{
+    int rc, audited;
+    char *query;
+
+    /* + 1 for null separator and then + 1 AA_CLASS_FILE */
+    int label_size = strlen(label);
+    int size = label_size + 1 + strlen(path) + AA_QUERY_CMD_LABEL_SIZE + 1;
+    /* +1 for null terminator used by strcpy, yes we could drop this
+     * using memcpy */
+    query = (char*)malloc(size + 1);
+    if (!query)
+        return -1;
+    /* we want the null terminator here */
+    strcpy(query + AA_QUERY_CMD_LABEL_SIZE, label);
+    query[AA_QUERY_CMD_LABEL_SIZE + label_size + 1] = AA_CLASS_FILE;
+    strcpy(query + AA_QUERY_CMD_LABEL_SIZE + label_size + 2, path);
+    rc = aa_query_label(AA_MAY_READ, query, size , allowed, &audited);
+    free(query);
+    return rc;
+}
+
+bool check_profile_read(QString profile, QString path)
+{
+    TRACE() << Q_FUNC_INFO << "PROFILE:" << profile;
+
+    int allowed;
+    if (query_file(profile.toStdString().c_str(), path.toStdString().c_str(), &allowed) == -1) {
+        qWarning() << "error:" << strerror(errno) << path;
+        return false;
+    }
+   
+    if (allowed) {
+        TRACE() << "ALLOWED:" << QString::number(allowed);
+        return true;
+    }
+    TRACE() << "NOT ALLOWED:" << QString::number(allowed);
+    return false;
+
+}
+
 }
 
=== modified file 'tests/peers/exporter/CMakeLists.txt'
--- tests/peers/exporter/CMakeLists.txt	2014-08-07 19:35:30 +0000
+++ tests/peers/exporter/CMakeLists.txt	2015-06-02 12:46:00 +0000
@@ -24,6 +24,7 @@
 )
 
 qt5_use_modules(content-hub-test-exporter Core Gui DBus)
+pkg_check_modules(APPARMOR REQUIRED libapparmor)
 
 set_target_properties(
   content-hub-test-exporter
@@ -35,6 +36,7 @@
   content-hub-test-exporter
  
   content-hub 
+  ${APPARMOR_LDFLAGS}
 )
 
 install(
 
=== modified file 'tests/peers/exporter/autoexporter.cpp'
--- tests/peers/exporter/autoexporter.cpp	2015-04-14 15:28:56 +0000
+++ tests/peers/exporter/autoexporter.cpp	2015-06-02 12:46:00 +0000
@@ -24,6 +24,12 @@
     hub->register_import_export_handler(this);
 }
 
+void AutoExporter::setUrl(QString url)
+{
+    qDebug() << Q_FUNC_INFO << url;
+    m_url = url;
+}
+
 void AutoExporter::handle_import(cuc::Transfer *transfer)
 {
     qDebug() << Q_FUNC_INFO << "not implemented";
@@ -42,19 +48,20 @@
 
     QVector<cuc::Item> items;
 
-    if (transfer->contentType() == cuc::Type::Known::contacts().id()) {
-        items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/Joker.vcf"));
-
-        if (transfer->selectionType() == cuc::Transfer::SelectionType::multiple) {
-            items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/Stark,_Tony.vcf"));
+    if (m_url.isEmpty()) {
+        if (transfer->contentType() == cuc::Type::Known::contacts().id()) {
+            items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/Joker.vcf"));
+            if (transfer->selectionType() == cuc::Transfer::SelectionType::multiple) {
+                items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/Stark,_Tony.vcf"));
+            }
+        } else {
+            items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/webbrowser-app.png"));
+            if (transfer->selectionType() == cuc::Transfer::SelectionType::multiple) {
+                items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/clock.png"));
+            }
         }
-
     } else {
-        items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/webbrowser-app.png"));
-
-        if (transfer->selectionType() == cuc::Transfer::SelectionType::multiple) {
-            items << cuc::Item(QUrl("file:///usr/share/content-hub/testability/data/clock.png"));
-        }
+        items << cuc::Item(QUrl(m_url));
     }
 
     transfer->charge(items);
@@ -83,9 +90,11 @@
 
     qDebug() << Q_FUNC_INFO << "STATE:" << transfer->state();
 
+    if (transfer->state() == cuc::Transfer::aborted)
+        QCoreApplication::instance()->exit(1);
 
     if (transfer->state() == cuc::Transfer::collected)
-        QCoreApplication::instance()->quit();
+        QCoreApplication::instance()->exit(0);
 }
 
 
 
=== modified file 'tests/peers/exporter/autoexporter.h'
--- tests/peers/exporter/autoexporter.h	2014-08-07 18:48:10 +0000
+++ tests/peers/exporter/autoexporter.h	2015-06-02 12:46:00 +0000
@@ -39,6 +39,10 @@
     Q_INVOKABLE void handle_export(cuc::Transfer*);
     Q_INVOKABLE void handle_share(cuc::Transfer*);
     Q_INVOKABLE void stateChanged();
+    void setUrl(QString);
+
+private:
+    QString m_url;
 };
 
 #endif // AUTOEXPORTER_H
 
=== modified file 'tests/peers/exporter/exporter.cpp'
--- tests/peers/exporter/exporter.cpp	2014-08-07 18:48:10 +0000
+++ tests/peers/exporter/exporter.cpp	2015-06-02 12:46:00 +0000
@@ -18,6 +18,8 @@
 
 #include <QCoreApplication>
 #include <QStringList>
+#include <QUrl>
+#include <sys/apparmor.h>
 
 #include "autoexporter.h"
 
@@ -30,12 +32,26 @@
         qputenv("APP_ID", "content-hub-test-exporter");
     }
 
-    AutoExporter exporter;
-
-    QString peerName;
+    QString peerName, url, profile;
 
     if (a.arguments().size() > 1)
         peerName = a.arguments().at(1);
+    if (a.arguments().size() > 2)
+        url = a.arguments().at(2);
+    if (a.arguments().size() > 3)
+        profile = a.arguments().at(3);
+
+    if (not profile.isEmpty()) {
+        int ret = 2;
+        ret = aa_change_profile(profile.toStdString().c_str());
+        if (ret != 0)
+            return 1;
+    }
+
+    AutoExporter exporter;
+
+    if (not url.isEmpty())
+        exporter.setUrl(url);
 
     if (!peerName.isEmpty())
     {