Dear Maintainer,

Something was removing the world-readable mode of /etc/polkit-1/rules.d/
on my system (repeatedly), which meant that an un-privileged git process
was not able to check for changes until I fixed it.

It took me a while to track this down.  I thought it was not the polkitd
package, because its maintainer scripts specifically skip the "chmod" on
the directory if a dpkg-statoverride entry exists, as does on my system.

But it seems that the /usr/lib/tmpfiles.d/polkit-tmpfiles.conf file from
this package is to blame; it does not respect the statoverride settings.
I don't see why it is necessary, and the relevant changelog entry has no
explanation.  I also don't consider it appropriate to modify a permanent
administrator-controlled directory via the temporary-file mechanism; the
directory could be created by a maintainer script if necessary, and then
left alone.

If there is a good reason for this, the tmpfiles.d files should at least
be mentioned in polkitd.postinst to avoid frustration (dpkg-divert could
then be used to get rid of it).

- Michael

Am 14.11.24 um 19:04 schrieb Michael Gold:

The way to override package provided tmpfiles snippets is to ship one
under the same name in /etc/tmpfiles.d, i.e.
/etc/tmpfiles.d/polkit-tmpfiles.conf in your case.

There you can easily specify the desired mode.


Michael

Am 14.11.24 um 19:17 schrieb Michael Biebl:

Simon, do you think we should drop /etc/polkit-1/rules.d from
polkitd.dirs and rely on the tmpfiles snippet only?

I don't know. I don't have a sense of how close we are to project
consensus on the extent to which we as a project are using the
cross-distro tmpfiles.d, and its intended interactions with the
Debian-specific dpkg-statoverride.

I was a little surprised that a "d" line in tmpfiles.d alters the
permissions and ownership of a pre-existing directory, but I see that's
behaving as documented. In principle it would be possible to use ":0750"
instead of "0750", which would only set the permissions if the directory
doesn't already exist - but then, if the upstream- or distro-provided
permissions *changed*, that change would not be reflected on existing
systems (unless applied redundantly by imperative code in the maintainer
script), and that's maybe bad.

    smcv

Paraphrasing your orginal report to step back from the specific request
a little:

The reasoning for /etc/polkit-1/rules.d/ not being world-readable is that
it sets security policy, and knowing the precise local security policy
that has been configured would make it easier for a malicious or compromised
local process to make use of any loopholes that might exist in it.
If the policy is not readable, then a malicious or compromised process
might still be able to reverse-engineer it by trying various operations and
seeing which ones are allowed, but it would be more difficult for an
attacker to do that "quietly" without causing unexpected UI popups or
leaving traces in logs.

If you have a specific user or group that needs to be able to run git
against /etc, it might be better to set a POSIX ACL on /etc/polkit-1/rules.d,
like perhaps one of these (untested!):

    setfacl -m group:staff:r-x /etc/polkit-1/rules.d
    setfacl -m user:michael:r-x /etc/polkit-1/rules.d

which I believe neither dpkg nor systemd-tmpfiles will interfere with.

The big advantage of the increasingly mis-named tmpfiles.d is that it's
declarative, unlike maintainer scripts, which are imperative code that
can in principle do absolutely anything, and as a result is difficult
to analyze or reason about.

At the moment polkitd needs the maintainer-script code *and* the
tmpfiles.d fragment, because we can't rely on the tmpfiles.d fragment
taking effect on non-systemd systems unless something adds a dependency
on an implementation of the systemd-tmpfiles virtual package, which has
been controversial and has some non-trivial corner cases to consider
(see #945269).

    smcv

Michael, thanks for the work-around.

That was the first thing I tried, long ago; the access control lists get
deleted.

Out of curiosity, how do we tell whether it's mis-named or is just being
increasingly mis-used?

I like the idea of Debian packages being more declarative, but it seems
like something that ought to be handled by dpkg, which could maybe do a
better job of respecting areas such as /etc that are meant to be mostly
under administrator control (for example, when I've changed a file, I'm
prompted about whether to replace it with a new version).

A similar problem came up recently in Debian bug #1074076.  The file(s)
can't be initially installed with proper ownership, because the owner is
a dynamic user and may not exist yet; so the postinst script needed to
run "adduser" and "chown", but it'd be nicer to do both declaratively.
(It was also noted that checking dpkg-statoverride is a hack.)

I think all that matters for your stated goal is that the directory is
not world-accessible when created.  Even that doesn't really matter till
an administrator starts to change settings there; the installed list of
packages, and their default rules, are publically available.

	d
		Create a directory. The mode and ownership will be adjusted
		if specified.

It's not clear whether the second sentence applies if the documented
operation "fails".  Does someone want to split this off as a separate
bug report?  Something like this might be more clear:
	"Create a directory if it does not exist yet."
	(mirroring the wording used for most other types)
	"Set the mode and ownership, if specified."

The "Type Modifiers" and "Mode" sections do add some clarity, but only
after the reader may have gotten the wrong idea.

On Thu, 14 Nov 2024 19:47:02 +0000 Simon McVittie <smcv@debian.org> wrote:
one
/etc/tmpfiles.d/polkit-
polkitd.dirs

That just means we can't ask *others* to do such things in their
packages, but there's no reason why we can't in ours, no? And I agree
it's time to drop .dirs, it makes no sense in this day and age to do
this manually in scripts.

Incidentally, we also have some leftovers handling of /var/lib/polkit-1
- I think that's no longer necessary as well, given Michael dropped
pkla support entirely, right?

In existing installations it might still be the home directory of the
polkitd user (we try to change it to /nonexistent, but we might not be
able to if there's some stray process running as polkitd), and we can't
`rm -r` it because other packages might still own files in there.

I don't think that necessarily blocks removing all of the leftover
handling of it, but it will need doing a bit carefully.

    smcv

Yeah removing might not be feasible, however we can at least stop
creating it, setting the user/groups, etc, right?

Am 15.11.24 um 00:29 schrieb Luca Boccassi:

I think it's safe (and probably a good idea) to drop
-        set_perms root polkitd 750 /var/lib/polkit-1
from polkitd.postinst.

I'm not so sure we can easily drop it from polkitd.dirs.
This would cause dpkg to attempt its removal on upgrades which might not
be a good idea if the polkitd system user, as Simon explained above,
could not successfully be updated to the new home directory.
That said, it's indeed a bit unclean that we still ship the old
directory in the package.

But we have code to change the old users homedir though, so it's a
fallback for a fallback for a fallback... we should just change it to
assert that the user is correctly configured after trying to change
it, and refuse to continue unless manual action is taken to repair it,
with an explicit error. That way we know that even in the corner case
of a corner case of a corner case, it's safe to drop.

On Fri, 15 Nov 2024 01:03:00 +0000 Luca Boccassi <bluca@debian.org> wrote:
wrote:
/var/lib/polkit-1
dropped
of the
not be
we can't
there.
leftover
might not
above,
it,

Here's the change, it errors out with a clear error when the user
homedir cannot be fixed automatically, tested by mangling it manually
and it seems to work as intended:

https://salsa.debian.org/utopia-team/polkit/-/merge_requests/15