Bug 1114383 - permissions: permissions.local is not honored by zypper updates (chkstat is not always called)
permissions: permissions.local is not honored by zypper updates (chkstat is n...
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Security
Current
Other Other
: P5 - None : Normal (vote)
: ---
Assigned To: Matthias Gerstner
E-mail List
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-01 22:02 UTC by Björn Voigt
Modified: 2022-09-28 12:20 UTC (History)
4 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Björn Voigt 2018-11-01 22:02:06 UTC
In the old openSUSE times, after each Yast package installation SuSEconfig was called, with did some useful things like checking and applying permissions from /etc/permissions*.

Permissions in /etc/permissions* now have to be checked and applied with "chkstat --system --set".

Unfortunately Zypper (and maybe Yast (not tested)) do not always call "chkstat --system --set" after packages are updated. As a result permissions may be incorrect after updates and before the user manually calls "chkstat --system --set".

For instances the following use case can lock out users:

Prerequisite for this example: some special PAM configuration (e.g. pam_yubico) only work, if /usr/lib64/libexec/kcheckpass is set with SGID bit.
* user is logged in
* user installs updates with "zypper dup"
* user locks the screen
* user tries to unlock the screen
* /usr/lib64/libexec/kcheckpass is changed after "zypper dup" (permissions are standard: -rwxr-xr-x)
* kscheckpass fails, user is locked out
Comment 1 Matthias Gerstner 2018-11-02 09:48:33 UTC
Thank you for the bug report. When permissions are not updated then this is a
per-package bug. The package in question needs to specify a line like:

%set_permissions %{_kf5_libdir}/libexec/kcheckpass

in the %post section of their RPM spec. For the case of kscreenlocker this
line seems to be missing. Therefore I am assigning this bug to the
kscreenlocker bug owner.

If you are aware of any other packages with this behaviour then please open
individual bugs for them. Actually we could also find such cases automatically
via scripting ... but there is no such script implemented at the moment. I
will discuss this in the security team. It maybe an area where we can improve.
Comment 2 Fabian Vogt 2018-11-02 10:05:26 UTC
In KDE4 days, kcheckpass was setuid root as can be seen in /etc/permissions:

# needs setuid root when using shadow via NIS:
# #66218
/opt/kde3/bin/kcheckpass                                root:shadow       4755
/usr/lib/kde4/libexec/kcheckpass                        root:shadow       4755
/usr/lib64/kde4/libexec/kcheckpass                      root:shadow       4755

This is not the case anymore, now kcheckpass is just a normal binary without any special capabilities.

Manually adding suid/sgid permissions to a system binary is not something accounted for in the package and it shouldn't need to be. Otherwise every package shipping a binary would need to call %set_permissions.
Comment 3 Matthias Gerstner 2018-11-02 10:31:30 UTC
(In reply to fvogt@suse.com from comment #2)
> This is not the case anymore, now kcheckpass is just a normal binary without
> any special capabilities.

If kcheckpass no longer needs setuid root then we should remove it from the
permissions package, too. It's only because the location of kcheckpass changed
that 'chkstat' doesn't reapply setuid permissions to it.

> Manually adding suid/sgid permissions to a system binary is not something
> accounted for in the package and it shouldn't need to be. Otherwise every
> package shipping a binary would need to call %set_permissions.

Every package shipping a *setuid* binary actually needs to call
%set_permissions as is also documented in [1].

[1]: https://en.opensuse.org/openSUSE:Packaging_Conventions_RPM_Macros#.25set_permissions

The question remains why the user can no longer unlock after the `zypper dup`
as explained in comment 0 then.
Comment 4 Fabian Vogt 2018-11-02 11:21:43 UTC
(In reply to Matthias Gerstner from comment #3)
> (In reply to fvogt@suse.com from comment #2)
> > This is not the case anymore, now kcheckpass is just a normal binary without
> > any special capabilities.
> 
> If kcheckpass no longer needs setuid root then we should remove it from the
> permissions package, too. It's only because the location of kcheckpass
> changed that 'chkstat' doesn't reapply setuid permissions to it.
>
> > Manually adding suid/sgid permissions to a system binary is not something
> > accounted for in the package and it shouldn't need to be. Otherwise every
> > package shipping a binary would need to call %set_permissions.
> 
> Every package shipping a *setuid* binary actually needs to call
> %set_permissions as is also documented in [1].
> 
> [1]:
> https://en.opensuse.org/openSUSE:Packaging_Conventions_RPM_Macros#.
> 25set_permissions

Exactly, but kscreenlocker does not ship anything like that.
What the reporter does is like this:

$ stat /bin/cat
(not setuid)
$ sudo chmod u+s /bin/cat
$ stat /bin/cat
(setuid)
$ sudo zypper in --force coreutils
$ stat /bin/cat
(not setuid)

Which is IMO working as expected.

> The question remains why the user can no longer unlock after the `zypper dup`
> as explained in comment 0 then.

kcheckpass invoked pam_* modules as normal user, which pam_yubico does apparently not support.
Not sure what the best fix here is:
a) Make kcheckpass setuid root again
b) Get pam_yubico to work without root privileges
Comment 5 Fabian Vogt 2018-11-02 11:55:28 UTC
Correction:

(In reply to Fabian Vogt from comment #4)
> What the reporter does is like this:
> 
> $ stat /bin/cat
> (not setuid)

$ sudoedit /etc/permissions.local # set cat setuid
$ sudo chkstat --system --set

> $ stat /bin/cat
> (setuid)
> $ sudo zypper in --force coreutils
> $ stat /bin/cat
> (not setuid)
Comment 6 Matthias Gerstner 2018-11-02 13:41:55 UTC
Ah, I missed the bit that the reporter seems to have added custom set*id
permissions for kcheckpass via the permissions file. So I got this partly
wrong.

Entries in /etc/permissions.local actually only work reliably for locally
installed files like in /usr/local/... that are not managed by zypper. Or for
overriding permissions of files that ship with set*id and thus call
%set_permissions and %verify_permissions.

There has been a long discussion regarding pam_yubico and gnome / KDE
screensavers [1]. It is a difficult topic. There will always be some PAM
modules that don't work without root privs. But having a lot of set*id
binaries is also not desireable.

SuSEconfig was removed in openSUSE 10.3, because it was only invoked by YaST
but not by zypper or rpm directly. I don't know of any replacement. Maybe
there is a possibility to run a hook script after certain zypper operations?
Adding the zypper maintainer, maybe he has got some input on this.

@fvogt: Since there seem to be at least some valid use cases to add a custom
setuid bit to kcheckpass, you could still add calls to %set_permissions and
%verify_permissions to your package, to allow users to override the
permissions in a defined away.

Otherwise only hacks come to my mind:

- adding a system start service or cron job to call 'chkstat' on a regular
  basis
- using a wrapper around 'zypper' to run chkstat after each zypper in/up/dup
  operation

[1]: https://github.com/Yubico/yubico-pam/issues/113
Comment 7 Fabian Vogt 2018-11-02 14:10:05 UTC
(In reply to Matthias Gerstner from comment #6)
> SuSEconfig was removed in openSUSE 10.3, because it was only invoked by YaST
> but not by zypper or rpm directly. I don't know of any replacement. Maybe
> there is a possibility to run a hook script after certain zypper operations?
> Adding the zypper maintainer, maybe he has got some input on this.

It might work to add

%transfiletriggerin -- /
/usr/bin/chkstat --system

to permissions.spec to run chkstat at the end of every transaction.

> @fvogt: Since there seem to be at least some valid use cases to add a custom
> setuid bit to kcheckpass, you could still add calls to %set_permissions and
> %verify_permissions to your package, to allow users to override the
> permissions in a defined away.

Yes, if a generic approach doesn't work it's something that can be considered.
Comment 8 Marcus Meissner 2018-11-02 18:33:22 UTC
the trigger is overkill I think, this will slow down any rpm update operation.

Preferable would be to tag it like Matthias says.
Comment 9 Björn Voigt 2018-11-02 21:41:21 UTC
Sorry, the commenters in #c1, #c2, #c3, #c4 and #c5 misunderstood me a bit.

I did NOT wrote this bug report to complain about /usr/lib64/libexec/kcheckpass permissions in package kscreenlocker. The kscreenlocker problem was meant as an example for a more general issue with Zypper and chkstat.

#c6 pointed this out correctly.
> Entries in /etc/permissions.local actually only work reliably for locally
> installed files like in /usr/local/... that are not managed by zypper. Or for
> overriding permissions of files that ship with set*id and thus call
> %set_permissions and %verify_permissions.

I do not think, that it is enough, that single packages call "chkstat --system --set". This may work with the files listed in

/etc/permissions
/etc/permissions.d/*
/etc/permissions.easy
/etc/permissions.paranoid
/etc/permissions.secure

because these files are more or less static and only a limited number of packages are affected. E.g. the files listed in /etc/permissions.easy come from around 40 RPM packages on my TW installation. In theory each of the >=40 packages may call chkstat in postinstall scripts.

But Root can list any file in /etc/permissions.local, e.g. files from package A.rpm and B.rpm. During a Zypper run, Zypper can for instance update packages C.rpm and D.rpm. Then it is useless to call "chkstat", if C.rpm and D.rpm does not contain files from /etc/permissions.{easy,paranoid,secure}. But if Zypper updates the packages A.rpm, C.rpm and D.rpm, then Zypper should call chkstat to update the permissions.

I find #c7 interesting:
> It might work to add

> %transfiletriggerin -- /
> /usr/bin/chkstat --system

> to permissions.spec to run chkstat at the end of every transaction.
Comment 10 Matthias Gerstner 2018-11-05 11:24:08 UTC
(In reply to bjoernv@arcor.de from comment #9)
> But Root can list any file in /etc/permissions.local, e.g. files from
> package A.rpm and B.rpm. During a Zypper run, Zypper can for instance update
> packages C.rpm and D.rpm. Then it is useless to call "chkstat", if C.rpm and
> D.rpm does not contain files from /etc/permissions.{easy,paranoid,secure}. But
> if Zypper updates the packages A.rpm, C.rpm and D.rpm, then Zypper should call
> chkstat to update the permissions.

Implementing this into zypper directly is most probably out of the question.

> I find #c7 interesting:
> > It might work to add
> 
> > %transfiletriggerin -- /
> > /usr/bin/chkstat --system

As c#8 says this approach looks like overkill.

I fear that there is no quick solution for this issue. We are investigating a
proper solution. An approach might be to implement a zypper plugin like it
exists for snapper, that is used for maintaining btrfs snapshots after zypper
operations.

Until then you need to resort to one of the workarounds I sketched in c#6.
Comment 11 Björn Voigt 2018-11-05 15:41:55 UTC
Another idea is to analyze how Etckeeper integrates with Zypper. Etckeeper can be configured so, that changes in /etc are automatically committed before Zypper updates packages or that Zypper stops with an error message if Etckeeper finds uncommitted changes.

We need something like this for starting Chkstat after Zypper was executed.

From /etc/etckeeper/etckeeper.conf:
# Uncomment to avoid etckeeper committing existing changes to 
# /etc before installation. It will cancel the installation,
# so you can commit the changes by hand.
#AVOID_COMMIT_BEFORE_INSTALL=1

# The high-level package manager that's being used.
# (apt, pacman, pacman-g2, yum, dnf, zypper, apk etc)
HIGHLEVEL_PACKAGE_MANAGER=zypper

# The low-level package manager that's being used.
# (dpkg, rpm, pacman, pacmatic, pacman-g2, apk etc)
LOWLEVEL_PACKAGE_MANAGER=rpm
Comment 12 Matthias Gerstner 2018-11-16 10:53:59 UTC
(In reply to bjoernv@arcor.de from comment #11)
> Another idea is to analyze how Etckeeper integrates with Zypper. Etckeeper
> can be configured so, that changes in /etc are automatically committed before
> Zypper updates packages or that Zypper stops with an error message if
> Etckeeper finds uncommitted changes.

etckeeper actually also uses a zypper plugin like snapper does. A minimal
plugin seems to be quite easy to implement. So I will look into it, how to
integrate it with the permissions package.
Comment 13 Swamp Workflow Management 2018-11-16 17:20:13 UTC
This is an autogenerated message for OBS integration:
This bug (1114383) was mentioned in
https://build.opensuse.org/request/show/649630 Factory / permissions
Comment 15 Matthias Gerstner 2018-11-28 11:42:59 UTC
Included in the current Tumbleweed snapshot dated 20181126, a new permissions
subpackage becomes available named 'permissions-zypp-plugin'. This is
currently not installed by default via a Recommends:, because I want it to be
tested a bit first.

Can you please try it out? It should suffice to install this plugin. Once the
plugin is present, after any zypper commit operation (this means
a complete 'zypper up' or 'zypper dup' process) is finished, 'chkstat' should
be called to reapply any custom permissions.

There is still a time window during which permissions can be wrong(namely
between an individual file being overwritten by a package update and until the
complete zypper commit operation is finished and chkstat is called). There can
be other such situations, however, when updating packages, and has to be
expected to some degree.
Comment 18 Michael Andres 2018-12-07 08:00:10 UTC
(In reply to Matthias Gerstner from comment #12)
> (In reply to bjoernv@arcor.de from comment #11)
> > Another idea is to analyze how Etckeeper integrates with Zypper. Etckeeper
> > can be configured so, that changes in /etc are automatically committed before
> > Zypper updates packages or that Zypper stops with an error message if
> > Etckeeper finds uncommitted changes.

This would indeed be interesting to know. 
(BTW it had to integrate with libzypp. Zypper is not executing the plugin).

1st because libzypp does not wait for commit plugins to complete. If they do not respond in time they simply get disconnected and commit proceeds.

2nd because a plugin is not able to stop the commit, unless it tries to forcefully kill it's parent process (which could be zypper,YAST,PK,..).
Comment 19 Björn Voigt 2018-12-11 09:29:01 UTC
The plugin permissions-zypp-plugin seems to work for me. Before I installed the plugin, "chkstat --system --set" showed changes after each "zypper dup" run. Now "chkstat --system --set" shows nothing after "zypper dup".
Comment 20 Matthias Gerstner 2018-12-11 11:12:21 UTC
Thank you for reporting back. I want to wait a bit longer and also test
myself, if nothing unexpected comes up when this plugin is installed. If all
goes well I will add a Recommends to this becomes the default for most
Tumbleweed users.
Comment 21 Michael Andres 2018-12-11 12:08:30 UTC
(In reply to Björn Voigt from comment #19)
> The plugin permissions-zypp-plugin seems to work for me.

I'm solely concerned about:
> Etckeeper can be configured so, ... that Zypper stops with an 
> error message if Etckeeper finds uncommitted changes.

Because libzypp plugins AFAIK offer no way to stop the pending commit (except killing their parent).
Comment 22 Björn Voigt 2018-12-11 12:15:29 UTC
(In reply to Michael Andres from comment #21)
> Because libzypp plugins AFAIK offer no way to stop the pending commit
> (except killing their parent).

But why chkstat should stop Zypper in case of an error? I think, chkstat is only useful after all packages are installed or upgraded with Zypper. If chkstat gets an error, then Zypper can do and should to nothing to solve this.
Comment 23 Michael Andres 2018-12-11 12:26:01 UTC
> Etckeeper can be configured so, ... that Zypper stops with an 
> error message if Etckeeper finds uncommitted change

I just cited what you wrote in comment #11 (also remembering https://github.com/openSUSE/libzypp/issues/137). If this behavior does not apply to etckeeper, I'm fine with it.
Comment 24 Björn Voigt 2018-12-11 12:36:29 UTC
(In reply to Michael Andres from comment #23)
> > Etckeeper can be configured so, ... that Zypper stops with an 
> > error message if Etckeeper finds uncommitted change

But this bug here is no Etckeeper bug.
Comment 25 Matthias Gerstner 2018-12-11 12:42:52 UTC
(In reply to ma@suse.com from comment #23)
> > Etckeeper can be configured so, ... that Zypper stops with an 
> > error message if Etckeeper finds uncommitted change
> 
> I just cited what you wrote in comment #11 (also remembering
> https://github.com/openSUSE/libzypp/issues/137). If this behavior does not
> apply to etckeeper, I'm fine with it.

The etckeeper zypper plugin is very simple [1]. It don't know if they expect
zypper to abort, or whether they just want to print errors. If you are
concerned about it you can split off a bug for etckeeper to be handled
seperately. Thank you!

[1]: https://git.joeyh.name/index.cgi/etckeeper.git/tree/zypper-etckeeper.py
Comment 26 Matthias Gerstner 2019-08-29 12:36:12 UTC
I think this bug is resolved for a while now. Closing.