Merge lp://staging/~jamesh/thumbnailer/use-aa-query-label into lp://staging/thumbnailer/devel

Proposed by James Henstridge
Status: Merged
Approved by: Michi Henning
Approved revision: 226
Merged at revision: 218
Proposed branch: lp://staging/~jamesh/thumbnailer/use-aa-query-label
Merge into: lp://staging/thumbnailer/devel
Diff against target: 1060 lines (+321/-200)
22 files modified
CMakeLists.txt (+1/-0)
include/internal/check_access.h (+42/-0)
include/internal/thumbnailer.h (+4/-1)
plugins/Ubuntu/Thumbnailer.0.1/CMakeLists.txt (+0/-1)
plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp (+1/-15)
src/CMakeLists.txt (+2/-0)
src/check_access.cpp (+83/-0)
src/service/CMakeLists.txt (+1/-3)
src/service/dbusinterface.cpp (+1/-2)
src/service/dbusinterface.h (+0/-1)
src/service/dbusinterface.xml (+1/-2)
src/service/handler.cpp (+1/-1)
src/thumbnailer-admin/get_local_thumbnail.cpp (+1/-13)
src/thumbnailer.cpp (+41/-26)
tests/CMakeLists.txt (+1/-0)
tests/check_access/CMakeLists.txt (+3/-0)
tests/check_access/check_access_test.cpp (+64/-0)
tests/dbus/dbus_test.cpp (+5/-42)
tests/qml/tst_albumart.qml (+4/-12)
tests/testsetup.h.in (+1/-1)
tests/thumbnailer-admin/thumbnailer-admin_test.cpp (+2/-2)
tests/thumbnailer/thumbnailer_test.cpp (+62/-78)
To merge this branch: bzr merge lp://staging/~jamesh/thumbnailer/use-aa-query-label
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Michi Henning (community) Approve
Review via email: mp+262060@code.staging.launchpad.net

Commit message

Remove the filename_fd argument to the GetThumbnail() D-Bus method, instead using aa_query_label() to check whether the caller has access to the relevant file.

Description of the change

Update the GetThumbnail dbus method to remove the filename_fd argument. Instead, use aa_query_label() to check whether the client has access to the target file.

We were already canonicalising the path, so shouldn't have trouble getting tricked by symlinks.

I'll provide some more concrete ways to test this on the phone once I've got ARM binaries.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:222
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/use-aa-query-label/+merge/262060/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/285/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/94/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/96/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/94/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:223
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/use-aa-query-label/+merge/262060/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/286/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-amd64-ci/95/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-armhf-ci/97/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-wily-i386-ci/95/console

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

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

Here is the steps I went to test this on the phone after installing it:

1. run "sudo aa-status" to find the list of profiles installed on the system. I looked for the label for the gallery app, which in this case was "com.ubuntu.gallery_gallery_2.9.1.1183"

2. Copy an image to ~/.local/share/com.ubuntu.music (or any other app-specific directory).

3. Change to ~/.local/share/com.ubuntu.gallery (a directory the profile from (1) could write to).

4. Run the following command (adjusting for exact label name):

   aa-exec -p com.ubuntu.gallery_gallery_2.9.1.1183 thumbnailer-admin get \
       ~/.local/share/com.ubuntu.music/image.jpg .

I then received the following error message:

thumbnailer-admin: checkFinished(): thumbnail: /home/phablet/.local/share/com.ubuntu.music/image20150602_014415108.jpg (0,0): unity::ResourceException: RequestBase::thumbnail(): key = /home/phablet/.local/share/com.ubuntu.music/image20150602_014415108.jpg\0145638\01434511100.933353751\01434511100.933353751:
    LocalThumbnailRequest::fetch(): AppArmor policy forbids access to /home/phablet/.local/share/com.ubuntu.music/image20150602_014415108.jpg

(plus a bunch of output complaining that it couldn't write to .gcda files, since this was a coverage build).

Revision history for this message
Michi Henning (michihenning) wrote :

Beautiful!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
No commit message was specified in the merge proposal. Hit 'Add commit message' on the merge proposal web page or follow the link below. You can approve the merge proposal yourself to rerun.
https://code.launchpad.net/~jamesh/thumbnailer/use-aa-query-label/+merge/262060/+edit-commit-message

review: Needs Fixing (continuous-integration)
225. By James Henstridge

Add some more debug logging.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
226. By James Henstridge

Remove the extra nul byte at the end of the buffer passed to
aa_query_label() -- this causes the check to fail for AppArmor labels
other than "unconfined".

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michi Henning (michihenning) wrote :

Excellent, thank you!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: