Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pam_xauth: use the owner of Xauthority file #556

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

pam_xauth: use the owner of Xauthority file #556

wants to merge 1 commit into from

Conversation

keentux
Copy link
Contributor

@keentux keentux commented Apr 4, 2023

  • Get the owner of the users Xauthority file done by state on the cookiefile. /usr/bin/xauth chokes on the old user's $HOME being on an NFS file system. Run /usr/bin/xauth using the old user's uid/gid.
  • In a case where $Home, and ~/.Xauthority file was in NFS server, pam_xauth module should read the users Xauthority file as user because with NFS share used for user HOME root is not allowed to read the user's Xauthority file even with the xauth program, and it runs into a timeout (20 seconds).

Patch courtesy of Dr. Werner Fink.

ldv-alt
ldv-alt requested changes Apr 4, 2023
View reviewed changes
Copy link
Member

@ldv-alt ldv-alt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a few unrelated changes crammed into a single PR for no particular reason,
and only one of these changes is mentioned in the commit message.

modules/pam_xauth/pam_xauth.c Outdated Show resolved Hide resolved
modules/pam_xauth/pam_xauth.c Outdated Show resolved Hide resolved
modules/pam_xauth/pam_xauth.c Outdated Show resolved Hide resolved
modules/pam_xauth/pam_xauth.c Outdated Show resolved Hide resolved
modules/pam_xauth/pam_xauth.c Outdated Show resolved Hide resolved
modules/pam_xauth/pam_xauth.c Outdated Show resolved Hide resolved
@keentux keentux marked this pull request as draft April 5, 2023 07:52
@keentux keentux changed the title pam_xauth: use the owner of Xauthority file on NFS pam_xauth: use the owner of Xauthority file Apr 6, 2023
@keentux
Copy link
Contributor Author

keentux commented Apr 6, 2023

Thank you @ldv-alt for all the reviews. Indeed, I didn't well format and explained all changes, my apologies.

So, I split them in two, adding a more concrete description for each.

  • First commit: As it was done for module pam_unix and pam_wheel, it would be good to get the uid/gid of the requesting user (so the logged in user). As it has this importance to get the proper authority file if XAUTHENV is not set. Otherwise, using pam_modutil_getpwuid(pamh, getuid()) we get root user uid as it runs by PAM.
  • Second commit:
    • As explained in the description, it would be better to stat the cookiefile (the authority file) getting this ownership to run the xauth command. For xauth, it based this workflow on the information in this file. Moreover, it can properly run even if the user's HOME directory is on NFS shared.
    • Also, it could be a good idea to add the -i argument to the xauth command, to ignore any authority file locks. It could occurs with NFS server also where the authority file can be locked by xdm for example.

Hopefully this commit's description explained well all changes.
I am happy to get feedback and gather other information you need.

@keentux keentux marked this pull request as ready for review April 6, 2023 15:22
@keentux keentux requested a review from ldv-alt April 6, 2023 15:23
@keentux
pam_xauth: use login user instead of effective one
4026d42 
* As it was done for pam_wheel, check if the command will be run by the
  effective user, and then get the UID of the remote host name RHOST,
  or the remote user name RUSER instead. For some programs, getuid()
  could provides the root ownership if the effective user is set before,
  and therefore could not have access to the cookiefile. The module
  could become extremelly slow as xauth hence it runs into a timeout if
  it cannot read Xauthority.
* Add the `-i` argument to xauth to ignore lock on authority file.
  Useful when using a NFS, where program as xdm could locked it.

Signed-off-by: Valentin Lefebvre <valentin.lefebvre@suse.com>
@keentux
Copy link
Contributor Author

keentux commented Apr 11, 2023

This request was created according to the bug reported here: #272, and as said on previous comment, it is also related to this one: #22.
However, after some research, sudo project get a patch two years ago that fixed both bugs, see: https://www.sudo.ws/repos/sudo/rev/2c6fef0107c8.
So, this request is no more useful for PAM project. Even if, FMHO, it always good to check effective user and get the username as it was done for pam_wheel module, before running xauth.

@ldv-alt it is up to you to decline the request, I defere to your decision.

@keentux keentux requested a review from ldv-alt April 17, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldv-alt ldv-alt Awaiting requested review from ldv-alt

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@keentux @ldv-alt