I will take it.
Where is the source?

PolicyKit checked into BETA.

/work/SRC/BETA/all/PolicyKit

Overnight it also gained a setuid root pam helper binary for unknown reasons.

we need to audit that one too.

The whole PAM conversation to authenticate as the own user or as root has been split out to its own 200 lines helper, which doesn't have any other dependency.

For us at least, it needs root privileges to be able to read /etc/shadow or NIS passwords. Other distros, not using PAM, would like to use replace that part with their own binary here doing the authentication chat.

/work/SRC/BETA/all/PolicyKit has been updated to the latest version. Thanks!

So, we only talk about polkit-grant/ subdir and the library calls
it needs right?

Yes, it is all about the invoked setgid binary, which creates the "auth cookie", which is read by the system-service.

I have installed the packages on "westernhagen" (10.3 alpha7, hal,policykit,consolekit taken from BETA)

-rwxr-sr-x 1 root polkituser 19117  4. Aug 13:51 /usr/lib64/PolicyKit/polkit-grant-helper
-rwsr-xr-x 1 root root       16401  4. Aug 13:51 /usr/lib64/PolicyKit/polkit-grant-helper-pam

The code itself does not look problematic.

However I do not like that all the suids/sgids are linked against
blown libs such as libdbus or glib and its dependencies. This will
cause trouble sooner or later.

Kay, are DBUS and GLIB dependencies necessary for both setgid/setuid helper
programs or can we avoid them?

The separate setuid root binary exists only for the reason not to link these libraries while running as root and using pam.

It is only the setgid polkituser binary which links dbus and glib, and that isn't trivial to change.

ldd /usr/lib64/PolicyKit/polkit-grant-helper-pam
  libpam.so.0 => /lib64/libpam.so.0 (0x00002b213f159000)
  libc.so.6 => /lib64/libc.so.6 (0x00002b213f365000)
  libaudit.so.0 => /lib64/libaudit.so.0 (0x00002b213f6a9000)
  libdl.so.2 => /lib64/libdl.so.2 (0x00002b213f8be000)
  /lib64/ld-linux-x86-64.so.2 (0x0000555555554000)

ldd /usr/lib64/PolicyKit/polkit-grant-helper
  libpolkit.so.1 => /usr/lib64/libpolkit.so.1 (0x00002b3bce363000)
  libpolkit-dbus.so.1 => /usr/lib64/libpolkit-dbus.so.1 (0x00002b3bce56f000)
  libdbus-1.so.3 => /lib64/libdbus-1.so.3 (0x00002b3bce774000)
  libglib-2.0.so.0 => /usr/lib64/libglib-2.0.so.0 (0x00002b3bce9ad000)
  libc.so.6 => /lib64/libc.so.6 (0x00002b3bcec74000)
  libexpat.so.1 => /lib64/libexpat.so.1 (0x00002b3bcefb8000)
  /lib64/ld-linux-x86-64.so.2 (0x0000555555554000)

Although there are no immidiate bugs, we should try to avoid
new suid/sgid binaries.

It is the alternative to the current polkit daemon running as root using dbus, using glib and pam, and talking to users.

Reset NEEDINFO.

PolicyKit does not build with the new permissions file:

Running chkstat with permissions and permissions.secure
found the following differences:

/usr/lib/PolicyKit/polkit-grant-helper-pam from RPMS/i586/PolicyKit-0.4-7.i586.rpm
/usr/lib/PolicyKit/polkit-grant-helper-pam should be root:root 0755. (wrong permissions 4755)

Ludwig, can you investigate, please?

Marcus fixed it now

Please update permissions for PolicyKit 0.7, which has the following programs:
  %attr(2755,root,polkituser) %{_libexecdir}/polkit-set-default-helper
  %attr(2755,root,polkituser) %{_libexecdir}/polkit-read-auth-helper
  %attr(2755,root,polkituser) %{_libexecdir}/polkit-revoke-helper
  %attr(2755,root,polkituser) %{_libexecdir}/polkit-explicit-grant-helper
  %attr(2755,root,polkituser) %{_libexecdir}/polkit-grant-helper
  %attr(4750,root,polkituser) %{_libexecdir}/polkit-grant-helper-pam

