Ubuntu

[MIR] accountsservice

Reported by Rodrigo Moya on 2011-05-20
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
accountsservice (Ubuntu)
Undecided
Rodrigo Moya

Bug Description

Binary package hint: accountsservice

1. Availability - already packaged & builds in Ubuntu universe since Maverick & Debian experimental

2. Rationale - The new gnome-control-center needs accountsservice so that the user accounts panel works, so this would need to be in Main. The latest upload to oneiric for g-c-c has this as a Recommends:, but once accountsservice is in the main archive, it will be a dependency, as the Debian package.

3. Security - Since this is an official external dependency of GNOME, and actively maintained by the same people working on the user accounts panel in gnome-control-center, we can expect security updates to go through the usual channels. There are no security issues right now -> http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=accountsservice

4. QA - There are no open bugs for this package as of today -> https://bugs.launchpad.net/ubuntu/+source/accountsservice

5. UI - accountsservice is a service, so there is no UI

6. Dependencies - libc6, libdbus-1-3, libdbus-glib-1-2, libglib2.0-0, libpolkit-gobject-1-0, libaccountsservice0, dbus, all of which are in main already

7. Standards-compliant 3.9.2

8. Maintenance - We are currently in sync with Debian

9. Background information - Development of accountsservice started in 2010

description: updated
Michael Terry (mterry) on 2011-05-20
Changed in accountsservice (Ubuntu):
assignee: nobody → Didier Roche (didrocks)
Didier Roche (didrocks) wrote :

Packaging and code looks good to me.

Not that the testsuite is disabled at built time as all test are using dbus.

However, this package enables (and uses from g-c-c) sending the password hint over dbus. I'm not sure what the security concerns on that part can be (as 3rd part application can sniff the password hint?). Isn't that kind of issue which made /etc/passwd password to be in a root-owned file /etc/shadow?

Subscribing the security team to have some input on that.

If the package is approved, then, feel free to assign me back to do the promotion

Changed in accountsservice (Ubuntu):
assignee: Didier Roche (didrocks) → Ubuntu Security Team (ubuntu-security)
Kees Cook (kees) wrote :

Seems like it will fail to handle usernames with "-", ".", or "_" in the name? Should compare this exclusion list against what "adduser" does. src/daemon.c:
        re = g_regex_new ("(?P<username>[0-9a-zA-Z]+)[ ]+(?P<frequency>[0-9]+)", 0, 0, &error);

daemon_create_user_authorized_cb() lacks a "--" in the argument builder, so accounts with a leading dash will be taken as an argument:
        argv[4] = cd->user_name;
Additionally, nothing validates the contents of user_name and real_name ("useradd" should, but best to do _some_ sanity checking).

daemon_delete_user_authorized_cb() lacks "--" in argument builder too.
In fact, all the spawned cb()'s lack the "--" separator (see user.c too).

I see no way for the daemon to shut down during a package upgrade (and the associated postinst to perform that). This should be an MIR requirement for all D-Bus daemon, IMO.

And I would agree: it's not okay for the password hint to be sent over the system bus.

I can't see how user_set_icon_file() is actually called, but contains a few problems:
 - source file is examined with root privileges (size, mode)
 - ToCToU on file size (checks first, then performs copy; file size can change between the two)
 - missing "--" in the spawn again

user_change_password_authorized_cb expects the crypted password to be sent over D-Bus. This is not okay since even the encrypted password is considered private (hence /etc/shadow), though this is what system-tools-backends did before, IIRC (also, holy cow, did s-t-b revert to sending passwords in the clear over D-Bus??). (Not sure what calls user_set_password though.) What is generating the crypted password? Is it respecting the settings in /etc/login.defs ?

Anyway, that's enough for now. This stuff should get fixed before this goes into main.

Changed in accountsservice (Ubuntu):
assignee: Ubuntu Security Team (ubuntu-security) → Didier Roche (didrocks)
status: New → Incomplete
Didier Roche (didrocks) wrote :

@Rodrigo: can you fix that or talk to upstream to answer Kees' concern?

