This is coming from the following commit: http://git.gnome.org/browse/gnome-keyring/commit/?id=bc2d1d3789368779e6a39bff6ca841188edef6c7

> # The daemon is installed as setuid so as to obtain specialized
> # capabilities, then immediately drops permissions. In other words,
> # it does *not* run as setuid.
> # If installing as non-root, chown+chmod will not succeed but
> # the build will continue.
> install-exec-hook:
>  chown root $(bindir)/gnome-keyring-daemon || true
>  chmod u+s $(bindir)/gnome-keyring-daemon || true

From the patch it looks like it is using ipc_lock, can we use file capabilities instead of setuid.  Not sure why they are locking the memory?  Prevent it from swapping?

Yes, this should be possible to use file system based capabilities. But about the patch, I don't think it does what they think it does. It drops all capabilities and then tries to change the uid. However, we dropped the setuid capability. So, does it really do that? Running as a user with a capability requires using prctl(). This patch would have been correct and only 3 lines long if they used libcap-ng rather than libcap.

I ran a test of the code in that patch. This is what it does before and after:

before:4325 0
Effective:    00000003, FFFFFFFF
Permitted:    00000003, FFFFFFFF
Inheritable:  00000000, 00000000
Bounding Set: 00000003, FFFFFFFF
after:4325 4325
Effective:    00000000, 00000000
Permitted:    00000000, 00000000
Inheritable:  00000000, 00000000
Bounding Set: 00000003, FFFFFFFF

It did not keep the IPC_LOCK capability, but it was able to change the effective uid away from 0. I then tested the following code based on the libcap-ng library:

 capng_clear(CAPNG_SELECT_CAPS);
 capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_IPC_LOCK);
 if (capng_change_id(getuid(), getgid(), 0))
   printf("failed");

Notice how much smaller the code is. :) The same test:

before:4325 0
Effective:    00000003, FFFFFFFF
Permitted:    00000003, FFFFFFFF
Inheritable:  00000000, 00000000
Bounding Set: 00000003, FFFFFFFF
after:4325 4325
Effective:    00000000, 00004000
Permitted:    00000000, 00004000
Inheritable:  00000000, 00000000
Bounding Set: 00000003, FFFFFFFF

Now it has the capability. So, I would say that the code doesn't work right. 

Changing this to file system based capabilities would do the right thing. The program would be running with just the IPC_LOCK capability for normal users. For root it will have all caps. So, you would want the following patch:

capng_get_caps_process();
if (capng_have_capabilities(CAPNG_SELECT_CAPS) == CAPNG_FULL) {
    capng_clear(CAPNG_SELECT_BOTH);
    capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_IPC_LOCK);
    if (getuid() != geteuid())
        if (capng_change_id(getuid(), getgid(), 0))  //setuid
            exit(1);
    else
        if (capng_apply(CAPNG_SELECT_CAPS))  // root user
            exit(1);
}

And then when it no longer needs capabilities:

 capng_clear(CAPNG_SELECT_CAPS);
 capng_apply(CAPNG_SELECT_CAPS);

Created attachment 473323 [details]
patch attempting to fix problem

The attached patch should allow either setuid or file system based capabilities to work correctly.

Created attachment 473333 [details]
improved patch addressing capabilities

This patch is a slight improvement in that we really do not need to check if euid != uid. We can just do it regardless.

Good work, Steve. Notified Stef and Yaron to have their word on this.

Packaged in gnome-keyring-2.91.4-3.fc15

Used "%attr(0755,root,root) %caps(cap_ipc_lock=ep) %{_bindir}/gnome-keyring-daemon" for gaining required capability. Only not sure about that "ep", judging from CAPNG_EFFECTIVE|CAPNG_PERMITTED from the code above. Please correct me if I'm wrong.

I talked to Stef and agreed to give our users some time to test this.

Yes, that would be the file system capabilities I would expect. Thanks!

Tomas, I'm getting ready to merge this upstream, could you write up something that I can include in the release notes for packagers on how to package this?

Do you know if packaging file capabilities works with .deb packages?

Pushed this to gnome-keyring master. It looks like however the daemon is still installed as setuid: daemon/Makefile.am is not changed. Is another patch coming for that?

(In reply to comment #10)
> Tomas, I'm getting ready to merge this upstream, could you write up something
> that I can include in the release notes for packagers on how to package this?
> 
Sure:

Since the 2.91.93 release gnome-keyring-daemon makes use of file capabilities (via libcap-ng), specifically CAP_IPC_LOCK. To actually be able to use the particular capability, the binary executable must be set with corresponding file caps. This can be done several ways:

 1. manually by setcap command

 2. in rpm spec file, just add "%caps(<caps>)" modifier before the packaged file.
    The <caps> argument is a list of file capabilities in a format described in 
    cap_from_text(3) man page.

    So the actual line would look like this:
    %attr(0755,root,root) %caps(cap_ipc_lock=ep) %{_bindir}/gnome-keyring-daemon

> Do you know if packaging file capabilities works with .deb packages?

Couldn't find anything relevant in Google, better to ask Debian/Ubuntu folks directly. An inspiration might be "iputils" package which in Fedora uses filecaps, but not in Debian apparently. FYI, file caps are stored in xattrs and not all filesystems support that. 

Found a whiteboard that might come handy: https://wiki.ubuntu.com/Security/FilesystemCapabilties

Created attachment 483745 [details]
patch removing suid setting

(In reply to comment #11)
> Pushed this to gnome-keyring master. It looks like however the daemon is still
> installed as setuid: daemon/Makefile.am is not changed. Is another patch coming
> for that?
Yeah, this is not needed anymore.

Thanks Tomas for the write up. I didn't use this patch, because it completely removes the setuid support. Instead I committed something that disables it conditionally based on whether we're using libcapng or not. Let me know if it looks wrong. Here's the commit:


commit b9d69a5751c421cca2bee9bab78c1067e1d1acac
Author: Stef Walter <stefw.uk>
Date:   Wed Mar 16 15:26:44 2011 +0100

    If we're using linux capabilities then use setcap instead of setuid.
    
    Only use setuid when not using linux capabilities. Run this on
    install when we are using caps:
    
    setcap cap_ipc_lock=ep $(DESTDIR)$(bindir)/gnome-keyring-daemon

(In reply to comment #14)
> Instead I committed something that disables it conditionally
> based on whether we're using libcapng or not.
Looks good, thanks.

I consider this bug as fixed, closing. Feel free to open separate bug reports if anything goes wrong.