Bug 864716 (CVE-2014-5033)

Just noticed that KAuth is using 

PolkitQt1::UnixProcessSubject subject(pid);

which is polkit's deprecated way of checking authorizations: by pid.

UnixProcess subjects shall not be used. I am not familar on how
KAuth is used internally by KDE, but this doesnt look good.

Sent mail to some KDE developers and author of KAuth last week.
No presonse so far; escalating.

sent report to security@kde.org

sent to OSS-sec

Sebastian, as a comment from my side. You indicate that KAuth is using a deprecated method from polkit. However if I look at building the bindings, I can only find the following deprecated method: 

[  104s] /home/abuild/rpmbuild/BUILD/polkit-qt-1-0.103.50/core/polkitqt1-subject.cpp:125:16: warning: 'PolkitSubject* polkit_unix_process_new(gint)' is deprecated (declared at /usr/include/polkit-1/polkit/polkitunixprocess.h:51): Use polkit_unix_process_new_for_owner instead [-Wdeprecated-declarations]
[  104s]      setSubject(polkit_unix_process_new(pid));

Is this the one you are referring ?  Otherwise it seems that Polkit didn't declare that particular way of checking authorizations as deprecated and would then be still a valid way. 

I don't think that KAuth is the issue here, but just the Qt-bindings around PolKit, which haven't been updated for the last two years. However checking a very recent PolKitQt-1 build against the polkit in factory, then the above is the only warning we receive about deprecated.

Sebastian, Can you confirm that the above issue is based on the following bugreport https://bugzilla.redhat.com/show_bug.cgi?id=1002375  ?  

This is the CVE-2013-4288 polkit: unix-process subject for authorization is racy ? 

At this moment I have a possible patch for it and also the author of the polkit-qt bindings is looking into this one. 

Thanks

Right.

I have no idea whether the QT binding or the KAuth needs to be fixed.
But just passing the pid is definitely wrong.

Sebastian, 

KAuth doesn't directly communicate with PolKit, but does this through the Qt bindings for PolKit. Therefore the bindings have to be adjusted as they perform the calls to PolKit. 

This is the proposed step, which should bring it up to the right level of PolKit: 
--- a/core/polkitqt1-subject.cpp
+++ b/core/polkitqt1-subject.cpp
@@ -122,13 +122,13 @@ Subject Subject::fromString(const QString &string)
 UnixProcessSubject::UnixProcessSubject(qint64 pid)
         : Subject()
 {
-    setSubject(polkit_unix_process_new(pid));
+    setSubject(polkit_unix_process_new_for_owner(pid, 0, getuid ()));
 }

 UnixProcessSubject::UnixProcessSubject(qint64 pid, quint64 startTime)
         : Subject()
 {
-    setSubject(polkit_unix_process_new_full(pid, startTime));
+    setSubject(polkit_unix_process_new_for_owner(pid, startTime, getuid ()));
 }

 UnixProcessSubject::UnixProcessSubject(PolkitUnixProcess *pkUnixProcess)

However Martin Briza (from redhat and the author of the Qt bindings for polkit) is also looking at the issue at this moment. 

So, hopefully we get this sorted out today or latest tomorrow. As soon as Polkit-qt-1 is patched, I will create an update and submit it.

If getuid() is what you want. If thats a DBUS activation as root,
that grants everything. Is there something in KAuth that allows
for credential passing?

please just note that we do not have polkit-qt-1 enabled for Qt5. there is upstream support for it in a branch, but the libs, pc files, etc, are not renamed. 
until rename is done, it will not be submitted in the distribution, and also KAuth is basically ineffective.
if this is a major issue - then it would be more important to adjust polkit-qt-1 and kdelibs(4).

@Sebastian,  

As far as I know getuid() gives me the user of the current process. The call to  PolkitQt1::UnixProcessSubject subject(pid) comes from within KAuth where KAuth tries to validate if the user/process is authorized to perform that action and return that value back to the calling program. So I am not sure how this would fit in a DBUS activation as root ? 

