Bug 832532 (CVE-2012-2737) - CVE-2012-2737 accountsservice: local file disclosure flaw
Summary: CVE-2012-2737 accountsservice: local file disclosure flaw
Alias: CVE-2012-2737
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
Depends On: 836284
Blocks: 832538 832902
TreeView+ depends on / blocked
Reported: 2012-06-15 16:38 UTC by Vincent Danen
Modified: 2019-09-29 12:53 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2012-08-14 11:30:46 UTC

Attachments (Terms of Use)
util: CVE-2012-2737: Validate SetIconFile caller over bus (3.41 KB, patch)
2012-06-19 16:21 UTC, Ray Strode [halfline]
no flags Details | Diff
Drop all uses of polkit_unix_process_get_uid (17.04 KB, patch)
2012-06-19 19:05 UTC, Ray Strode [halfline]
no flags Details | Diff

Description Vincent Danen 2012-06-15 16:38:20 UTC
Florian Weimer found a local file disclosure flaw in accountsservice, an account management system using D-Bus for querying and manipulating user accounts.  The implementation of the SetIconFile method of the org.freedesktop.Accounts.User D-Bus interface can disclose arbitrary files due to a race condition in user_change_icon_file_authorized_cb() in /usr/libexec/accounts-daemon.  When this function calls get_caller_uid(), it uses PolicyKit to obtain the UID of the requesting process from /proc.  At the time the UID is fetched, it may not match the original UID making the D-Bus request if the process has executed an SUID binary.

Comment 3 Ray Strode [halfline] 2012-06-19 15:35:08 UTC
So this code got added here:


Be more careful when copying the icon file
Don't read the file in the root process, instead fork, become the calling user, then read the file and pipe it back to the parent process. This protects against callers passing e.g. "/etc/shadow" as filename. 

Matthias and I talked through this, this morning.

I think ideally we'd have the client open the file and pass the fd to the accounts daemon, but failing that, we need to check the peer credentials collected from the dbus-daemon at the time the client authorized with the daemon.  DBus provides that information with the GetConnectionUnixUser method.

Is there any sort of embargo being organized?

Comment 4 Ray Strode [halfline] 2012-06-19 16:21:20 UTC
Created attachment 593003 [details]
util: CVE-2012-2737: Validate SetIconFile caller over bus

The AccountsService SetIconFile call associates an icon
with a user.

This method allows users to have icons at the login screen,
that don't necessarily originate in globally readable/always
available locations.  This is accomplished by copying the
originating icon to the local disk in /var.

Since AccountsService runs with escalated privileges, the
implemention of the SetIconFile method queres the callers
uid, forks(), assumes that uid and performs the copy as if
it were the user.

Unfortunately, the UID look up peformed is done "just in time"
instead of looking at peer credentials from the time the call
was initiated. This is a race condition that means a caller
could invoke the method call, quickly exec a setuid binary, and
then cause the copy to be performed as the uid of the setuid

This commit changes the uid look up logic, to query the system
bus daemon for peer credentials it cached from the caller at the
time of the call.

Comment 5 Ray Strode [halfline] 2012-06-19 19:05:24 UTC
Created attachment 593044 [details]
Drop all uses of polkit_unix_process_get_uid

I discussed attachment 593003 [details] with davidz and mitr on IRC.  They both agree it's a viable approach.

mitr did some additional auditing and discovered a few other callers of polkit_unix_process_get_uid(), so this patchset takes care of them as well.

Comment 7 Matthias Clasen 2012-06-19 21:02:51 UTC
The patches look fine to me

Comment 8 Vincent Danen 2012-06-19 21:06:52 UTC
Florian, can you review the patch as well?

For the embargo, there is none set at the moment.  As this is just in Fedora (do any other distributions use this?), once we get the patch vetted we can send it along to linux-distros with a short embargo, or skip the embargo altogether and notify oss-security instead.

Comment 9 Florian Weimer 2012-06-20 08:10:53 UTC
(In reply to comment #8)
> Florian, can you review the patch as well?

The patch looks reasonable to me.  Dbus uses credential passing to obtain the UID, so this should be fine (but I haven't looked at Dbus in detail).

> For the embargo, there is none set at the moment.  As this is just in Fedora
> (do any other distributions use this?)

Debian packages acountsservice, but not in the stable distribution.  Canonical might have released this version already.

Comment 10 Ray Strode [halfline] 2012-06-20 16:41:28 UTC
accountsservice is a requirement of GNOME and is probably used by most recent distributions that ship GNOME.

Comment 11 Vincent Danen 2012-06-20 21:29:32 UTC
Oh, ok.  So if I send the details and the patch to linux-distros today, would an unembargo of Monday be acceptable?

Comment 12 Ray Strode [halfline] 2012-06-22 01:49:40 UTC

Comment 13 Vincent Danen 2012-06-28 14:52:54 UTC
CRD has passed, so making this bug public now.

Comment 14 Vincent Danen 2012-06-28 14:53:38 UTC
Created accountsservice tracking bugs for this issue

Affects: fedora-all [bug 836284]

Comment 16 Vincent Danen 2012-08-16 15:35:24 UTC
This is corrected via accountsservice-0.6.21-2.fc17 in Fedora 17.

Note You need to log in before you can comment on or make changes to this bug.