oss-sec mailing list archives
accountsservice: insufficient path check in user_change_icon_file_authorized_cb()
From: Matthias Gerstner <mgerstner () suse de>
Date: Mon, 2 Jul 2018 14:21:22 +0200
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 () suse 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)
Attachment:
0001-user_change_icon_file_authorized_cb-fix-insufficient.patch
Description:
Attachment:
signature.asc
Description:
Current thread:
- accountsservice: insufficient path check in user_change_icon_file_authorized_cb() Matthias Gerstner (Jul 02)
- Re: accountsservice: insufficient path check in user_change_icon_file_authorized_cb() Jakub Wilk (Jul 02)
- Re: accountsservice: insufficient path check in user_change_icon_file_authorized_cb() Matthias Gerstner (Jul 02)
- Re: accountsservice: insufficient path check in user_change_icon_file_authorized_cb() Simon McVittie (Jul 02)
- Re: accountsservice: insufficient path check in user_change_icon_file_authorized_cb() Matthias Gerstner (Jul 03)
- Re: accountsservice: insufficient path check in user_change_icon_file_authorized_cb() Jakub Wilk (Jul 02)