jsauthority: add 'system_unit' subject attribute - [merged]

[vmihalko]

In GitLab by @bluca on Jan 1, 2023, 16:27

_Merges check_unitpid -> master

When building with libsystemd support, query the systemd unit name that the process if part of (if any) and add it as a subject attribute. Allows allow-listing actions based on the systemd unit:

 polkit.addRule(function(action, subject) {
     if (action.id.indexOf("org.foo.bar") == 0) {
         if (subject.system_unit == "test.service") {
             return polkit.Result.YES;
         }
     }
 });

We call it system_unit instead of just unit to make it extra clear that this is about system units, rather than user units. If we ran as root we could also query for the user unit, but we are running as the polkitd user in most cases which means we cannot connect to other D-Bus sessions to perform the query.

[vmihalko]

In GitLab by @bluca on Jan 19, 2023, 03:09

added 5 commits

• 0fed3518...7550b683 - 2 commits from branch polkit:master
• 8ed955f1 - systemd: set User/Group and don't change uid/gid if already set
• a2cbd3be - polkitd: use PIDFDs if available, when possible
• 2c2d0d36 - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Jan 19, 2023, 03:09

marked this merge request as draft

[vmihalko]

In GitLab by @poettering on Jan 19, 2023, 17:43

Commented on src/polkitbackend/polkitbackendcommon.c line 556

i am pretty sure you want to use the existing gdbus connection, no?

[vmihalko]

In GitLab by @poettering on Jan 19, 2023, 17:47

Commented on src/polkit/polkitsystembusname.c line 394

this is a security issue. the pidfd might end up referring to an entirely different process than originally sent the message, because the pid got recycled.

it's a trivially exploitable security issue, to authenticate things via PID because of this. You'd need SCM_PIDFD to lock this down.

[vmihalko]

In GitLab by @poettering on Jan 19, 2023, 17:49

Commented on src/polkit/polkitsystembusname.c line 394

(it might be OK to validate that the pidfd after you got it to ensure it matches the sender's uid. if so, it's not too bad to allow this race to happen, since it will mean that the pid recycle security issue can only be exploited by the original user, which limits the attack surface a bit. but not sure how much that is worth in a world where userns mappings are commonplace. In particular as people might assume you actually authenticate by cgroup, but what you actually do is authenticate by the uid, just via some baroque way to reference it)

[vmihalko]

In GitLab by @bluca on Jan 19, 2023, 17:50

Commented on src/polkit/polkitsystembusname.c line 394

I know, there's a full set of things needed. This moves further along the way, and doesn't make anything worse than it already was. It's one piece of the puzzle

[vmihalko]

In GitLab by @bluca on Jan 20, 2023, 02:42

added 2 commits

• 23bf0b75 - polkitd: use PIDFDs if available, when possible
• a3ef2c60 - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Jan 20, 2023, 17:56

Commented on src/polkitbackend/polkitbackendcommon.c line 556

sd-bus is so much nicer... - I leave that to @jrybar to decide whether he wants this to use gdbus or not

[vmihalko]

In GitLab by @bluca on Mar 22, 2023, 04:27

Commented on src/polkit/polkitsystembusname.c line 394

changed this line in version 4 of the diff

[vmihalko]

In GitLab by @bluca on Mar 22, 2023, 04:27

added 17 commits

• a3ef2c60...b7c9c0ce - 14 commits from branch polkit:master
• 64fc2960 - systemd: set User/Group and don't change uid/gid if already set
• 0485ed9a - polkitd: use PIDFDs if available to track processes
• 7a18b762 - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 22, 2023, 13:53

Commented on src/polkit/polkitsystembusname.c line 394

This is now implemented depending on the new SO_PEERPIDFD https://lore.kernel.org/netdev/20230321183342.617114-1-aleksandr.mikhalitsyn@canonical.com/ and GetCredentials() https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/398

[vmihalko]

In GitLab by @bluca on Mar 22, 2023, 13:53

mentioned in merge request dbus/dbus!398

[vmihalko]

In GitLab by @bluca on Mar 25, 2023, 03:55

Commented on src/polkitbackend/polkitbackendcommon.c line 556

changed this line in version 5 of the diff

[vmihalko]

In GitLab by @bluca on Mar 25, 2023, 03:55

added 2 commits

• 38e82f5d - polkitd: use PIDFDs if available to track processes
• 258d4f88 - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 25, 2023, 03:55

Commented on src/polkitbackend/polkitbackendcommon.c line 556

switched to gdbus

[vmihalko]

In GitLab by @bluca on Mar 25, 2023, 03:55