This would mean that the program is running under root and then KAuth will indicate that the user is authorized. I just followed the same approach as that Red Hat did for the spicy-gtk library, so I guess that I am missing the point. Or I am a little bit too naive. 

@Hrvoje. This issue came up when asking for a security review for smb4k, where Sebastian mentioned that KDE upstream is not following up on security issues. That is where I picked this up and tried to come up with an acceptable solution. The patch was reviewed (https://git.reviewboard.kde.org/r/117056/) and I got a Ship It! for it so that it gets fixed for KDE4

Created attachment 583670 [details]
poor man's patch

Sebastian, could you review this one?

Raymond correctly pointed out that KAuth uses polkit-qt as a backend, so i don't think that we need to change function names in KAuth/kdelibs

Thanks for your efforts.

I dont think using uid is the right way for the framework to check
authorizations.

Consider org.kde.fontinst.service DBUS service, that is activated
on behalf of users request as a root service. It will therefore run with uid 0,
even if triggered by user.
For now it is just using the pid of user requesting the service.
Thats racy and the thing we want to fix. Your patch proposal also
integrates the uid, but I fear thats the uid of the currently running
process (root == 0). From the small patch I cannot see where the
uid is coming from. If that would be the uid of the requesting user,
that would be fine (although not perfect if suid helpers request DBUS
services).

The preferred way is to use system-bus-name polkit authorization.
polkit-qt bindings seem to offer SystemBusNameSubject class already,
so is it possible to use that in KAuth rather than UnixProcess subjects?

Sebastian, 

uid comes from the following routine:

qint64 UnixProcessSubject::uid() const
{
    return polkit_unix_user_get_uid((PolkitUnixUser *) subject());
}



At this moment KDE is undergoing a heavy transformation into the KDE Frameworks and maybe we can get the changes for KAuth done there. However this will not resolve the KDE4 stuff. Therefore we are trying to make just small changes in order to make things work. And lets be fair, this has been working for the last years without major concerns until you discovered the racy condition. So we are trying to accommodate this and using the new method to resolve the racy condition. I don't see this rewrite for KAuth happening for KDE4 and my assumption is that openSUSE stands alone in this and as openSUSE doesn't deliver any developer resources to KDE, I guess we have to wait until it becomes an issue within Fedora/Red Hat. So in my opinion we have the following choices:

1)  We leave things as it is and accept the racy condition for KDE 
2)  We adjust polkit-qt-1 to move away from the deprecated method and using the new one that prevents the racy condition, but is still PID based.
3)  Neither of the above is accepted, which would mean that KDE is a security risk and it is then up to the security team to decide whether or not KDE should be removed from Factory. 

Maybe black and white, but these are the choices I see at the moment for KDE4 in openSUSE. 

I will test Hrvoje's patch today to see if this one works as expected and then I will submit it for review to KDE Upstream. And this is the best that I can do at the moment.

(In reply to comment #14)
First, just to say that this is a patch for polkit-qt-1, based and slightly extended on Raymond's.

> Thats racy and the thing we want to fix. Your patch proposal also
> integrates the uid, but I fear thats the uid of the currently running
> process (root == 0). From the small patch I cannot see where the
> uid is coming from. If that would be the uid of the requesting user,
> that would be fine (although not perfect if suid helpers request DBUS
> services).
In reality, we have no SUID helpers in KDE - except for the kdeinit's OOM killer ;-) UID is the one of the requesting user - i've tested the patch, and from user perspective, things still operate as before - e.g. for killing other users processes in KSysGuard i need to enter root pass, also for changing clock, etc.


> The preferred way is to use system-bus-name polkit authorization.
> polkit-qt bindings seem to offer SystemBusNameSubject class already,
> so is it possible to use that in KAuth rather than UnixProcess subjects?
As Raymond pointed out, our chances for chaning the internals are more for the KAuth framework/polkiq-qt-1 based on Qt5, rather than in kdelibs4/Qt4 world...

I also tested the patch locally and have the same result as Hrvoje.  The same behavior (requesting authorization for killing processes, change system clock, etc) as before the patch is there. So from that perspective, I believe we have the right one and resolved the issue with the racy condition.

@Raymond,

I see that uid comes via

return polkit_unix_user_get_uid((PolkitUnixUser *) subject());

my question is how is it constructed into the subject at the first place,
as it was originally a process subject and I doubt the uid will magically
appear by just type-casting it to a PolkitUnixUser *.

If thats taken from the unix socket credentials - thats OK.

If thats filled in by polkit libs itself since it reads
/proc/$pid/status when no uid is explicitely given on construction, then
its a NULL patch and we should not waste time making updates for it. :)

