Merge lp:~jamesh/thumbnailer/aa-access-fix into lp:thumbnailer/devel

[James Henstridge]

Move the AppArmor check earlier so it is not skipped for files already in the cache.

With the previous AppArmor changes, I moved the GetThumbnail() security check to the LocalThumbnailRequest::fetch() method. Unfortunately this method is not called if there is a cached version of the thumbnail, giving clients access to data they shouldn't have.

This branch renames the ThumbnailRequest::set_client_credentials() method to check_client_credentials(), and performs the access checks from within this method. This code path is always hit as part of the request, so avoids the problem.

Unfortunately, I can't easily check the AppArmor aspects from within the test suite since (a) AppArmor isn't available within the Jenkins environment, and (b) even if it was, it isn't clear what profiles we can rely on being loaded into the kernel.

Once I'm done with manual testing, I'll try to formulate this as something people can repeat in the test plan. The thumbnailer-admin utility is really useful here.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:219
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~jamesh/thumbnailer/aa-access-fix/+merge/262295/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/291/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/100
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/102
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/102/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/100

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

review: Needs Fixing (continuous-integration)

[Michi Henning (michihenning)]

Looks good! One comment needs a fix (see inline).

review: Needs Fixing

[lp:~jamesh/thumbnailer/aa-access-fix]

220. By James Henstridge on 2015-06-18

Fix stale comment, as per Michi's review.

[Michi Henning (michihenning)]

Thank you!

review: Approve

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

=== modified file 'include/internal/thumbnailer.h'
--- include/internal/thumbnailer.h	2015-06-16 10:16:03 +0000
+++ include/internal/thumbnailer.h	2015-06-18 07:19:26 +0000
@@ -73,8 +73,9 @@
 
     virtual std::string const& key() const = 0;
 
-    // Set the credentials of the caller.
-    virtual void set_client_credentials(uid_t user, std::string const& apparmor_label) = 0;
+    // Check that the client has access to the thumbnail.  Throws an
+    // exception on authentication failure.
+    virtual void check_client_credentials(uid_t user, std::string const& apparmor_label) = 0;
 
 Q_SIGNALS:
     void downloadFinished();

=== modified file 'src/service/handler.cpp'
--- src/service/handler.cpp	2015-06-16 10:16:03 +0000
+++ src/service/handler.cpp	2015-06-18 07:19:26 +0000
@@ -201,7 +201,16 @@
         sendError("gotCredentials(): " + details() + ": could not retrieve peer credentials");
         return;
     }
-    p->request->set_client_credentials(credentials.user, credentials.label);
+    try
+    {
+        p->request->check_client_credentials(
+            credentials.user, credentials.label);
+    }
+    catch (std::exception const& e)
+    {
+        sendError("gotCredentials(): " + details() + ": " + e.what());
+        return;
+    }
 
     auto do_check = [this]() -> FdOrError
     {

=== modified file 'src/thumbnailer.cpp'
--- src/thumbnailer.cpp	2015-06-17 03:58:34 +0000
+++ src/thumbnailer.cpp	2015-06-18 07:19:26 +0000
@@ -77,10 +77,8 @@
     {
         return key_;
     }
-    void set_client_credentials(uid_t user, std::string const& label) override
+    void check_client_credentials(uid_t, std::string const&) override
     {
-        client_user_ = user;
-        client_apparmor_label_ = label;
     }
     enum class CachePolicy
     {
@@ -144,8 +142,6 @@
     string key_;
     QSize const requested_size_;
     chrono::milliseconds timeout_;
-    uid_t client_user_ = 0;
-    string client_apparmor_label_;
 
 private:
     FetchStatus status_;
@@ -162,6 +158,7 @@
                           string const& filename,
                           QSize const& requested_size,
                           chrono::milliseconds timeout);
+    void check_client_credentials(uid_t user, std::string const& label) override;
 
 protected:
     ImageData fetch(QSize const& size_hint) override;
@@ -415,6 +412,23 @@
     key_ += to_string(st.st_ctim.tv_sec) + "." + to_string(st.st_ctim.tv_nsec);
 }
 
+void LocalThumbnailRequest::check_client_credentials(uid_t user,
+                                                     std::string const& label)
+{
+    if (user != geteuid())
+    {
+        throw runtime_error("LocalThumbnailRequest::fetch(): Request comes from a different user ID");
+    }
+    if (!apparmor_can_read(label, filename_)) {
+        // LCOV_EXCL_START
+        qDebug() << "Apparmor label" << QString::fromStdString(label) << "has no access to" << QString::fromStdString(filename_);
+        throw runtime_error("LocalThumbnailRequest::fetch(): AppArmor policy forbids access to " + filename_);
+        // LCOV_EXCL_STOP
+    }
+
+}
+
+
 RequestBase::ImageData LocalThumbnailRequest::fetch(QSize const& size_hint)
 {
     if (image_extractor_)
@@ -423,19 +437,6 @@
         return ImageData(Image(image_extractor_->data()), CachePolicy::cache_fullsize, Location::local);
     }
 
