oss-security - accountsservice: insufficient path check in user_change_icon_file_authorized

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date: Mon, 2 Jul 2018 14:21:22 +0200
From: Matthias Gerstner <mgerstner@...e.de>
To: oss-security@...ts.openwall.com
Subject: accountsservice: insufficient path check in
 user_change_icon_file_authorized_cb()

Hello,

during a code review the following issue was uncovered in
accountsservice <https://www.freedesktop.org/wiki/Software/AccountsService/>:

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
  value

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 try to interpret it as an image file. This is for
example the case for Cinnamon desktop in the cinnamon-settings-users GUI
application. Luckily in this example 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.

Suggested Fix:

I think the easiest way to fix this is to normalize the user supplied
filename e.g. using realpath(), before making the test above. A
preliminary patch that takes this approach is found in the upstream bug
referenced below and also attached to this mail.

References:

OpenSUSE bug: https://bugzilla.suse.com/show_bug.cgi?id=1099699
Upstream bug: https://bugs.freedesktop.org/show_bug.cgi?id=107085

Timeline:

- 2018-06-28: I found the issue during a code review
- 2018-06-28: I privately disclosed the issue to the upstream developers
- 2018-07-02: The upstream developers agreed to publish the details

-- 
Matthias Gerstner <matthias.gerstner@...e.de>
Dipl.-Wirtsch.-Inf. (FH), Security Engineer
https://www.suse.com/security
Telefon: +49 911 740 53 290
GPG Key ID: 0x14C405C971923553

SUSE Linux GmbH
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nuernberg)

View attachment "0001-user_change_icon_file_authorized_cb-fix-insufficient.patch" of type "text/x-diff" (2413 bytes)

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.