| Summary: | accountsservice: insufficient path check in user_change_icon_file_authorized_cb() | ||
|---|---|---|---|
| Product: | accountsservice | Reporter: | Matthias Gerstner <matthias.gerstner> |
| Component: | general | Assignee: | Matthias Clasen <mclasen> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | heirecka, marius.vollmer, rstrode, stefw |
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: | Suggested Patch | ||
Created attachment 140419 [details] [review] Suggested Patch Adding a patch that tries to address this issue by canonicalizing the input filename. 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.
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 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 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.