resolved all threads

[vmihalko]

In GitLab by @bluca on Mar 25, 2023, 04:20

added 2 commits

• ab5979c8 - polkitd: use PIDFDs if available to track processes
• 39039762 - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 25, 2023, 04:25

added 1 commit

• 3751faca - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 25, 2023, 04:27

added 2 commits

• 4f1087c6 - polkitd: use PIDFDs if available to track processes
• 46d98c73 - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 27, 2023, 24:56

added 2 commits

• dbfa54be - polkitd: use PIDFDs if available to track processes
• 69acdfbf - jsauthority: add 'system_unit' subject attribute

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 27, 2023, 01:11

added 1 commit

• a756356a - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 27, 2023, 01:14

added 2 commits

• 1e7ad3ae - polkitd: use PIDFDs if available to track processes
• d04829ce - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Mar 27, 2023, 12:42

added 1 commit

• 788ccc84 - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Apr 5, 2023, 24:30

added 4 commits

• b6169f31 - 1 commit from branch polkit:master
• d3bb5d48 - systemd: set User/Group and don't change uid/gid if already set
• f5939a4d - polkitd: use PIDFDs if available to track processes
• 397ba17b - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Apr 5, 2023, 03:45

added 1 commit

• 1f26318d - jsauthority: parse groups from GetConnectionCredentials() if available

Compare with previous version

[vmihalko]

In GitLab by @bluca on Apr 5, 2023, 13:00

added 1 commit

• b6b6d60e - jsauthority: parse groups from GetConnectionCredentials() if available

Compare with previous version

[vmihalko]

In GitLab by @bluca on Apr 5, 2023, 14:37

added 4 commits

• 41023354 - 1 commit from branch polkit:master
• 853ea504 - polkitd: use PIDFDs if available to track processes
• 7694432e - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes
• 024b0dab - jsauthority: parse groups from GetConnectionCredentials() if available

Compare with previous version

[vmihalko]

In GitLab by @smcv on May 15, 2023, 21:25

Commented on src/polkit/polkitsystembusname.c line 513

(dbus hat on)

Please don't make authoritative statements like this about changes that haven't been merged. It's entirely possible that 1.15.6 will be released without this feature.

[vmihalko]

In GitLab by @bluca on May 15, 2023, 21:29

Commented on src/polkit/polkitsystembusname.c line 513

I did not intent to un-draft this until the corresponding dbus-daemon PR was merged. I'll update to a TODO instead.

[vmihalko]

In GitLab by @smcv on May 15, 2023, 22:19

Commented on src/polkit/polkitsystembusname.c line 513

1.15.TODO would make sense :-)

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 16:00

resolved all threads

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 16:00

added 4 commits

• 7b3c9c85 - 1 commit from branch polkit:master
• 19f2ee39 - polkitd: use PIDFDs if available to track processes
• 9ddbd5ec - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes
• d5bfae8d - jsauthority: parse groups from GetConnectionCredentials() if available

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 16:01

marked this merge request as ready

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 16:01

Commented on src/polkit/polkitsystembusname.c line 513

changed this line in version 18 of the diff

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 16:01

added 21 commits

• d5bfae8d...7f0d7923 - 19 commits from branch polkit:master
• 8cabb118 - jsauthority: parse groups from GetConnectionCredentials() if available
• 1232d897 - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 17, 2023, 16:06

added 1 commit

• a23d9ce3 - jsauthority: add 'system_unit' and 'no_new_privileges' subject attributes

Compare with previous version

[vmihalko]

In GitLab by @bluca on Aug 28, 2023, 18:34

@jrybar gentle ping :-) kernel 6.5 is out with the new API, so we can add this feature now. I've also split out the first commit, which is a bugfix, in a separate MR if that makes it easier to review: https://gitlab.freedesktop.org/polkit/polkit/-/merge_requests/189

[vmihalko]

In GitLab by @jrybar on Sep 1, 2023, 20:57

requested review from @jrybar

[vmihalko]

In GitLab by @bluca on Sep 8, 2023, 18:32

@jrybar thank you! I'll be doing a lighting talk about this workstream at All Systems Go next week in case you are interested: https://cfp.all-systems-go.io/all-systems-go-2023/talk/T3LJAM/

[vmihalko]

In GitLab by @jrybar on Sep 11, 2023, 12:29

Eager to watch, thanks! :smile:

BTW, I've just found out it somehow breaks polkit-pkla-compat on Fedora. I'll investigate it, but more likely just another nail in the coffin of the package (or making it a weak dependency at least).