Created attachment 508204 [details]
patch

This fixes the issues I could think of.

I'll wait for Sebastian's feedback before doing anything else; I don't think a few more days will harm.

bugbot adjusting priority

Ok, will have a look.

Since you seem to require to auth yourself as admin to do that,
its not so severe issue, IMHO.

Quick look at the patch (yet need to re-check some
of the other cups-pk functions; I remember to have a review
dont some years ago, but we usually only focus on functions
that can be called by any user):

Theres still a race condition between the lstat() and the
open(O_CREAT) in which the regular file inside /home/tux/foo could
be replaced by a symlink to /etc/passwd. The fchmod() afterwards
wont save from that (worse, it chowns the file to the user:)

There is also something weird in the logic if the lstat()
must not fail due to a missing file, but the file is opened
via O_CREAT. The comment there sounds like it has already been
in mind that the file could disappear after the stat.

So, we are again in the problem of securely creating files
in the user-dir as root, which is _really hard to get right_.
One could argue to use O_NOFOLLOW, but then the targetfile could
also be a hard-link to /etc/passwd.

The proper way to handle such situations is to switch to euid (fsuid) and
egid of the user, so all the file operations are automagically done right.
Unfortunally (we did that with ecryptfs) its also a horror to get
that right with the supplementary group id's and switching it back
after the operations.
 
Then, the following cupsGetFile() is passed the absolute
pathname again so its maybe again doing insecure operations inside
a user owned dir. I need to check that.

So my first thought is that we probably cant get around switching
the user/group id's.

I also wonder whats with the cph_cups_file_put(), would it be
possible to push /etc/shadow to some printer or fake cups server?

For the cupsGetFile() problem, it can maybe be solved using cupsGetFd().
The cupsGetFile() seems to use O_EXCL internally anyway, so I wonder
it could work at all, as the file has just been created.

But again, if the polkit rules require auth_admin, this is not
really a big problem.

Created attachment 508581 [details]
Reworked patch

Change effective uid/gid and then work on fd.

Looks legit. Yes, the problem was that you cannot get around the race
if not switching euid.

But we need also need to return FALSE in case of seteuid() failure.
Yes, seteuid() can really fail. Otherwise code would continue to
run as root.

One other issue is the supplementary groups. I dont know how
cups-pk is started, so this might not be necessary, as daemons
usually have no supplementary gids. But if they have, they need
to be cleared/resetted too.

Reminds me to write generic functions for all these, as with DBUS
etc it seems to happen quite often that one needs to
temp drop privileges.

No problem.

I hope you have seen my latest mail, we really need to add
this setgroups(0, NULL) to the cups-pk uid-switching ourself to
drop any supplementary groups.
The reset can be done using initgroups("root", 0)
(if we assume that the helper will always be activated to run as
root)

(In reply to comment #18)
> I hope you have seen my latest mail, we really need to add
> this setgroups(0, NULL) to the cups-pk uid-switching ourself to
> drop any supplementary groups.
> The reset can be done using initgroups("root", 0)
> (if we assume that the helper will always be activated to run as
> root)


Yes, I was looking at that. However, Alexander was mentioning in his mail that "setgroups(0, NULL)" might not be working on all BSDs, though.

I was actually thinking that if we can drop all supplementary groups at startup, we'd be happy enough and wouldn't need to play the set/reset game. But on the other hand, we might want to call initgroups() for the remote user, so we can really read/write all files he has access too... Ah well, I'll come with something.

Created attachment 508874 [details]
Additional patch

Attaching the additional patch, for the record.

Exactly. the sizeof() is larger than the NGROUPS_MAX, so
it would result in getgroups() potentially copying more
gid's into the array than would fit. Welcome to security world. :)

Problem also is that NGROUPS_MAX is not really known at
compile time (speaking of hard-coding).

If you dont like initgroups("root"), would it be OK to
call the following in reset/error case:

pw = getpwuid(getuid()); // UID was unchanged in switch-user
initgroups(pw->pw_name, pw->pw_gid);

+ error checking :)

That avoids any hard coding and also any allocs+deallocs.

Created attachment 508895 [details]
Additional patch, 2nd try

So I went with the allocation instead, which I feel is the proper way.

Sebastian: sorry, saw your comment about using initgroups() for the reset too late. Hrm, if you feel it's much better, I'll do your way.

Created attachment 508909 [details]
Additional patch

(slightly cleaner version)

Looks legal for me, lets see what Alexander says. :)

What would

groups = g_new (gid_t, ngroups);

return if ngroups is 0 (e.g. no supplementary groups)?
If it returns NULL, you will still have a setgroups(0, NULL) again
later in the error case or on reset.

(In reply to comment #28)
> What would
> 
> groups = g_new (gid_t, ngroups);
> 
> return if ngroups is 0 (e.g. no supplementary groups)?
> If it returns NULL, you will still have a setgroups(0, NULL) again
> later in the error case or on reset.

Yes, it returns NULL. We would indeed have setgroups(0, NULL) in that case; however, my understanding is that platforms that don't support such a call (BSDs) actually won't return ngroups=0 from getgroups(): they will also put the effective gid as part of the list.

getgroups() has a slightly different meaning on BSD, since it's not about the supplementary list of groups, like in glibc, but about the whole list of groups; at least, as far as I understand ;-)

Ok, makes sense.

Got CVE-2012-4510 meanwhile.

(just my 2 cents.... you should never need this kind of privilege dropping, it shows something is flawed in the design and the original rights are too high. :( 
)

We will need to review design of this at some point.

Update submitted to G:F: https://build.opensuse.org/request/show/137976

system-config-printer is on

sle11-sp1-x86_64,sle11-sp2-i586,sle11-sp2-ia64,sle11-sp2-ppc64,sle11-sp2-s390x,sle11-sp2-x86_64

(In reply to comment #36)
> system-config-printer is on
> 
> sle11-sp1-x86_64,sle11-sp2-i586,sle11-sp2-ia64,sle11-sp2-ppc64,sle11-sp2-s390x,sle11-sp2-x86_64

Ah, indeed, the tarball is part of the system-config-printer source package in SLE11. So this has the issue too.

Excellent, thanks, Scott!

I've put in the _GNU_SOURCE fix and submitted the package.  It has request id 22483.

it does not seem to have build.

https://build.suse.de/package/show?package=system-config-printer&project=home%3Afederico-mena%3Abranches%3ASUSE%3ASLE-11-SP1%3AUpdate%3ATest

is all failed.

Dang, I submitted the wrong patch.  Fixing it...

reopen for tracking (to reassign to security-team@suse.de)

I put it to the planned update list; no need to start an update now
(see c#20)

Released.