(Serious, I am not deep enough into KDE and QT to see immediately which of both
is the case, but I fear its the later.)

I wonder whether just using SystemBusNameSubject would be so
intrusive? I am sorry for playing the bad cop here but I am not
yet convinced that the patch makes sense.

@Sebastian,

Unfortunately I am not a developer and I just manage to resolve some bug fixing, etc. So I am the wrong person to indicate that using SystemBusNameSubject is intrusive. It could mean that a part of KAuth has to be rewritten and the question is if there are resources available to do so as that almost everybody is working on getting KDE Frameworks 5 going. 

Do you have examples on how to use SystemBusNameSubject ??  Calls seem to be pretty similar to the UnixProcess one, but I am not sure what kind of parameter it requires. Can I make this equal to the name of the application that is requesting the authorization or does this needs to be a specific value ? e.g. Application has to register itself first on the SystemBus and then this name has to be used ? 

It would be great if you could assist me in this part and maybe together we can manage to rewrite KAuth. 

Thanks

The ideal case would be to ask the maintainer/author of KAuth,
because he should know best. Unfortunally security@kde is unresponsive,
and so is the KAuth author. :/

Maybe we can go ahead and getting help from polkit upstream.

Using SystemBusNameSubject requires to pass the sender (from dbus,
not the PID) as
an argument. I dont know where we can get that from in KAuth.
The callerID thats inside KAuth is also just the PID as a byte-array.
Seems like the whole KAuth is just based on the PID.

Maybe thats hard to backport it for existing
installs, but at least upcoming KDE should include a KAuth thats
using SystemBusNameSubject.

Thanks for your help. I will try harder to allocate ressources
to get that fixed in upcoming KDE versions.

Adding Will to Cc, maybe he has insight into hows it possible
in KDE to pass DBUS sender along into KAuth so it can be
used for authenticating requests.

Adding Dirk as well.

@Sebastian,

Doesn't the openSUSE team within SuSE have their own KDE developer (Antonio Larrosa) ?  As far as I know when Will left the openSUSE team, a new developer was hired specifically to cover the KDE area. I guess he would be the right person from within SuSE.

Raymond

Added Antonio, maybe he can help.

Sebastian,
can we revisit the status of this? Missing DBus configuration is now starting to create some real problems with KF5 based desktop.

The deprecated polkit method in polkit-qt5 bindings has been updated to polkit_unix_process_new_for_owner.

Wrt SystemBusName, i have talked with de-facto maintainer of KDE's auth/polkit components, and he said that he didn't understand why would that be better than current solution. You can reach him at mbriza [at] redhat.com.

@Sebastian,

It seems that this one is really becoming a blocker for delivering KDE for openSUSE. Crippling KDE with disabling KAuth is not the right approach as that would cause certain functionality to stop working or even break KDE itself. If this is the risk that SuSE want to take with SLE12, then this is up to the SLE maintainers. However I will not accept crippling KDE in such a way for openSUSE. 

So I guess that the Security team now really has to take the decision that they can accept something, that is accepted by all other distributions or that they want to continue the very strict policy (remember the words of Linus Torvald about the openSUSE security setups) and block this. 

In the latter, I will send delete requests for KDE to openSUSE:Factory and will blog that openSUSE has dropped official support for KDE in their distribution. We will then deliver KDE through other repositories on OBS for those users that still want to use KDE on openSUSE. 