Other packages depend on the updated version, would be nice if that could be done in time. Thanks!

Package copied to STABLE. Please update permissions.

All binaries are called by untrusted user processes and need to be able to access the PolicyKit database, therefore the setgid=polkituser. The pam helper needs to be setuid=root to verify passwords.

Sourcde files are here:
  src/polkit-dbus/polkit-set-default-helper.c
  src/polkit-dbus/polkit-read-auth-helper.c
  src/polkit-grant/polkit-revoke-helper.c
  src/polkit-grant/polkit-explicit-grant-helper.c
  src/polkit-grant/polkit-grant-helper.c
  src/polkit-grant/polkit-grant-helper-pam.c

Is westernhagen still set up with current PolicyKit?

no, westernhagen does not have it currently. (I also have no latest STABLE updates currently.)  Perhaps time to update ;)

I hope my user is allowed to invoke it, as it asks DBUS to do so.

polkit-set-default-helper: since its setgid, files created are
owned by calling user, thus file-content in /usr/local/lib/PolicyKit-public/
can be changed afterwards at any time; if thats not a problem.
So, the only reason why this needs 02755 is that it is able to move
files to this directory??

I talked to the author: It is a known minor issue, which will be addressed soon. A user can not gain any privileges by changing the file content, it will just change the supplied defaults.
If we (SUSE) want to wait for the final fix, we can leave it without setgid for now and a call to change defaults will just fail, which should not prevent the current users to continue working.

/usr/local/lib/PolicyKit-public/ looks wrong as well - it should not install in /usr/local at all.

/usr/local is my local compilation.

For now I dont see much reason to have it setgid, more or less
current installation is like having the directory mode 1777,
since files are user-owned anyway. They probably mean setuid polkituser.

We have the powermanagement stuff as well as PackageKit, powermanagement, pulse audo and storage mounting all using policykit now.  The command line tools throw errors now and refuse to give info.  This also allows users not to do all or nothing with things like pluggable usb devices.

Given that, I don't think we can ship 11.0 without this change.

pulseaudio will and should just work fine without policykit answering.
(otherwise it is a bug)

hal mounting of removable devices works fine, without those additional setgid bits too.

well, not with gnome-mount ... 

gnome-mount has org.gnome.PolicyKit hardcoded, whiole the service is called
org.freedesktop.PolicyKit.

ah,. org.gnome.PolicyKit starts the /usr/lib64/PolicyKit-gnome/polkit-gnome-manager,
while org.freedesktop.PolicyKit is something else.

confusing at best.

The 464 file permissions do not help, if the user has write access thjey can just write into the file (ownership overrides above).


However, as /var/lib/PolicyKit is 770 for polkituser:polkitgroup, it is fine
(users should not be in polkitgroup).

To dispel more FUD, powersave does not need the GRANT mechanisms for all regular cases, except multisession shutdown/reboot.

And neither does regular removable storage mounting, as said previously.


However there are cases that need it, including PackageKit.


So I will discuss this further tomorrow and try to see a solution.

discussed and reviewed together with Ludwig.

We will add the setgid bits to the above listed grant helper binaries.

Currently in "easy" mode, but not in "secure" and "paranoid" yet.

-> Ludwig for inclusion into permission package.

Will do. Could you please change the %_libexecdir to %_prefix/lib please? Having to specify all binaries twice sucks. There are bugs open to change the macro in rpm but it looks like it's intentionally broken..

done for
/usr/lib/PolicyKit/polkit-set-default-helper   
/usr/lib/PolicyKit/polkit-read-auth-helper     
/usr/lib/PolicyKit/polkit-revoke-helper        
/usr/lib/PolicyKit/polkit-explicit-grant-helper
/usr/lib/PolicyKit/polkit-grant-helper         
/usr/lib/PolicyKit/polkit-grant-helper-pam     

Please change the use of %_libexecdir

Submitted.