Changed in accountsservice (Ubuntu):
assignee: Didier Roche (didrocks) → Rodrigo Moya (rodrigo-moya)
Changed in accountsservice (Ubuntu):
status: Incomplete → In Progress
status: In Progress → Incomplete
Rodrigo Moya (rodrigo-moya) wrote :

> daemon_create_user_authorized_cb() lacks a "--" in the argument builder, so accounts with a leading dash will be taken as an argument:

looking at adduser's source code, I see this comment:

(gtx("%s: To avoid problems, the username should consist only of
letters, digits, underscores, periods, at signs and dashes, and not start with
a dash (as defined by IEEE Std 1003.1-2001).

So I guess we don't want to fix that. It works perfectly adding users with '-' and '_' in the name.

> Additionally, nothing validates the contents of user_name and real_name ("useradd" should, but best to do _some_ sanity checking).

the user accounts panel in gnome-control-center does check the user_name, and also accountsservice itself uses the regexp to check the validity of the user_name

> I see no way for the daemon to shut down during a package upgrade (and the associated postinst to perform that).

ok, added this to the attached package branch

Rodrigo Moya (rodrigo-moya) wrote :

> I can't see how user_set_icon_file() is actually called, but contains a few problems:

it is called from the client, via the SetIconFile DBus method

As for the crypted password, it is crypted with apg, on the client, which calls SetPassword DBus method, which maps to call user_set_password on the daemon.

Rodrigo Moya (rodrigo-moya) wrote :

About sending the password hint over DBus, this is what upstream says:

1. its the system bus, so you can (at least in theory) lock it down somewhat, with selinux policy and whatnot
2. there's no good alternative anyway

About the file size check: the file size check is just a trivial sanity check; I don't think it is a big concern that there's a race there

Rodrigo Moya (rodrigo-moya) wrote :

Also, about the file size check: when we actually read the file, we do the extra work to become the user (that's the /usr/bin/cat thing)

Rodrigo Moya (rodrigo-moya) wrote :

For the '--' issue in argument builders: https://bugs.freedesktop.org/show_bug.cgi?id=38365

Changed in accountsservice (Ubuntu):
assignee: Rodrigo Moya (rodrigo-moya) → Kees Cook (kees)
status: Incomplete → New
Kees Cook (kees) wrote :

Instead of respawning the account daemon from the postinst, I think it would be better to just let D-Bus relaunch it on demand to keep its environment standard (rather than getting the environment of whatever was running dpkg).

The "--" fixes look good, thanks.

I think the username string comment was misunderstood. I was saying that since 'adduser' accepts "-", ".", and "_", then so should accountsservice, but it seems that the username filter regular expression doesn't allow those characters (and should be fixed).

The file size check as the root user is a problem because it is an information leak (it can be used to test for the existence of files, etc). If the size check is going to be used at all, it should be done during the copy (as the real user), to avoid information leaks or ToCToU races.

I'd still like to see some kind of solution for passing the password in the clear over D-Bus. "apg" is just used to generate a password, IIUC, not to do the hashing.

Changed in accountsservice (Ubuntu):
assignee: Kees Cook (kees) → Rodrigo Moya (rodrigo-moya)
Kees Cook (kees) wrote :

Okay, after proving to myself that the system bus can't normally be snooped, I'm satisfied not to block the MIR, but the file size limit test (moving it into the GIO stream copy) should be fixed before release.

+1

Kees Cook (kees) wrote :

Oh, one more thing -- the crypted password system must use the system crypt functions, not use an embedded method since the system may be configured for different systems:

static gchar *
make_crypted (const gchar *plain)
{
...
        /* SHA 256 */
        g_string_append (salt, "$6$");

This whole routine should be replaced with a call to "chpasswd -S" though that option appears to busted in natty. Regardless, the /etc/login.defs "ENCRYPT_METHOD" needs to be used, and preferably with tools from the "shadow" package.

Kees Cook (kees) wrote :

Oh, and "$6" is SHA512, not SHA256 -- that comment is wrong. :)

Martin Pitt (pitti) wrote :

Promoted.

Changed in accountsservice (Ubuntu):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.