In my opinion we have been discussing this issue already too long and a decision needs to be taken now as that we see that functionality is starting to fail. 

It would be a shame to see KDE disappear from the openSUSE distribution.

Raymond

> s the KDE folks was not responding to various mails from us

security@ko is not the right place to ask these questions. Either contact Martin Briza (see previous message from Hrvoje) or mail the Frameworks mailing list (kde-frameworks-devel@kde.org). These are the best places to contact upstream.

Nevertheless, this should not block KAuth from entering Factory. Not having this breaks several components needed for a basic desktop experience, including power management.

P0 is reserved. reduce sev/prio

Antonio?

I already contacted Martin Briza yesterday and I am looking into
the patches that were mentioned for qt5.

I just looked at the provided diffs and the same problem remains.
Using -1 as uid brings nothing, as it again looksup the UID via
/proc, which we want to avoid in the first place.

The problem in KAuth is that due to the layers there is no way
to determine who actually is trying to authenticate the polkit action.
The dbus sender is not available that should be used for this.
And getuid() might be misleading because its already running as root
due to DBUS activation on behalf of the user we want to authenticate.

I am sorry that the issue is stuck. We were not just contacting security@kde,
but also several developers and it was entirely silent. So its not
to blame us.

If I get an override from management to include the KAuth thats
actually not authenticating anything, I'll happily do that.

In KAuth, the action autentication is IIRC done by the helper KAuth is calling, not KAuth itself.

For a breakdown of how KAuth works, which may lead to an understanding of the issue:

https://techbase.kde.org/Development/Tutorials/KAuth/KAuth_Basics

As I said, KAuth is community-maintaned (no single maintainer). That's why I mentioned kde-frameworks-devel. Contacting individual developers is likely to be inefficient on this regard.

I correct my statement, KAuth is the one elevating privileges. (see the link and the "writing helpers" section)

Did you also try contacting Dario Freddi? Although he's not much involved in KDE nowadays, he's the original developer of this system.