-    if (client_user_ != geteuid())
-    {
-        // LCOV_EXCL_START
-        throw runtime_error("LocalThumbnailRequest::fetch(): Request comes from a different user ID");
-        // LCOV_EXCL_STOP
-    }
-    if (!apparmor_can_read(client_apparmor_label_, filename_)) {
-        // LCOV_EXCL_START
-        qDebug() << "Apparmor label" << QString::fromStdString(client_apparmor_label_) << "has no access to" << QString::fromStdString(filename_);
-        throw runtime_error("LocalThumbnailRequest::fetch(): AppArmor policy forbids access to " + filename_);
-        // LCOV_EXCL_STOP
-    }
-
     // Work out content type.
     gobj_ptr<GFile> file(g_file_new_for_path(filename_.c_str()));
     assert(file);  // Cannot fail according to doc.

=== modified file 'tests/thumbnailer/thumbnailer_test.cpp'
--- tests/thumbnailer/thumbnailer_test.cpp	2015-06-16 09:37:33 +0000
+++ tests/thumbnailer/thumbnailer_test.cpp	2015-06-18 07:19:26 +0000
@@ -93,18 +93,15 @@
     Image img;
 
     request = tn.get_thumbnail(EMPTY_IMAGE, QSize());
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     EXPECT_EQ("", thumb);
 
     // Again, this time we get the answer from the failure cache.
     request = tn.get_thumbnail(EMPTY_IMAGE, QSize());
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     EXPECT_EQ("", thumb);
 
     request = tn.get_thumbnail(TEST_IMAGE, QSize());
-    request->set_client_credentials(geteuid(), "unconfined");
     EXPECT_TRUE(boost::starts_with(request->key(), TEST_IMAGE)) << request->key();
     thumb = request->thumbnail();
     img = Image(thumb);
@@ -113,28 +110,24 @@
 
     // Again, for coverage. This time the thumbnail comes from the cache.
     request = tn.get_thumbnail(TEST_IMAGE, QSize());
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     img = Image(thumb);
     EXPECT_EQ(640, img.width());
     EXPECT_EQ(480, img.height());
 
     request = tn.get_thumbnail(TEST_IMAGE, QSize(160, 160));
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     img = Image(thumb);
     EXPECT_EQ(160, img.width());
     EXPECT_EQ(120, img.height());
 
     request = tn.get_thumbnail(TEST_IMAGE, QSize(1000, 1000));  // Will not up-scale
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     img = Image(thumb);
     EXPECT_EQ(640, img.width());
     EXPECT_EQ(480, img.height());
 
     request = tn.get_thumbnail(TEST_IMAGE, QSize(100, 100));  // From EXIF data
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     img = Image(thumb);
     EXPECT_EQ(100, img.width());
@@ -143,7 +136,6 @@
     try
     {
         request = tn.get_thumbnail(BAD_IMAGE, QSize());
-        request->set_client_credentials(geteuid(), "unconfined");
         request->thumbnail();
         FAIL();
     }
@@ -154,21 +146,18 @@
     }
 
     request = tn.get_thumbnail(RGB_IMAGE, QSize(48, 48));
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     img = Image(thumb);
     EXPECT_EQ(48, img.width());
     EXPECT_EQ(48, img.height());
 
     request = tn.get_thumbnail(BIG_IMAGE, QSize());  // > 1920, so will be trimmed down
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     img = Image(thumb);
     EXPECT_EQ(1920, img.width());
     EXPECT_EQ(1439, img.height());
 
     request = tn.get_thumbnail(BIG_IMAGE, QSize(0, 0));  // unconstrained, so will not be trimmed down
-    request->set_client_credentials(geteuid(), "unconfined");
     thumb = request->thumbnail();
     img = Image(thumb);
     EXPECT_EQ(2731, img.width());
@@ -200,7 +189,6 @@
             // Load a song so we have something in the full-size and thumbnail caches.
             auto request = tn.get_thumbnail(TEST_SONG, QSize());
             ASSERT_NE(nullptr, request.get());
-            request->set_client_credentials(geteuid(), "unconfined");
             // Audio thumbnails cannot be produced immediately
             ASSERT_EQ("", request->thumbnail());
 