(In reply to comment #25 from Sebastian Krahmer)
> polkit-set-default-helper: since its setgid, files created are
> owned by calling user, thus file-content in /usr/local/lib/PolicyKit-public/
> can be changed afterwards at any time; if thats not a problem.

http://gitweb.freedesktop.org/?p=PolicyKit.git;a=commitdiff;h=149a3df1926c24b51b0f0336ac051473aed980fe

New version submitted:
  /work/src/done/STABLE/PolicyKit/

Please add:
  %{_prefix}/lib/PolicyKit/polkit-resolve-exe-helper
as:
  %attr(4755,root,polkituser)

Thanks!

what is this new helper used for?

setuid root helper for PolicyKit to resolve /proc/$pid/exe symlinks

"Add constraints for exe and SELinux context when granting an
authorization. The way it works is that added constraints now look
like this:
  scope=always:action-id=org.freedesktop.hal.storage.mount-fixed:
  when=1197008218:auth-as=0:constraint=local:constraint=active:
  constraint=exe%3A%2Fusr%2Fbin%2Fgnome-mount
This is a bit icky to implement for mechanisms, like HAL, running
as an unprivileged user. The problem is that we can't resolve the
symlink /proc/pid/exe. On the other hands such mechanisms has the
authorization org.freedesktop.policykit.read already. So use that."

http://gitweb.freedesktop.org/?p=PolicyKit.git;a=commit;h=a8e46ceb39137582b0fbacb69fdfda72ae7139f8

Please update the permissions, some things are not working as expected now.

this not a valid bugreport.

what is the error?

are you perhaps stumbling about the "does not create user problem" in beta1?

You can not mount volumes which require authorization to mount them. This is a very real problem. There are probably other authorizations failing too.

Is this the beta1 "polkituser" missing problem?

What is the errormessage?

Is it truly missing the exe symlink reader? 

Yes, it is. Mounting built-in drives does not work. Anyway, regardless of this specific bug, PolicyKit is broken without proper permissions.

  $ rpm -q PolicyKit
  PolicyKit-0.7.git20080410-3

  chmod u-s /usr/lib/PolicyKit/polkit-resolve-exe-helper
  makes it working as expected

HAL log:
polkit-resolve-exe-helper: cannot do setuid(0): Operation not permitted
polkit-resolve-exe-helper: cannot do setuid(0): Operation not permitted
polkit-resolve-exe-helper: cannot do setuid(0): Operation not permitted
polkit-resolve-exe-helper: cannot do setuid(0): Operation not permitted
21:37:32.258 [I] device.c:1894: Removing locks from ':1.7908'
pid 32199: rc=1 signaled=0: /usr/lib64/hal/hal-storage-mount
21:37:32.259 [I] hald_dbus.c:4042: No more methods in queue
21:37:32.259 [I] hald_dbus.c:4105: failed with 'org.freedesktop.Hal.Device.PermissionDeniedByPolicy' 

Will have a look... 

All callers pass a static size, large enough for the common case. The return value of readlink() is checked, and the call fails if it is truncated.

I can see nothing which would immediately need to be fixed.

The upstream author is informed, and will change it according to the suggestions, and we will get the changes with the next version update. Thanks!

Still broken. Please adapt permissions.

  using action org.freedesktop.hal.storage.mount-fixed for uid 2702
  polkit-resolve-exe-helper: cannot do setuid(0): Operation not permitted
  polkit-resolve-exe-helper: cannot do setuid(0): Operation not permitted

*** Bug 392513 has been marked as a duplicate of this bug. ***

The policykit source code documentation for polkit_sysdeps_get_exe_for_pid() clearly states that containts based on the calling program's name cannot be relied upon as firstly they can be easily circumvented by the user himself and secondly don't work for interpreted languages. Therefore those exe name constaints are no security feature, just annoyance. The feature should be patched away instead of adding a setuid binary that cannot fix it's shortcomings anyways.

Marcus, can I have an executive summary please? This is so far the only RC1 blocking bug.

The helper's code is "ok" but what about comment #58?

Created attachment 218263 [details]
patch to disable sysdeps_get_exe_for_pid

Kay, is this working for you? We need a decision asap if we go with PolicyKit or permissions ;(

Nope, I will not apply that. Ludwig, get involved in upstream development, if you want to influence stuff. We are not going to change behaviour for no good reason.

that sounds like a bad interlock problem - as SUSE won't add suid permissions without good reasons.

SUSE also does not patch upstream software, because someone does not like a feature, for no good reasons.

The authorization scheme seems to be broken by design, it would not help adding a suid bit.

Same for you, express your opinion on the upstream mailing list, not in this bug. The patch does not add any security, so it is not needed.

If there is no concern regarding the security of this binary, please adapt the permissions as requested, I will not remove any feature because you personally don't like it.

(In reply to comment #58 from Ludwig Nussel)
> The policykit source code documentation for polkit_sysdeps_get_exe_for_pid()
> clearly states that containts based on the calling program's name cannot be
> relied upon as firstly they can be easily circumvented by the user himself and
> secondly don't work for interpreted languages.

It does however work for secure programs.

(In reply to comment #66)
> The authorization scheme seems to be broken by design, it would not help
> adding a suid bit.

I'm not sure what to make of this comment.

Coolo: Sorry for not answering sooner, I am in a training.

Executive summary:

1. It is not clear why mount.fixed permissions are required at all, Kay nor anyone else has not answered our several queries regarding this.

   This question needs to be answered first, all others depend on that one.


   Ability to mount fixed disks/partitions is definitely a critical security risk.


2. There is apparently no code in FACTORY exercising this for us to even look at.


3. The helper binary itself is fine in a self contained way.


4. The use of "exe" name checking does not help, if the user has control over this executable via LD_PRELOAD, PTRACE or other means. He can just inject any code into this binary. Davids comment regarding "secure" binary likely means that the user binary itself needs setgid/setuid permissions.

"exe" path name checking is not a security feature in any way and should not be handled as such, as the source code even says.

This is our integral concern regarding this.


5. We will NOT add setuid permissions to any random binary just because it is "fine by itself".

Fixed disks are different from removable disks, because one could be able to access the secure data from other operating systems on a multi-boot box. Therefore, mounting fixed disks have their own policy, and require the root password. And it works fine.

Please stop this, it is so pointless. The "exe" check is an additional check, add not any "security feature", does not harm anything, and does not need "fixing" in any sense.
And again, get involved in upstream development, discuss the stuff there, if you want to influence what Linux is doing today. Otherwise please stop wasting my/our time.

I will refuse any of these patches, which makes SUSE different from all other distros. I will just assign package maintenance to the one who will apply it, and not care anymore. I'm so tired of these useless "discussions" which never lead to anything in the past.

(In reply to comment #69 from Marcus Meissner)
> 1. It is not clear why mount.fixed permissions are required at all, Kay nor
> anyone else has not answered our several queries regarding this.
> 
>    This question needs to be answered first, all others depend on that one.

HAL allows you to mount "fixed" (e.g. partitions from non-hotpluggable disks that doesn't support removable media => typically windows, os x partitions) but that requires another authorization if it was non-fixed (e.g. partitions from hotpluggable OR removable disks). 

So in that case you get asked for the root password to do this. You can retain this authorization so you won't get asked for a password again. Which is the _sensible_ thing to do if you realize that users typing in passwords and getting interrupted by password dialogs is _bad_ "security". [1]

Anyway, distributions and/or sites can tweak this behaviour; for example

 # polkit-action --set-defaults-active org.freedesktop.hal.storage.mount-fixed auth_admin

will change the defaults such that the authorization can't be retained. All this works fine.

(And this is much more preferable than the rather useless patch you guys are shipping to PolicyKit-gnome (I found this out when I was meeting with Kay a few weeks ago) that just deselects the "retain authorization" checkbox... all that patch does is just to make the user type in his password again and again... Btw, I'd appreciate if you guys sent patches like that upstream so I don't have to explain why it's non-sense in a downstream bugzilla entry.)

>    Ability to mount fixed disks/partitions is definitely a critical security
> risk.

Depends on what OS you're shipping. If it's for a laptop this is definitely not true; clearly users wants to access files from the other OS on the laptop. However, if it's for a server it's definitely true. See the bug linked in [1] for more thoughts about this.

> 2. There is apparently no code in FACTORY exercising this for us to even look
> at.

Maybe you guys are still shipping a patch to HAL that hides all fixed disks (e.g. setting volume.ignore); I don't know. Either way, it works fine in Fedora and Ubuntu; I can happily mount my Windows and OS X partitions.

> 3. The helper binary itself is fine in a self contained way.
> 
> 
> 4. The use of "exe" name checking does not help, if the user has control over
> this executable via LD_PRELOAD, PTRACE or other means. He can just inject any
> code into this binary. Davids comment regarding "secure" binary likely means
> that the user binary itself needs setgid/setuid permissions.

Yes, that's what I meant with "secure" binary; it's shorter to write "secure binary" than go through a (non-exhaustive) list of what attack vectors to cover.

> 
> "exe" path name checking is not a security feature in any way and should not be
> handled as such, as the source code even says.

It's not really a security feature, I don't know where you got that idea from. Clearly, this bit should tell you otherwise

http://hal.freedesktop.org/docs/PolicyKit/polkit-polkit-sysdeps.html#polkit-sysdeps-get-exe-for-pid

However it _is_ useful for binaries that can be securely locked down. 

For the record, it's exactly the same approach that AppArmor and SELinux takes; e.g. in SELinux you can tag a process as running in a security context (through xattrs of the binary) and that way it can do more syscalls than other processes / get more privileges. Ditto with AppArmor, it just happens to be path-based. And surely such processes are just as vulnerable to code injection as other programs.. except that glibc secure mode and other bits kick in so you can't ptrace the process nor use LD_PRELOAD etc. But that still leaves plenty other attack vectors (e.g. PYTHONPATH, GTK_MODULES etc.). 

The "only" difference here is that PolicyKit is purely application based; the mechanism is exactly the same: you look at characteristics of the process wanting to access something to make a decision.

So if you're saying "omg, that exe thing is evil", I think you should examine how AppArmor works.

Hope this clarifies.

      David

[1] : the whole point of PolicyKit is absolutely not the password dialogs; users should simply have the authorizations they need to do their work / use their system. They should certainly *not* be interrupted by password dialogs. I'd go as far as saying that passwords dialog are evil and a disease. It's, for the most cases, bad security (trusted path is the exception). Anyway, there's a slightly longer and more detailed rant about this here

 http://bugzilla.gnome.org/show_bug.cgi?id=531609#c9

ok, after having another discussion with Marcus and Ludwig, I finally agree  that leaving out this feature for 11.0 is the right thing to do. And for the future, we might reconsider if it's not wiser to patch it out of PolicyKit. 

I would still recommend someone of our security team join the policykit upstream mailing list and discuss future developments there.

I'm unable to handle this any longer. I assigned package maintainership to Ludwig now. Please handle all further updates/issues/discussions/bugs/feature requests, I'm not available for this topic anymore.

(In reply to comment #73 from Stephan Kulow)
> I would still recommend someone of our security team join the policykit
> upstream mailing list and discuss future developments there.

That would be nice.

> ok, after having another discussion with Marcus and Ludwig, I finally agree 
> that leaving out this feature for 11.0 is the right thing to do. And for the
> future, we might reconsider if it's not wiser to patch it out of PolicyKit. 

As upstream for PolicyKit, this is unacceptable. I jumped into your downstream bugzilla to help clear the confusion. I spent a lot of time explaining to you guys how this works in comment 71. You could at the very least explain why you are crippling upstream sources. Don't expect me to care about SUSE PolicyKit bugs in the future if this continues.

David, thanks for your comments. We're not crippling the sources, we're just not setting it setuid for 11.0 as we don't need the feature.

I only wanted to say that for the future we're reconsidering all our options.

Sure, do we need it. I can not mount my fixed disk partitions, and maybe other things that are not part of the default install. That you don't need that, does in no way say others don't need it. PolicyKit is generic infrastructure, and you decided to ship a willfully broken version, for absolutely not valid reason. It's the equivalent of removing random functions from a shared library, because your system seems to run without them. Nice job!

Not _exactly_ - you can't readd the random functions, while a permissions.local entry is done in a second.

And yes, we remove all kind of things our system does not need while it's provided upstream. E.g. GBs of localization and documentation. That's basically the job of a distribution from my point of view.

So how many bytes did you save here? The job of a distribution is to provide a working system and working infrastructure for people to build stuff on top. You decided that openSUSE does want to do both of them. Really, what's needed to get a clue?

This is an autogenerated message for OBS integration:
This bug (295341) was mentioned in
https://build.opensuse.org/request/show/89843 Tumbleweed / permissions