Bug 258664 - sysutils/seatd: add pkg-message
Summary: sysutils/seatd: add pkg-message
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Jan Beich
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-21 14:37 UTC by Ghost
Modified: 2021-09-24 20:57 UTC (History)
1 user (show)

See Also:
jbeich: maintainer-feedback+


Attachments
v1 (use "git am") (1.48 KB, patch)
2021-09-21 14:37 UTC, Ghost
no flags Details | Diff
v1.1 (2.62 KB, patch)
2021-09-22 18:26 UTC, Ghost
no flags Details | Diff
v1.2 (1.87 KB, patch)
2021-09-22 18:29 UTC, Ghost
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ghost 2021-09-21 14:37:25 UTC
Created attachment 228098 [details]
v1 (use "git am")

Add post-install pkg-message on how to use seatd.
Comment 1 Ghost 2021-09-22 18:26:51 UTC
Created attachment 228126 [details]
v1.1

Needs PORTREVISION bump like sysutils/pam_xdg.
Comment 2 Ghost 2021-09-22 18:29:00 UTC
Created attachment 228127 [details]
v1.2

Don't include local unrelated change.
Comment 3 commit-hook freebsd_committer freebsd_triage 2021-09-24 17:23:00 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=0958855771675c0baba0262b7e5d4266dc9fe2df

commit 0958855771675c0baba0262b7e5d4266dc9fe2df
Author:     Jan Beich <jbeich@FreeBSD.org>
AuthorDate: 2021-09-24 15:04:54 +0000
Commit:     Jan Beich <jbeich@FreeBSD.org>
CommitDate: 2021-09-24 17:22:01 +0000

    sysutils/seatd: document common usage

    In wlroots affects WLR_BACKENDS=drm,libinput (default on console).
    Weston also supports libseat but hasn't been tested.

    PR:             258664
    Submitted by:   Evgeniy Khramtsov <evgeniy@khramtsov.org> (based on)

 sysutils/seatd/Makefile                   |  3 ++
 sysutils/seatd/files/pkg-message.in (new) | 47 +++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)