(In reply to comment #33)
> The problem in KAuth is that due to the layers there is no way
> to determine who actually is trying to authenticate the polkit action.
> The dbus sender is not available that should be used for this.
> And getuid() might be misleading because its already running as root
> due to DBUS activation on behalf of the user we want to authenticate.

If the (potential) vulnerability is in case of SUID helpers, we can have this case closed. As i wrote somewhere above, Qt, since 5.3, aborts action if the Q*Application is SUID. Applications can explicitly override this, but i am happy to add a patch to our Qt5 packages that would also disallow even that.

Adding myself as board watcher.

To clear the confusion here. This is not about suid helpers. This is a possible root hole using KAuth. So a rogue app using KAuth could gain root privileges and this is due the way PID checking is done, which found to be racy.

If Dario is drf@, I mailed him about it on April 7th, Ccing to 2 other
ppl.

Two examples of vulnerable KDE services are:

 /usr/share/dbus-1/system-services/org.kde.fontinst.service or
 /usr/share/dbus-1/system-services/org.kde.kcontrol.kcmclock.service

which can be DBUs-activated by users to run as root and which use KAuth
to check whether this user would be allowed to do that action.
This check can by bypassed since process-subject, as used by KAuth, is
racy. Please refer to

CVE-2013-4288 and the followups CVE-2013-4311, CVE-2013-4324, CVE-2013-4325,
CVE-2013-4326 and CVE-2013-4327 which all fix exactly the same issue.

Please write to kde-frameworks-devel@kde.org then, so that the issue may reach people able to handle this. KAuth has no official maintainer, meaning that the responsibility of handling it is shared among all the people working on the KDE Frameworks.

Also, does one know how other distros such as Debian and Red Hat handled this?

I am tired of reaching-out to the KDE folks again and again
explaining the issue the 42nd time to them. Can you do the short
path of telling them about this bug#, as you seem to be member of
the KDE team anyway?

This issue has been reported to other distros via the OSS-sec
list but did not found major ack. Thats probably b/c of a missing
PoC. In such cases nobody sees the severity because its 'theoretical'.

@sebastian, @Ismail,

Are we trying to make openSUSE into SLE here ??  Or are we trying to make the most secure distribution that nobody is able to use anymore due to all kind of security enhancements. Remember that in the eyes of many people SUSE already has the label of being the distribution that is kinda paranoid about security. Remember the flames by Linus Torvald ???

It seems also that this discussion is going around and around as that Sebastian is coming up again with the racy stuff in KAuth, despite that Martin Briza already indicated that this was resolved by moving to the the new polkit methods that were actually even coming sebastian, so if those are still racy then I don't know anymore but to believe we are on a witch hunt to get KDE kicked out from SLE and openSUSE. 

Other distributions like Fedora, etc do not seem to have any issues with the current situation as that they are using the code as it is being delivered from KDE upstream. 

At this moment, I really feel like just sending out the message to the whole world indicating that openSUSE will no longer deliver KDE due to the enhanced security setup and that the KAuth methodology doesn'f it in there. 

Raymond

Issue raised on kde-frameworks-devel: http://lists.kde.org/?l=kde-frameworks-devel&m=140552012224337&w=2

Raymond,

I am sorry you have such bad feelings against our sec-team but we are
just trying to get a real solution to this problem. We wont add
placebo-security fixes just so that everybody feels happy and honestly
I dont mind what Linus or other distros would think about it.

Due to KAuth layered lasagne-software, this is not just a one-line fix.

I hope recent posting will accelerate this. If you are in Berlin
one day I'll invite you to a beer to forget about all the hassle.
Its not that we dont honor or respect your work but we want to get
the KDE stuff sorted before its checked in.

I'm going through this on IRC, at least someone is looking at the code. I'll keep you posted.

And @Sebastian: the design of KAuth may not be the best, but as a Framework it supports also OSX and (perhaps in the future) Windows, so some abstraction is necessary (this of course doesn't mean it has no flaws). This is not a criticism, but helps in putthing things in context.

Currently testing patches that use SystemBusNameSubject. Will report back once finished.

Created attachment 598877 [details]
Patch to KAuth

Patch by Martin Sandsmark from KDE. Is this what is needed?

(In reply to comment #51)
> Created an attachment (id=598877) [details]
> Patch to KAuth
> 
> Patch by Martin Sandsmark from KDE. Is this what is needed?

Patch looks right but I am not an expert. NEEDINFO for Sebastian. Also could you please test the patched kdelibs and make sure systemsettings->clock still works as expected? Thanks a lot!

The patch works with KAuth. (iow consumers do correctly authenticate, execute actions, etc)
I am not a fan of patching kdelibs4 though, unless *really* necessary

I rebased the patch on kdelibs as well (the previous one was for KDE Frameworks 5). We'll be testing it while awaiting Sebastian's assessment.

Created attachment 598930 [details]
Patch to KAuth v.2

Updated patch, was one more place with PID usage

Created attachment 598931 [details]
kdelibs4 patch

Patch for kdelibs4 (just a line/dir replace, otherwise is the same)

@Ismail: From a functional point of view, the patch works in both the KF5 and the kdelibs 4.x versions (tested: backlight helper for PM init, ksysguard process helper, date and time helper).

All that's left to know if it addresses the security concerns.

(In reply to comment #57)
> @Ismail: From a functional point of view, the patch works in both the KF5 and
> the kdelibs 4.x versions (tested: backlight helper for PM init, ksysguard
> process helper, date and time helper).

Thanks a lot for handling this. A crippled KDE means a black eye for openSUSE and SLE, for which we never wanted to cripple it in the first place.

> All that's left to know if it addresses the security concerns.

As soon as Sebastian approves we can proceed. I'll handle the SLE side obviously.

Sebastian is not present on Thursday and Friday. 

I am not so familar with kauth/polkit.

Not sure if it is correct, we match the system busname (?) and not client identifiers?

Luca, did you also do negative checks? like testing that stuff corrctly gets forbidden or admin dialog popups?

I tried:

- non-interactive helpers (backlight, where no password is requested)
- interactive helpers (ksysguard + clock): in this case the authorization dialog pops up and requests the root password. 

Canceling or inserting the wrong password does not execute the action (as expected).

@Marcus http://www.freedesktop.org/software/polkit/docs/latest/PolkitSystemBusName.html

Basically it uses DBus, to my understanding, to get the process instead of using the PID, which is open to race conditions (the objection raised by Sebastian).

I made a further negative test with multiple wrong passwords: as expected, the actions refuse to be executed and errors are displayed (I also checked that the action - increase clock time by 1 min - was not actually done).

Thanks for doing this!

I think we are probably good for approval and I will do that.

Sebastian might have comments when he is back on Monday.

(In reply to comment #59)
> Sebastian is not present on Thursday and Friday. 
> 
> I am not so familar with kauth/polkit.
> 
> Not sure if it is correct, we match the system busname (?) and not client
> identifiers?
> 
> Luca, did you also do negative checks? like testing that stuff corrctly gets
> forbidden or admin dialog popups?

In case of successful authentication, one now gets e.g.:

polkitd[22395]: 06:46:24.368: Operator of unix-session:59 successfully authenticated as unix-user:root to gain ONE-SHOT authorization for action org.kde.ksysguard.processlisthelper.sendsignal for system-bus-name::1.3728 [/usr/bin/systemmonitor] (owned by unix-user:hrvoje)

in case of failure:
polkitd[22395]: Operator of unix-session:59 FAILED to authenticate to gain authorization for action org.kde.ksysguard.processlisthelper.sendsignal for system-bus-name::1.3885 [/usr/bin/systemmonitor] (owned by unix-user:hrvoje)

So this indeed looks like the procedure that was asked for ;-)

(In reply to comment #63)
> I think we are probably good for approval and I will do that
Many thanks for resolving this! (also to others involved!)

This is an autogenerated message for OBS integration:
This bug (864716) was mentioned in
https://build.opensuse.org/request/show/241367 Factory / rpmlint

This is an autogenerated message for OBS integration:
This bug (864716) was mentioned in
https://build.opensuse.org/request/show/241428 Factory / kauth

Ok, this one looks much better!

I am not KDE expert but the patch looks as this subject indeed
is the race-free object bound to the DBUS originator of the request.


Ismail, can we revert recent SLE12 FAKE patch to be substituted by this?

I hope the patch covers all necessary subject-based code in Kauth.

This patch went through code review upstream (which prompted the generation of the v2 patch) as well, along with some direct (on IRC) discussion with the person who wrote the initial version.

Sebastian, if you do not have further objections let us know: I'll tell upstream to merge this in stable and development branches (so that everyone benefits from this security review).

I just requested a CVE and Cc'ed oss-sec so other distributors can
also benefit from our hard work.

SLE part was checked in last Friday. Thanks for all those involved!

FYI, the patch has been merged upstream for both 4.13, 4.14 and KF5.

(In reply to comment #72)
> FYI, the patch has been merged upstream for both 4.13, 4.14 and KF5.

once we get CVE, i'll start maintenance sr's for 12.3 and 13.1.

this one can be closed now though

CVE-2014-5033

This is an autogenerated message for OBS integration:
This bug (864716) was mentioned in
https://build.opensuse.org/request/show/242441 13.1 / kdelibs4-apidocs+kdelibs4+kdebase4-workspace

openSUSE-SU-2014:0981-1: An update that solves one vulnerability and has one errata is now available.

Category: security (moderate)
Bug References: 819437,864716
CVE References: CVE-2014-5033
Sources used:
openSUSE 13.1 (src):    kdebase4-workspace-4.11.11-115.3, kdelibs4-4.11.5-484.1, kdelibs4-apidocs-4.11.5-484.1