pam: Add pam_xdg module

[manu]

Summary

This is a module to setup the XDG directories and environment variables.
For now the only usage is to have a XDG_RUNTIME_DIR environment setup at
user login.
All other environment variable have a default fallback so no need to export
them in this module.
The directory is created according to the XDG Base directory specification.

The default base directory is /var/run/xdg/<username> but can be configured
using the runtime_dir=<dir> module option.

According to the spec the directory *must* not survive a reboot so adding
var_run_enable="YES" to rc.conf is highly recommanded.

Sponsored by: Beckhoff Automation GmbH & Co. KG

[des]

lib/libpam/modules/pam_xdg/pam_xdg.8

2

Please add SPDX tags as per https://docs.freebsd.org/en/articles/license-guide/

40

.Pa

lib/libpam/modules/pam_xdg/pam_xdg.c

29

not needed

64

please use openpam_get_option(3)

74

space

110

where is this freed?

183

please use openpam_get_option(3)

193

space

244

runtime_dir_prefix is not freed

264

You don't need these dummy methods, just #define PAM_SM_SESSION before including the PAM headers.

[manu]

Address some of des@ comments.

[manu]

lib/libpam/modules/pam_xdg/pam_xdg.8

40

.Pa where ?

lib/libpam/modules/pam_xdg/pam_xdg.c

74

Space where ?

193

Space where ?

[manu]

Add a few comments and close a few fd

[des]

lib/libpam/modules/pam_xdg/pam_xdg.8

42

lib/libpam/modules/pam_xdg/pam_xdg.c

72

196

[manu]

Address latest des@ comments.

[des]

lib/libpam/modules/pam_xdg/pam_xdg.c

62

63

127

137

139

186

187

223

230

249

251

[des]

I also just noticed that you never check if asprintf() failed.

lib/libpam/modules/pam_xdg/pam_xdg.c

104

Should we remove the directory we just created if fchownat() fails?

196

This is never used, maybe either drop it or use it to confirm that the directory belongs to the correct user before attempting to remove it?

[manu]

lib/libpam/modules/pam_xdg/pam_xdg.c

127

openat return the fd, and it will not be zero.

[manu]

lib/libpam/modules/pam_xdg/pam_xdg.c

62

Why ?

[pauamma_gundo.com]

lib/libpam/modules/pam_xdg/pam_xdg.8

39

[manu]

setup -> sets up

[des]

lib/libpam/modules/pam_xdg/pam_xdg.c

62

Because 0 is a valid file descriptor.

127

It can be zero if stdin somehow gets closed, we've had CVEs in the past for things like this.

[manu]

Address des@ comments.

[des]

lib/libpam/modules/pam_xdg/pam_xdg.c

196

This is never used, maybe either drop it or use it to confirm that the directory belongs to the correct user before attempting to remove it?

you haven't addressed this

[des]

I'm not sure the logic in _pam_xdg_close() is correct. You seem to always close the last session, right? So if I initiate session A and then initiate session B and then log out of session A it will remove the user directory that was created for, and is potentially being used by, session B, which is still active, while leaving the user directory that was used by session A and is now unused in place.

[manu]

In D44011#1004814, @des wrote:

I'm not sure the logic in _pam_xdg_close() is correct. You seem to always close the last session, right? So if I initiate session A and then initiate session B and then log out of session A it will remove the user directory that was created for, and is potentially being used by, session B, which is still active, while leaving the user directory that was used by session A and is now unused in place.

No because the sessions aren't tied to a real session, this is just a count.
So :

• user login -> xdg_session.0 is created
• user login again -> xdg_session.1 is created
• 1st session close -> xdg_session.1 is deleted
• 2nd session close -> xdg_session.0 is delete and the user session directory.

lib/libpam/modules/pam_xdg/pam_xdg.c

196

Phab is so bad that I have no idea to which line this comment correspond.

[manu]

Check already existing directory for ownership and mode.

[manu]

also check directory ownership and mode at close

[manu]

style(9)

[des]

lib/libpam/modules/pam_xdg/pam_xdg.c

113

fstatat() could fail (consider for instance a race between two sessions for the same user closing at the exact same time because their network connection got dropped)

[pauamma_gundo.com]

Manual page LGTM.

[manu]

check fstatat return value

[manu]

check other fstatat

[jbeich]

Is the module name clash with sysutils/pam_xdg intentional? It creates a POLA violation on upgrade: /var/run/user/<uid> moves to /var/run/xdg/<uname>.

[arrowd]

Sorry for being late to the party, but what's the point in

1. Rewriting it, since we have sysutils/pam_xdg?
2. Having it in base instead of port?

[manu]

In D44011#1006994, @jbeich wrote:

Is the module name clash with sysutils/pam_xdg intentional? It creates a POLA violation on upgrade: /var/run/user/<uid> moves to /var/run/xdg/<uname>.

It's not really POLA on the directory as it's solved by a reboot, one cannot say the same thing about modules argument.

[manu]

In D44011#1007029, @arrowd wrote:

Sorry for being late to the party, but what's the point in

1. Rewriting it, since we have sysutils/pam_xdg?
2. Having it in base instead of port?

Because I believe that any user should be able to install FreeBSD, pkg install sway drm-kmod, kldload <kmod> and start sway without having anything else to do.
On the rewrite while it's true that we could have import it to base, I find the code really hard to read with all the defines and wanted something simpler.

[arrowd]

pkg install sway drm-kmod

At this point, the drm-kmod package could pull in the pam_xdg package. Until we move to pkgbase, the base updates are slow. An user usually have to wait for a new release to get the updated piece of base.

I find the code really hard to read with all the defines and wanted something simpler.

Maybe it could've been solved upstream.

Anyways, it is already committed, so let's see how it works out.