Skip to content

Conversation

@buhtz
Copy link
Member

@buhtz buhtz commented Sep 16, 2025

...in progress...

The systray icon do use two technics to determine the name of that user owning/running the current desktop session. First loginctl (systemd) is used. If not not present who and DISPLAY are used.

What is still missing: Disabling context menu entries if needed.

Fix #2237

@buhtz buhtz marked this pull request as draft September 16, 2025 08:06
@buhtz buhtz self-assigned this Sep 16, 2025
@buhtz buhtz added the PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. label Sep 16, 2025
@buhtz buhtz added this to the 1.6.0 (upcoming next) milestone Sep 16, 2025
@buhtz buhtz added the Security security and vulnerability related issues label Sep 16, 2025
@samo-sk
Copy link

samo-sk commented Sep 16, 2025

I am not sure if this is correct. Regarding _get_desktop_user_via_loginctl, the active session is not necessarily the session that corresponds to DISPLAY; this is the case if there are multiple sessions running (i.e. multiple X sessions, or a virtual terminal session and an X session).

As for _get_desktop_user_via_x11_who, this does not work if the X server is not launched by a login program (such as sddm), e.g. if the server was launched from shell using startx, or if the server is XWayland.


Also, what puzzles me about the fallback display :0.0 is that DISPLAY is not sufficient to launch an X client; XAUTHORITY is also needed. Some programs seem to use ~/.Xauthority if XAUTHORITY is not set. However, if I inspect a systray-icon process (root profile, schedule "Every 5 minutes"), it does have DISPLAY and XAUTHORITY envvars set. I don't know what sets them, but this could mean that the hardcoded fallback may actually never be used. It may be worth investigating what actually sets these envvars.

@samo-sk
Copy link

samo-sk commented Sep 16, 2025

Update: ~/.local/share/backintime/cron_env seems to set DISPLAY and XAUTHORITY variables for scheduled jobs.

Weirdly, this file is only written from Password_Cache.run(), and the only relevant place it is invoked from (during normal operation) is Mount.__init__() (and this happens only when snapshots.<Mode>.password.use_cache is true in the config). This is particularly weird, because none of the envvars saved are directly related to mounting.

(This is true for the dev branch; I haven't investigated the main branch.)

@buhtz
Copy link
Member Author

buhtz commented Sep 16, 2025

Puh... This gives me a headache. It is far beyond my skills and expertise. 😿

@samo-sk
Copy link

samo-sk commented Sep 16, 2025

loginctl list-sessions should be able to determine the display of an X login session.(1) Instead of checking if a session is active (as the draft code does now), I would check if the session's Display= from loginctl equals the DISPLAY environment variable.

Maybe it would be easier to only handle ordinary X login sessions. This way, the mechanism using who can stay. Users who use XWayland or run X from a virtual terminal would need to authenticate always, even on their own jobs.

(Actually, the icon doesn't reliably work for me on XWayland on KDE Plasma, because XAUTHORITY changes on every login. That is another reason why I wouldn't try to support XWayland right now.)

@samo-sk
Copy link

samo-sk commented Sep 17, 2025

I wrote:

Instead of checking if a session is active (as the draft code does now), I would check if the session's Display= from loginctl equals the DISPLAY environment variable.

This is how I imagine my version of _get_desktop_user_via_loginctl:

    def _get_desktop_user_via_loginctl(self) -> str | None:
        """Get name of user logged in to the current desktop session using
        loginctl.
        """

        try:
            # get list of sessions
            output = subprocess.check_output(
                ['loginctl', 'list-sessions', '--no-legend', '--json=short'],
                text=True)
        except FileNotFoundError:
            logging.warning('Can not determine user name of current desktop '
                            'session because "loginctl" is not available.')
            return None
        except Exception as exc:
            logging.error('Unexpected error while determining user name of '
                          f'current desktop session: {exc}')
            return None

        # Check each session
        for session in json.loads(output):
            # Ignore none-user sessions
            if session.get('class') != 'user':
                continue

            # properties of the session
            info = subprocess.check_output(
                ['loginctl', 'show-session', str(session['session']),
                '--property=Display', '--property=Name', '--property=Seat'],
                text=True
            ).strip()

            props = dict(line.split('=') for line in info.splitlines())

            # Does the display match?
            if props.get('Display', '') == os.environ.get('DISPLAY', ':0').split('.')[0]:
                # if props['Seat'] == 'seat0':
                logger.info(f'VARIANT - systemd loginctl: {props=}', self)
                return props['Name']

        return None

Like I have mentioned, this doesn't work with XWayland or X manually started from a VT. I have tested this code only very superficially.

@buhtz buhtz added PR: Modifications requested Maintenance team requested modifications and waiting for their implementation and removed PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. labels Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Modifications requested Maintenance team requested modifications and waiting for their implementation Security security and vulnerability related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Systray icon, started by scheduled root-mode or foreign-user backup profiles via cron, allows privilege escalation

2 participants