Redesign Xauth handling #1230

Vogtinator · 2020-02-10T15:02:28Z

This commit moves Xauthority handling over to libXau.
Advantage is that this allows use of FamilyWild, is faster, more reliable
and easier to read. However, we lose the ability to merge the new cookie into
an existing Xauthority file, so support for using a non-temporary file is
dropped. Even if merging was implemented manually, use of FamilyWild would
"infect" such a file and break it for DMs which don't write it.

tpgxyz · 2020-02-25T15:25:55Z

I've applied this PR to OpenMandrivaAssociation/sddm@5384de2

I'm running sddm on OpenMandriva with this PR, found no issues.

davidedmundson

Generally fine, one minor comment/question

The process env part was a bit confusing, but makes sense after IRC explanation.

davidedmundson · 2020-02-25T16:13:52Z

src/helper/UserSession.cpp

-            FILE *fp = popen(qPrintable(cmd), "w");
+        QString dir = QStandardPaths::writableLocation(QStandardPaths::RuntimeLocation);
+        if (!dir.isEmpty()) {
+            m_xauthFile.setFileTemplate(dir + QStringLiteral("/xauth_XXXXXX"));


This is somewhat weird.

setupChildProcess is after we've forked, we have the same memory contents, but it's not shared. It's COW.

Does it work as intended and actually clean up?

I suspect it would seem to work because:

the m_xauthFile of the parent that's actually scoped to the process stays effectively uninitialised

the child sets up m_xauthFile but in it's cloned memory, we would then effectively leak the temporary file as we don't call the destructor when we exec()

Good catch - I totally forgot to mention that.

It leaks the temporary file currently, so relies on /run getting cleaned up by something else.

Not sure how to improve that though, the deletion would need to happen in the user context as well to avoid any arbitrary deletion vulnerabilities.

There's also #1083 which needs a way to clean up stuff after a user session ends.

What about redesigning sddm-helper away from setupChildProcess? It could just do the setup in the main process itself (like setuid) and then use QProcess as usual. This means that sddm-helper would effectively run as the target user at that point and proper cleanup can be performed.

It would not help with #1083 though as that's managed by sddm, not sddm-helper IIRC.

This is technically possible, but currently sddm-helper performs utmp and PAM session handling after the user process quites, so that would have to be moved into the daemon first...

I tested how this behaves on FreeBSD and there the session setup is done as part of Xsession: https://github.com/freebsd/freebsd-ports-kde/blob/d70b3c9023dfe8414ea91c31881209d16c15b229/x11/sddm/files/patch-data_scripts_Xsession#L18
This also includes setting up XDG_RUNTIME_DIR, which means that it's not available inside sddm-helper at all.
While using the fallback directory there is technically fine, it definitely needs manual cleanup then.

data/man/sddm.conf.rst.in

src/helper/UserSession.cpp

src/common/XauthUtils.cpp

src/daemon/XorgDisplayServer.cpp

Vogtinator · 2020-04-04T18:13:54Z

plfiorini approved these changes 1 minute ago

Note that there are currently open topics, like the authority file leak and the manpage update.

still need some changes

Vogtinator · 2020-10-09T19:26:02Z

I changed the design somewhat. Instead of putting the temporary xauth_XXXXX file into the user's runtime dir, it now ends up in sddm's RUNTIME_DIR and gets its ownership changed to the target user. This means that cleanup by sddm-helper is possible now and it should also work on FreeBSD.

Future sddm version will use $XDG_RUNTIME_DIR/xauth_XXXXXX References: - https://bugzilla.opensuse.org/show_bug.cgi?id=1174290 - https://bugzilla.suse.com/show_bug.cgi?id=1174293 - sddm/sddm#1230 - jonls/redshift#763 This is the 2.13 version of 35f033c / https://gitlab.com/apparmor/apparmor/-/merge_requests/581 The difference is that this commit avoids using the @{run} variable.

Future sddm version will use $XDG_RUNTIME_DIR/xauth_XXXXXX References: * https://bugzilla.opensuse.org/show_bug.cgi?id=1174290 * https://bugzilla.suse.com/show_bug.cgi?id=1174293 * sddm/sddm#1230 * jonls/redshift#763 This is the 2.13 version of 35f033c / https://gitlab.com/apparmor/apparmor/-/merge_requests/581 The difference is that this commit avoids using the @{run} variable. MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/582 Acked-by: John Johansen <john.johansen@canonical.com>

Future sddm version will use $XDG_RUNTIME_DIR/xauth_XXXXXX References: - https://bugzilla.opensuse.org/show_bug.cgi?id=1174290 - https://bugzilla.suse.com/show_bug.cgi?id=1174293 - sddm/sddm#1230 - jonls/redshift#763 This is the 2.13 version of 35f033c / https://gitlab.com/apparmor/apparmor/-/merge_requests/582 The difference is that this commit avoids using the @{run} variable. (cherry picked from commit 02b9090)

Future sddm version will use $XDG_RUNTIME_DIR/xauth_XXXXXX References: - https://bugzilla.opensuse.org/show_bug.cgi?id=1174290 - https://bugzilla.suse.com/show_bug.cgi?id=1174293 - sddm/sddm#1230 - jonls/redshift#763 This is the 2.13 version of 35f033c / https://gitlab.com/apparmor/apparmor/-/merge_requests/582 The difference is that this commit avoids using the @{run} variable. (cherry picked from commit 02b9090) Signed-off-by: John Johansen <john.johansen@canonical.com>

tvolin · 2021-01-31T23:03:25Z

Placing file xauth_XXXXX has bad side-effects. The user cannot write to the containing directory, so xauth (the program) cannot create the lockfile RUNTIME_DIR/xauth_XXXXX-c
(it appends -c to the original filename and tries to create the lock file)
This makes all executions of xauth take 20 seconds to time out (and set RC=1). This is most painful whenever you ssh with X forwarding enabled.
See downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1922772

I changed the design somewhat. Instead of putting the temporary xauth_XXXXX file into the user's runtime dir, it now ends up in sddm's RUNTIME_DIR and gets its ownership changed to the target user. This means that cleanup by sddm-helper is possible now and it should also work on FreeBSD.

tpgxyz · 2021-02-02T08:26:50Z

@tvolin make sure you have set proper permissions on /run/sddm
see here OpenMandrivaAssociation/sddm@3cf5678

Vogtinator · 2021-02-02T08:30:04Z

Placing file xauth_XXXXX has bad side-effects. The user cannot write to the containing directory, so xauth (the program) cannot create the lockfile RUNTIME_DIR/xauth_XXXXX-c
(it appends -c to the original filename and tries to create the lock file)
This makes all executions of xauth take 20 seconds to time out (and set RC=1). This is most painful whenever you ssh with X forwarding enabled.
See downstream bug https://bugzilla.redhat.com/show_bug.cgi?id=1922772

Great, back to the drawing board. Apparently this approach doesn't work either then.

Vogtinator · 2021-02-02T08:33:07Z

@tvolin make sure you have set proper permissions on /run/sddm
see here OpenMandrivaAssociation/sddm@3cf5678

O.o

You realize that way you're giving EVERY USER on the system FULL ACCESS to your sessions and the login screen, which means they can just run xev and grab your passwords?

tpgxyz · 2021-02-02T11:04:46Z

You realize that way you're giving EVERY USER on the system FULL ACCESS to your sessions and the login screen, which means they can just run xev and grab your passwords?

oops, thanks for spotting this ⭕

Vogtinator · 2021-02-02T11:16:12Z

Actually, I missed that the directory has the sticky bit set - in that case it should actually be fine (but no guarantee).

There are other issues with this approach though, for instance that there's still no explicit cleanup in case of leftover files (sddm crash, power outage, etc.).

tvolin · 2021-02-02T13:09:01Z

Permissions 1733 seem to do the job. Non-root users can't even see the name of each other's xauth files. And with 0600 as the file permission (which it is), they cannot read each other's files either.
Cleanup of /var/run on Fedora is a non-issue since it's on tmpfs. Not sure about other distros.
Bob Richmond proposed this solution for Fedora. Since Mandriva did something similar I'll suggest we fix this downstream.

Vogtinator · 2021-02-02T13:12:17Z

Permissions 1733 seem to do the job. Non-root users can't even see the name of each other's xauth files. And with 0600 as the file permission (which it is), they cannot read each other's files either.
Cleanup of /var/run on Fedora is a non-issue since it's on tmpfs. Not sure about other distros.

I'm wondering mostly about FreeBSD actually.

Bob Richmond proposed this solution for Fedora. Since Mandriva did something similar I'll suggest we fix this downstream.

Vogtinator · 2021-04-20T06:43:48Z

@Vogtinator Can you rebase this on the current mainline to make it mergeable?

Well, I did yesterday but just shortly afterwards major changes got merged...

I don't currently have time to get familiar with them and redo this PR.

tpgxyz · 2021-06-11T14:11:11Z

@Vogtinator can you please rebase it on top current head ?

Conan-Kudo · 2021-07-05T17:19:38Z

@Vogtinator @davidedmundson Can someone please look at rebasing this on top of current mainline? We need this and I couldn't figure out how to do it myself. 😢

@Vogtinator

This is a rebase of sddm#1230 by @Vogtinator > This commit moves Xauthority handling over to libXau. > Advantage is that this allows use of FamilyWild, is faster, more reliable > and easier to read. However, we lose the ability to merge the new cookie into > an existing Xauthority file, so support for using a non-temporary file is > dropped. Even if merging was implemented manually, use of FamilyWild would > "infect" such a file and break it for DMs which don't write it.

@Vogtinator

This is a rebase of sddm#1230 by @Vogtinator > This commit moves Xauthority handling over to libXau. > Advantage is that this allows use of FamilyWild, is faster, more reliable > and easier to read. However, we lose the ability to merge the new cookie into > an existing Xauthority file, so support for using a non-temporary file is > dropped. Even if merging was implemented manually, use of FamilyWild would > "infect" such a file and break it for DMs which don't write it.

By moving this in display we don't need sddm-helper to perform anything as root after the initial setup. This allows us to drop the priveleges in UserSession before forking. This should unblock sddm#1230 as well as give us a path to simplify the paths where the helper is running the display manager.

@Vogtinator

This is a rebase of sddm#1230 by @Vogtinator > This commit moves Xauthority handling over to libXau. > Advantage is that this allows use of FamilyWild, is faster, more reliable > and easier to read. However, we lose the ability to merge the new cookie into > an existing Xauthority file, so support for using a non-temporary file is > dropped. Even if merging was implemented manually, use of FamilyWild would > "infect" such a file and break it for DMs which don't write it.

@Vogtinator

This is a rebase of sddm#1230 by @Vogtinator > This commit moves Xauthority handling over to libXau. > Advantage is that this allows use of FamilyWild, is faster, more reliable > and easier to read. However, we lose the ability to merge the new cookie into > an existing Xauthority file, so support for using a non-temporary file is > dropped. Even if merging was implemented manually, use of FamilyWild would > "infect" such a file and break it for DMs which don't write it.

Vogtinator · 2023-02-04T20:30:31Z

Tested to work as expected with x11, x11-user and wayland. For x11, XAUTHORITY is set to /tmp/xauth_rAnDoM, for x11-user to /run/user/1337/xauth_rAnDoM.

davidedmundson

Looks good. Thanks

davidedmundson · 2023-02-07T15:54:37Z

src/common/XAuth.cpp

-    // Check file
-    if (!fp)
+    if (XauWriteAuth(authFp, &auth) == 0) {
+        fclose(authFp);


I would suggest using QScopeGuard rather than having to fClose in all the paths

This commit moves Xauthority handling over to libXau. Advantage is that this allows use of FamilyWild, is faster, more reliable and easier to read. However, we lose the ability to merge the new cookie into an existing Xauthority file, so support for using a non-temporary file is dropped. Even if merging was implemented manually, use of FamilyWild would "infect" such a file and break it for DMs which don't write it. Co-authored-by: Matt Jolly <Matt.Jolly@footclan.ninja>

Use proper temporary files instead of UUIDs. This makes the file's purpose more obvious and also takes care of cleanup.

By moving this in display we don't need sddm-helper to perform anything as root after the initial setup. This allows us to drop the priveleges in UserSession before forking. This should unblock sddm#1230 as well as give us a path to simplify the paths where the helper is running the display manager.

Starting from sddm 0.20 (by sddm#1230), the xauth file is at `/tmp/xauth_*` instead of `~/.Xauthority`. The new location is subject to systemd-tmpfiles's cleanup, which deletes files from /tmp/ that are older than 10 days: https://github.com/systemd/systemd/blob/v254/tmpfiles.d/tmp.conf The xauth file should not be removed while the session is active, because it is needed for starting X applications. When removed, X applications won't start any more. This patch fixes the issue by declaring these files as "ignored" in sddm-tmpfiles.conf, so that systemd-tmpfiles doesn't remove them. Minimal test case: ``` touch /tmp/xauth_testonly SYSTEMD_LOG_LEVEL=debug systemd-tmpfiles --prefix=/tmp/ --clean ``` Pre-patch output: File "/tmp/xauth_testonly": change time Mon 2023-10-02 01:27:38.395466 CEST is too new. (meaning that the file was considered for removal) Post-patch output: Ignoring "/tmp/xauth_testonly": a separate glob exists. (meaning that the file is ignored and not considered for removal)

Starting from sddm 0.20 (by #1230), the xauth file is at `/tmp/xauth_*` instead of `~/.Xauthority`. The new location is subject to systemd-tmpfiles's cleanup, which deletes files from /tmp/ that are older than 10 days: https://github.com/systemd/systemd/blob/v254/tmpfiles.d/tmp.conf The xauth file should not be removed while the session is active, because it is needed for starting X applications. When removed, X applications won't start any more. This patch fixes the issue by declaring these files as "ignored" in sddm-tmpfiles.conf, so that systemd-tmpfiles doesn't remove them. Minimal test case: ``` touch /tmp/xauth_testonly SYSTEMD_LOG_LEVEL=debug systemd-tmpfiles --prefix=/tmp/ --clean ``` Pre-patch output: File "/tmp/xauth_testonly": change time Mon 2023-10-02 01:27:38.395466 CEST is too new. (meaning that the file was considered for removal) Post-patch output: Ignoring "/tmp/xauth_testonly": a separate glob exists. (meaning that the file is ignored and not considered for removal)

Vogtinator mentioned this pull request Feb 10, 2020

Use libXau instead of xauth #863

Closed

plfiorini added this to the 0.19 milestone Feb 10, 2020

davidedmundson reviewed Feb 25, 2020

View reviewed changes

plfiorini requested changes Feb 25, 2020

View reviewed changes

data/man/sddm.conf.rst.in Show resolved Hide resolved

src/helper/UserSession.cpp Outdated Show resolved Hide resolved

src/common/XauthUtils.cpp Outdated Show resolved Hide resolved

src/daemon/XorgDisplayServer.cpp Outdated Show resolved Hide resolved

Vogtinator force-pushed the libxau branch from 0a22759 to 7236fb5 Compare February 25, 2020 17:47

plfiorini previously approved these changes Apr 4, 2020

View reviewed changes

plfiorini self-requested a review April 4, 2020 18:23

Vogtinator force-pushed the libxau branch from d760275 to 46d7a53 Compare October 9, 2020 19:19

plfiorini modified the milestones: 0.19, 0.20 Nov 3, 2020

Vogtinator force-pushed the libxau branch from 46d7a53 to 592ad41 Compare November 3, 2020 21:31

This was referenced Jan 25, 2021

Xauthority issues (on SDDM?): Can't open display: 0 due to toolbox hostname containers/toolbox#679

Closed

toolbox: init at 0.0.99 NixOS/nixpkgs#110473

Closed

Vogtinator marked this pull request as draft February 2, 2021 08:34

Kangie mentioned this pull request Jun 19, 2022

Redesign Xauth handling #1562

Closed

Vogtinator force-pushed the libxau branch 3 times, most recently from bb0a3a5 to 6e012fb Compare February 4, 2023 20:20

Vogtinator marked this pull request as ready for review February 4, 2023 20:28

Vogtinator force-pushed the libxau branch from 6e012fb to 09252a8 Compare February 4, 2023 20:59

Vogtinator mentioned this pull request Feb 6, 2023

Add config files for systemd-tmpfiles and systemd-sysusers #1659

Merged

davidedmundson approved these changes Feb 7, 2023

View reviewed changes

Vogtinator and others added 2 commits February 7, 2023 18:54

Use QTemporaryFile with xauth_XXXXXX ih XAuth

7f20e41

Use proper temporary files instead of UUIDs. This makes the file's purpose more obvious and also takes care of cleanup.

Vogtinator force-pushed the libxau branch from 09252a8 to 7f20e41 Compare February 7, 2023 18:00

Vogtinator merged commit eb4a7d5 into sddm:develop Feb 8, 2023

Vogtinator deleted the libxau branch February 8, 2023 07:12

Vogtinator mentioned this pull request Feb 9, 2023

User switching breaks #1660

Closed

Vogtinator mentioned this pull request Jun 22, 2023

update changelog for 0.20 #1738

Merged

Rob--W mentioned this pull request Oct 2, 2023

Prevent tmpfiles from removing "old" xauth files #1805

Merged

sigprof mentioned this pull request Dec 11, 2023

nixos/ssh: pass XAUTHORITY to ssh-askpass NixOS/nixpkgs#273537

Merged

13 tasks

Redesign Xauth handling #1230

Redesign Xauth handling #1230

Uh oh!

Conversation

Vogtinator commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tpgxyz commented Feb 25, 2020

Uh oh!

davidedmundson left a comment

Choose a reason for hiding this comment

Uh oh!

davidedmundson Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

Vogtinator Feb 25, 2020

Choose a reason for hiding this comment

Uh oh!

Vogtinator Jun 27, 2020

Choose a reason for hiding this comment

Uh oh!

Vogtinator Jul 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Vogtinator commented Apr 4, 2020

Uh oh!

Vogtinator commented Oct 9, 2020

Uh oh!

tvolin commented Jan 31, 2021

Uh oh!

tpgxyz commented Feb 2, 2021

Uh oh!

Vogtinator commented Feb 2, 2021

Uh oh!

Vogtinator commented Feb 2, 2021

Uh oh!

tpgxyz commented Feb 2, 2021

Uh oh!

Vogtinator commented Feb 2, 2021

Uh oh!

tvolin commented Feb 2, 2021

Uh oh!

Vogtinator commented Feb 2, 2021

Uh oh!

Vogtinator commented Apr 20, 2021

Uh oh!

tpgxyz commented Jun 11, 2021

Uh oh!

Conan-Kudo commented Jul 5, 2021

Uh oh!

Vogtinator commented Feb 4, 2023

Uh oh!

davidedmundson left a comment

Choose a reason for hiding this comment

Uh oh!

davidedmundson Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Vogtinator Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Vogtinator commented Feb 10, 2020 •

edited

Loading