Comment 4 Jan Beich freebsd_committer freebsd_triage 2021-09-24 17:23:48 UTC
Landed after rewording a bit.
Comment 5 Ghost 2021-09-24 17:42:54 UTC
(In reply to Jan Beich from comment #4)

Thanks.

> a bit

To be fair, "based on" could be omitted. This is a whole different pkg-message, it is way more advanced. I thought that pkg-message should be brief, but this is much better.

Is there a reason to omit "seatd-launch(1) drops {G,U}ID" ? I checked the
source of seatd-launch, and it calls setgid() and setuid():
https://github.com/kennylevinsen/seatd/blob/0f20175752b7/seatd-launch/seatd-launch.c#L141

rg 'setgid' work/seatd-0.6.2 results in one result in seatd-launch/seatd-launch.c
Comment 6 Jan Beich freebsd_committer freebsd_triage 2021-09-24 18:26:00 UTC
(In reply to Evgeniy Khramtsov from comment #5)
> Is there a reason to omit "seatd-launch(1) drops {G,U}ID" ?

- setgid bit isn't used in FreeBSD port
- dedicated seatd instance has EUID == 0
- seatd-launch(1) manpage explains it all better
Comment 7 Jan Beich freebsd_committer freebsd_triage 2021-09-24 18:39:48 UTC
Besides, seatd-launch if run as root keeps root for the target command. river/phoc/gamescope don't barf if run as root unlike sway/hikari/wayfire/cage/labwc. And running as root can be useful for debugging (bypass DRM_AUTH), recovery (single user mode) and in some embedded scenarios (root shell instead of sudo/doas).
Comment 8 Ghost 2021-09-24 19:10:27 UTC
(In reply to Jan Beich from comment #6)

> - setgid bit isn't used in FreeBSD port

No. I checked:

--- seatd-launch/seatd-launch.c.orig    2021-09-15 23:07:42 UTC
+++ seatd-launch/seatd-launch.c
@@ -8,6 +8,7 @@
 #include <string.h>
 #include <sys/stat.h>
 #include <sys/wait.h>
+#include <sys/types.h>
 #include <unistd.h>

 int main(int argc, char *argv[]) {
@@ -138,6 +139,15 @@ int main(int argc, char *argv[]) {
                goto error_seatd;
        }

+       static gid_t rgid, egid, sgid;
+       static uid_t ruid, euid, suid;
+       getresgid(&rgid, &egid, &sgid);
+       getresuid(&ruid, &euid, &suid);
+       fprintf(stderr, "RGID = %d EGID = %d SGID = %d""\n",
+               (int)rgid, (int)egid, (int)sgid);
+       fprintf(stderr, "RUID = %d EUID = %d SUID = %d""\n",
+               (int)ruid, (int)euid, (int)suid);
+
        // Drop privileges
        if (setgid(gid) == -1) {
                perror("Could not set gid to drop privileges");
@@ -147,6 +157,13 @@ int main(int argc, char *argv[]) {
                perror("Could not set uid to drop privileges");
                goto error_seatd;
        }
+
+       getresgid(&rgid, &egid, &sgid);
+       getresuid(&ruid, &euid, &suid);
+       fprintf(stderr, "RGID = %d EGID = %d SGID = %d""\n",
+               (int)rgid, (int)egid, (int)sgid);
+       fprintf(stderr, "RUID = %d EUID = %d SUID = %d""\n",
+               (int)ruid, (int)euid, (int)suid);

        pid_t child = fork();
        if (child == -1) {

stderr:
RGID = 1001 EGID = 1001 SGID = 1001
RUID = 1001 EUID = 0 SUID = 0
RGID = 1001 EGID = 1001 SGID = 1001
RUID = 1001 EUID = 1001 SUID = 1001
[...]

> - dedicated seatd instance has EUID == 0

1. seatd-launch indeed drops privileges
2. my version mentioned this:

[...]
seatd-launch(1) dedicated server instance:
[...]
seatd-launch(1) drops {G,U}ID of instance from
root to user's, compared to daemon launched via rc(8).
[...]

Can't you also check yourself via this patch and top(1)?
I know what I was talking about and cared to check before submitting the PR.

> - seatd-launch(1) manpage explains it all better

A casual user is not going to read manpages and will just follow the pkg-message (if they are lucky to even have the attention span to spot and read it.)

seatd via rc(8) doesn't drop privileges, I think this should be mentioned.
Comment 9 Ghost 2021-09-24 19:15:41 UTC
(In reply to Evgeniy Khramtsov from comment #8)

> drops {G,U}ID

maybe UID, in this case there is no GID to drop
Comment 10 Jan Beich freebsd_committer freebsd_triage 2021-09-24 19:51:40 UTC
(In reply to Evgeniy Khramtsov from comment #8)
> 1. seatd-launch indeed drops privileges

seatd-launch starts a dedicated seatd instance as root then drops priveleges to run the target command but your pkg-message claimed seatd was running as regular user which is false.

$ ps axd -Ouid,gid
 PID  UID  GID TT  STAT      TIME COMMAND
...
1501    0 1234 v1  Is     0:00,00 `-- login [pam] (login)
1502 1234 1234 v1  S      0:00,10   `-- -zsh (zsh)
1949 1234 1234 v1  S+     0:00,00     `-- seatd-launch cage -s foot sh
1950    0 1234 v1  S+     0:00,00       |-- seatd -n 4 -s /tmp/seatd.1949.sock
1951 1234 1234 v1  S+     0:00,17       `-- cage -s foot sh
1952 1234 1234 v1  S+     0:00,07         `-- foot sh
1953 1234 1234  6  Ss+    0:00,00           `-- -sh (sh)
...
Comment 11 Ghost 2021-09-24 20:57:29 UTC
(In reply to Jan Beich from comment #10)

# ps axd -O euid,ruid | grep seatd
   40 1001 1001 v0  I+      0:00.00 |   `-- seatd-launch dbus-run-session sway
  678    0 1001 v0  I+      0:00.00 |     |-- seatd -n 4 -s /tmp/seatd.40.sock

# ps axd -O egid,rgid | grep seatd
   40 1001 1001 v0  I+      0:00.00 |   `-- seatd-launch dbus-run-session sway
  678 1001 1001 v0  I+      0:00.00 |     |-- seatd -n 4 -s /tmp/seatd.40.sock

seatd-launch before privilege drop forks child that execve() seatd, and only then the parent process calls setuid(). I assumed that because the child process inherits its credentials from its parent it is also affected by setuid(), and top(1) also showed different user.

EUID in your output is "uid". top(1) shows RUID instead, as it seems. Sorry for the noise.