@@ -218,7 +206,6 @@
             // Load same song again at different size, so we get a hit on full-size cache.
             auto request = tn.get_thumbnail(TEST_SONG, QSize(20, 20));
             ASSERT_NE(nullptr, request.get());
-            request->set_client_credentials(geteuid(), "unconfined");
             ASSERT_NE("", request->thumbnail());
         }
 
@@ -226,21 +213,18 @@
             // Load same song again at same size, so we get a hit on thumbnail cache.
             auto request = tn.get_thumbnail(TEST_SONG, QSize(20, 20));
             ASSERT_NE(nullptr, request.get());
-            request->set_client_credentials(geteuid(), "unconfined");
             ASSERT_NE("", request->thumbnail());
         }
 
         {
             // Load an empty image, so we have something in the failure cache.
             auto request = tn.get_thumbnail(EMPTY_IMAGE, QSize());
-            request->set_client_credentials(geteuid(), "unconfined");
             EXPECT_EQ("", request->thumbnail());
         }
 
         {
             // Load empty image again, so we get a hit on failure cache.
             auto request = tn.get_thumbnail(EMPTY_IMAGE, QSize());
-            request->set_client_credentials(geteuid(), "unconfined");
             EXPECT_EQ("", request->thumbnail());
         }
     };
@@ -340,7 +324,6 @@
     ASSERT_EQ(0, unlink(testfile.c_str()));
     ASSERT_EQ(0, link(BIG_IMAGE, testfile.c_str()));
 
-    request->set_client_credentials(geteuid(), "unconfined");
     string data = request->thumbnail();
     Image img(data);
     EXPECT_EQ(640, img.width());
@@ -352,7 +335,6 @@
     Thumbnailer tn;
     auto request = tn.get_thumbnail(TEST_VIDEO, QSize());
     ASSERT_NE(nullptr, request.get());
-    request->set_client_credentials(geteuid(), "unconfined");
     // Video thumbnails cannot be produced immediately
     ASSERT_EQ("", request->thumbnail());
 
@@ -371,7 +353,6 @@
         // Fetch the thumbnail again with the same size.
         // That causes it to come from the thumbnail cache.
         auto request = tn.get_thumbnail(TEST_VIDEO, QSize());
-        request->set_client_credentials(geteuid(), "unconfined");
         string thumb = request->thumbnail();
         ASSERT_NE("", thumb);
         Image img(thumb);
@@ -383,7 +364,6 @@
         // Fetch the thumbnail again with a different size.
         // That causes it to be scaled from the thumbnail cache.
         auto request = tn.get_thumbnail(TEST_VIDEO, QSize(500, 500));
-        request->set_client_credentials(geteuid(), "unconfined");
         string thumb = request->thumbnail();
         ASSERT_NE("", thumb);
         Image img(thumb);
@@ -399,7 +379,6 @@
 
     Thumbnailer tn;
     auto request = tn.get_thumbnail(testfile, QSize());
-    request->set_client_credentials(geteuid(), "unconfined");
     ASSERT_EQ("", request->thumbnail());
 
     // Replace test image with a different file with different
@@ -422,7 +401,6 @@
     Thumbnailer tn;
     auto request = tn.get_thumbnail(TEST_SONG, QSize());
     ASSERT_NE(nullptr, request.get());
-    request->set_client_credentials(geteuid(), "unconfined");
     // Audio thumbnails cannot be produced immediately
     ASSERT_EQ("", request->thumbnail());
 
@@ -467,7 +445,6 @@
         setenv("TN_UTILDIR", "no_such_directory", true);
 
         auto request = tn.get_thumbnail(TEST_SONG, QSize());
-        request->set_client_credentials(geteuid(), "unconfined");
         EXPECT_EQ("", request->thumbnail());
 
         QSignalSpy spy(request.get(), &ThumbnailRequest::downloadFinished);
@@ -489,6 +466,24 @@
     }
 }
 
+TEST_F(ThumbnailerTest, check_client_access)
+{
+    Thumbnailer tn;
+    auto request = tn.get_thumbnail(TEST_IMAGE, QSize());
+    ASSERT_NE(nullptr, request.get());
+    // Check succeeds for correct user ID and valid label
+    request->check_client_credentials(geteuid(), "unconfined");
+    try
+    {
+        request->check_client_credentials(geteuid() + 1, "unconfined");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_TRUE(boost::contains(e.what(), "Request comes from a different user ID")) << e.what();
+    }
+}
+
 class RemoteServer : public ThumbnailerTest
 {
 protected: