Bug 107085 - accountsservice: insufficient path check in user_change_icon_file_authorized_cb()
Summary: accountsservice: insufficient path check in user_change_icon_file_authorized_...
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Matthias Clasen
QA Contact:
Depends on:
Reported: 2018-07-02 10:45 UTC by Matthias Gerstner
Modified: 2018-07-10 14:04 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:

Suggested Patch (2.36 KB, patch)
2018-07-02 10:46 UTC, Matthias Gerstner
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Gerstner 2018-07-02 10:45:05 UTC
This is a public report of an issue previously reported privately to the
accountsservice developers via email.

I have found a weakness regarding the handling of the users' icon files.
Regular users are by default allowed to change their own data as per
polkit rule for action org.freedesktop.accounts.change-own-user-data.

In function user_change_icon_file_authorized_cb() in src/user.c there is
quite some effort for safely setting the icon file property. The logic
wants to achieve the following:

a) take over the provided path as is, if it points to a world-readable
  file in /usr/share
b) otherwise safely copy the file with user privileges into
  /var/lib/AccountsService/icons, and use that path as the property

The following if clause tries to determine whether a) is the case:

        if ((mode & S_IROTH) == 0 ||
            (!g_str_has_prefix (filename, DATADIR) &&
             !g_str_has_prefix (filename, ICONDIR))) {

However, the prefix check is insufficient. Passing ../ components in the
user supplied path can circumvent the check like this:

$ touch /tmp/test
$ dbus-send --system --print-reply --dest=org.freedesktop.Accounts /org/freedesktop/Accounts/User1000 org.freedesktop.Accounts.User.SetIconFile string:/usr/  share/../../tmp/test
$ rm /tmp/test
$ ln -s /root/.bash_history /tmp/test

Now the accountsservice stores /usr/share/../../tmp/test as icon file
path, which actually points to /root/.bash_history. A third party
application that trusts this property can potentially read from this
location as root and tries to interpret it as an image file. This is for
example the case for Cinnamon desktop in the cinnamon-settings-users GUI
application. Luckily it does not simply copy the file, but tries to read
it into an image object first. There may be other clients of
accountsservice where this leads to more severe consequences.

I think the easiest way to fix this is to normalize the user supplied
filename e.g. using realpath(), before making the test above.
Comment 1 Matthias Gerstner 2018-07-02 10:46:57 UTC
Created attachment 140419 [details] [review]
Suggested Patch

Adding a patch that tries to address this issue by canonicalizing the input filename.
Comment 2 Ray Strode [halfline] 2018-07-10 14:04:58 UTC
Pushed with some small changes:

To ssh://git.freedesktop.org/git/accountsservice
   34bedec..f9abd35  master -> master

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.