diff mbox series

pam_cifscreds, tmux and session keyrings

Message ID 774233f766bf26976c0d923cc1dc53c7@polymtl.ca (mailing list archive)
State New, archived
Headers show
Series pam_cifscreds, tmux and session keyrings | expand

Commit Message

Nick Guenther July 21, 2022, 8:45 p.m. UTC
I have set up pam_cifscreds and multiuser samba mounts for my users, which seemed to work well, until some of my users started complaining that their overnight batch jobs were failing.

The common feature is running their jobs in tmux.

It seems tmux sometimes has access to the keyring that pam_cifscreds uses, but isn't able to put a lock on it. So, only overnight when no one is looking (which is of course the worst time to debug), when mount.cifs's connection drops, it isn't able to reconnect because it has no password available.

I would like to have your wisdom on this situation. Can I configure pam_cifscreds to set a longer expiry on the credentials? Can I put something in tmux's startup script to make it lock in the session keyring? Is this a bug in pam_cifscreds?



Here's the setup:

$ cat /etc/pam.d/sshd
[...]
# Create a new session keyring.
session    optional     pam_keyinit.so force revoke
[...]

$ cat /etc/pam.d/common-session
[...]
session [default=1]                     pam_permit.so
session requisite                       pam_deny.so
session required                        pam_permit.so
session optional                        pam_umask.so
session required        pam_unix.so 
session optional                        pam_sss.so 
session optional    pam_cifscreds.so host=duke.neuro.polymtl.ca
session optional        pam_systemd.so 
session optional                        pam_mkhomedir.so umask=0077


$ cat /etc/fstab
[...]
//duke.neuro.polymtl.ca/projects /mnt/duke/projects cifs vers=3.0,nobrl,file_mode=0644,multiuser,credentials=/etc/activedirectory-credentials,nofail,_netdev 0 0
//duke.neuro.polymtl.ca/temp /mnt/duke/temp cifs vers=3.0,nobrl,file_mode=0644,multiuser,credentials=/etc/polygrames-credentials,nofail,_netdev 0 0



Here's a demo:

If someone sshes in, pam_cifscreds stashes their password in the keyring

p115628@davis:~$ keyctl list @s
2 keys in keyring:
452396344: --alswrv 703204575 65534 keyring: _uid.703204575
626459666: ----sw-v     0     0 logon: cifs:a:132.207.65.200

and it forwards their password to the CIFS server so they can access its contents:
 
p115628@davis:~$ ls -l /mnt/duke/projects/pmj/pmj_csa/results.txt 
-rw-r--r-- 1 p115628 domain users 831 Jul  5  2021 /mnt/duke/projects/pmj/pmj_csa/results.txt


Both commands work inside of a tmux session as well.

However if I make a tmux session, _detach_ it, and then log out

p115628@davis:~$ keyctl list @s
2 keys in keyring:
452396344: --alswrv 703204575 65534 keyring: _uid.703204575
293860075: ----sw-v     0     0 logon: cifs:a:132.207.65.200
p115628@davis:~$ tmux
[detached (from session 0)]
p115628@davis:~$ 
logout
Shared connection to davis.neuro.polymtl.ca closed.


and then **wait** for the CIFS connection to time out (or, plan to access the CIFS share that is not yet open) and then back in again:

[kousu@laptop ]$ ssh davis
p115628@davis:~$ tmux attach
p115628@davis:~$ keyctl list @s
keyctl_read_alloc: Key has been revoked
p115628@davis:~$ ls -l /mnt/duke/temp 
ls: cannot access '/mnt/duke/temp': Permission denied
^Bd

But outside of tmux, the connection works, because it has access to the reinitialized keyring:

[detached (from session 0)]
p115628@davis:~$ keyctl list @s
2 keys in keyring:
452396344: --alswrv 703204575 65534 keyring: _uid.703204575
462100230: ----sw-v     0     0 logon: cifs:a:132.207.65.200
p115628@davis:~$ ls -l /mnt/duke/temp 
total 0
drwxr-xr-x 2 p115628 domain users 0 Jun  7 18:10 user1
drwxr-xr-x 2 p115628 domain users 0 Jun 20 18:14 user2


which then makes the share temporarily available inside of tmux as well, at least until it times out again.



Lennart Poettering and co. have the same problem, because `systemd --user` sessions also lose access to the pam_cifscreds keyring: https://github.com/systemd/systemd/issues/1299. They think it's your bug. They offered a fix too, but I want to hear your opinion on it: use user-keyring(7) instead of user-session-keyring(7), or:




I see in this old thread https://www.spinics.net/lists/linux-cifs/msg18249.html that you actually want to go the _other_ direction, and isolate your sessions even more:

> > multiuser SMB connections should also be initiated per session, same like the
> > keyring. Currently the cifs SMB connections are accessible also from other all
> > sessions.
>
> That needs to be implemented indeed.

but that doesn't sound like it would make my users happy. In their perspective, tmux should be the same environment as ssh or as the GUI, just more persistent. And I tend to agree.

Anyway, I hope this isn't too intricate or confusing for you. I would really appreciate a second opinion, and maybe a consideration of that patch, if that patch is actually the right answer.

-Nick Guenther
Associé de Recherche
NeuroPoly, Polytechnique de Montréal

Comments

Mantas Mikulėnas July 22, 2022, 6:38 p.m. UTC | #1
On 2022-07-21 23:45, Nick Guenther wrote:
> [...]
> I see in this old thread https://www.spinics.net/lists/linux-cifs/msg18249.html that you actually want to go the _other_ direction, and isolate your sessions even more:
> 
>>> multiuser SMB connections should also be initiated per session, same like the
>>> keyring. Currently the cifs SMB connections are accessible also from other all
>>> sessions.
>>
>> That needs to be implemented indeed.
> 
> but that doesn't sound like it would make my users happy. In their perspective, tmux should be the same environment as ssh or as the GUI, just more persistent. And I tend to agree.
> 
> Anyway, I hope this isn't too intricate or confusing for you. I would really appreciate a second opinion, and maybe a consideration of that patch, if that patch is actually the right answer.

As another user, I'd expect the keyring search to be done recursively -- 
start from the session keyring as now, but follow the link into the user 
keyring, which is usually present (and isn't that its whole purpose?)

Then pam_cifscreds could be told which one to insert keys to, allowing 
it to be used both ways depending on needs -- just like how Kerberos or 
AFS can also have either isolated credentials or user-wide ones.
Nick Guenther July 29, 2022, 2:47 a.m. UTC | #2
July 22, 2022 2:38 PM, "Mantas Mikulėnas" <grawity@gmail.com> wrote:

> On 2022-07-21 23:45, Nick Guenther wrote:
> 
>> [...]
>> I see in this old thread https://www.spinics.net/lists/linux-cifs/msg18249.html that you actually
>> want to go the _other_ direction, and isolate your sessions even more:
>> multiuser SMB connections should also be initiated per session, same like the
>> keyring. Currently the cifs SMB connections are accessible also from other all
>> sessions.
>>> That needs to be implemented indeed.
>> 
>> but that doesn't sound like it would make my users happy. In their perspective, tmux should be the
>> same environment as ssh or as the GUI, just more persistent. And I tend to agree.
>> Anyway, I hope this isn't too intricate or confusing for you. I would really appreciate a second
>> opinion, and maybe a consideration of that patch, if that patch is actually the right answer.
> 
> As another user, I'd expect the keyring search to be done recursively -- start from the session
> keyring as now, but follow the link into the user keyring, which is usually present (and isn't that
> its whole purpose?)
> 
> Then pam_cifscreds could be told which one to insert keys to, allowing it to be used both ways
> depending on needs -- just like how Kerberos or AFS can also have either isolated credentials or
> user-wide ones.



I've figured out a workaround, but I'm unsure about it and I could really use some advice from people with more insight.


I said before that my servers (and yours too) have

# cat /etc/pam.d/sshd | grep keyinit
session    optional     pam_keyinit.so revoke force

And the problem shows up when I detach tmux, **log out**, log back in and reattach tmux; then I see


p115628@davis:~$ keyctl list @s
keyctl_read_alloc: Key has been revoked

The word 'revoked' was the obvious clue I: just to remove the 'revoke' option and the problem goes away:

# cat /etc/pam.d/sshd | grep keyinit
session    optional     pam_keyinit.so force


This keeps the session-keyring(7) working even after reattaching. Because it's the **log out** that is disabling the keyring; per pam_keyinit(8) [1]:

>   revoke
>           Causes the session keyring of the invoking process to be revoked when the invoking
>           process exits if the session keyring was created for this process in the first place.


This change seems to have solved the immediate complaints from my users. But I don't like overriding the default like this; I assume there's a series of good reasons for using 'revoke' that I don't understand.

Would there be interest in switching to KEY_SPEC_USER_KEYRING? Would it be a good idea? Can I assume the kernel CIFS code would need a matching change?


[1] https://manpages.ubuntu.com/manpages/focal/man8/pam_keyinit.8.html
Steve French Aug. 1, 2022, 4:10 a.m. UTC | #3
Adding Shyam to this discussion - interesting questions worth investigating.

On Thu, Jul 28, 2022 at 9:50 PM Nick Guenther <nick.guenther@polymtl.ca> wrote:
>
> July 22, 2022 2:38 PM, "Mantas Mikulėnas" <grawity@gmail.com> wrote:
>
> > On 2022-07-21 23:45, Nick Guenther wrote:
> >
> >> [...]
> >> I see in this old thread https://www.spinics.net/lists/linux-cifs/msg18249.html that you actually
> >> want to go the _other_ direction, and isolate your sessions even more:
> >> multiuser SMB connections should also be initiated per session, same like the
> >> keyring. Currently the cifs SMB connections are accessible also from other all
> >> sessions.
> >>> That needs to be implemented indeed.
> >>
> >> but that doesn't sound like it would make my users happy. In their perspective, tmux should be the
> >> same environment as ssh or as the GUI, just more persistent. And I tend to agree.
> >> Anyway, I hope this isn't too intricate or confusing for you. I would really appreciate a second
> >> opinion, and maybe a consideration of that patch, if that patch is actually the right answer.
> >
> > As another user, I'd expect the keyring search to be done recursively -- start from the session
> > keyring as now, but follow the link into the user keyring, which is usually present (and isn't that
> > its whole purpose?)
> >
> > Then pam_cifscreds could be told which one to insert keys to, allowing it to be used both ways
> > depending on needs -- just like how Kerberos or AFS can also have either isolated credentials or
> > user-wide ones.
>
>
>
> I've figured out a workaround, but I'm unsure about it and I could really use some advice from people with more insight.
>
>
> I said before that my servers (and yours too) have
>
> # cat /etc/pam.d/sshd | grep keyinit
> session    optional     pam_keyinit.so revoke force
>
> And the problem shows up when I detach tmux, **log out**, log back in and reattach tmux; then I see
>
>
> p115628@davis:~$ keyctl list @s
> keyctl_read_alloc: Key has been revoked
>
> The word 'revoked' was the obvious clue I: just to remove the 'revoke' option and the problem goes away:
>
> # cat /etc/pam.d/sshd | grep keyinit
> session    optional     pam_keyinit.so force
>
>
> This keeps the session-keyring(7) working even after reattaching. Because it's the **log out** that is disabling the keyring; per pam_keyinit(8) [1]:
>
> >   revoke
> >           Causes the session keyring of the invoking process to be revoked when the invoking
> >           process exits if the session keyring was created for this process in the first place.
>
>
> This change seems to have solved the immediate complaints from my users. But I don't like overriding the default like this; I assume there's a series of good reasons for using 'revoke' that I don't understand.
>
> Would there be interest in switching to KEY_SPEC_USER_KEYRING? Would it be a good idea? Can I assume the kernel CIFS code would need a matching change?
>
>
> [1] https://manpages.ubuntu.com/manpages/focal/man8/pam_keyinit.8.html
Nick Guenther Aug. 3, 2022, 8:29 p.m. UTC | #4
August 1, 2022 12:10 AM, "Steve French" <smfrench@gmail.com> wrote:

> Adding Shyam to this discussion - interesting questions worth investigating.
> 
> On Thu, Jul 28, 2022 at 9:50 PM Nick Guenther <nick.guenther@polymtl.ca> wrote:
>> I've figured out a workaround, but I'm unsure about it and I could really use some advice from
>> people with more insight.
>> 
>> I just to remove the 'revoke' option and the problem goes away:
>> 
>> # cat /etc/pam.d/sshd | grep keyinit
>> session optional pam_keyinit.so force
>> 
>> This keeps the session-keyring(7) working even after reattaching [to tmux]
>> 
>> Would there be interest in switching to KEY_SPEC_USER_KEYRING? Would it be a good idea? Can I
>> assume the kernel CIFS code would need a matching change?

I've written a patch that switches cifs-utils to KEY_SPEC_USER_KEYRING. It seems to behave -- `keylist list @u` shows the cifs key instead of `keyctl list @s` -- but it doesn't solve my problem without being combined with the 'revoke' workaround, because the kernel is still using KEY_SPEC_SESSION_KEYRING.

I'm sending it anyway to see if there's any interest. I've never touched kernel-adjacent code before, so honestly I'm kind of scared of this. Maybe it'd be better to solve this in tmux -- is there some way to make tmux pin the keyring? Copy the existing keyring it inherits to a new KEY_SPEC_SESSION_KEYRING when it starts? But if you agree with the systemd people (https://github.com/systemd/systemd/issues/1299#issuecomment-141937177), please consider this.

I've also never used `git send-email` or `git format-patch` before, so I don't know if this is the right ettiquette for attaching a patch. Hopefully you can be patient as I figure it out.


From c73603f67bc945708e997e7e9585fd76575542e2 Mon Sep 17 00:00:00 2001
From: Nick Guenther <nick.guenther@polymtl.ca>
Date: Thu, 28 Jul 2022 11:00:15 -0400
Subject: [PATCH] Switch pam_cifscreds to user-keyring(7).

This makes CIFS credentials persist even in detached
shells like screen(1) or tmux(1) that outlive the
original login shell that spawned them.

However, the CIFS code in the kernel needs a patch
to match this change for this to be any use.
---
cifscreds.c | 39 +------
cifskey.h | 2 +-
pam_cifscreds.c | 290 +++++++++++++++++++++++++++++++-----------------
3 files changed, 194 insertions(+), 137 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index 32f2ee4..afea136 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -279,7 +279,7 @@ static int cifscreds_clear(struct cmdarg *arg)

/*
* search for same credentials stashed for current host
- * and unlink them from session keyring
+ * and unlink them from keyring
*/
currentaddress = addrstr;
nextaddress = strchr(currentaddress, ',');
@@ -326,7 +326,7 @@ static int cifscreds_clearall(struct cmdarg *arg __attribute__ ((unused)))
int count = 0, errors = 0;

/*
- * search for all program's credentials stashed in session keyring
+ * search for all program's credentials stashed in keyring
* and then unlink them
*/
do {
@@ -386,7 +386,7 @@ static int cifscreds_update(struct cmdarg *arg)
return EXIT_FAILURE;
}

- /* search for necessary credentials stashed in session keyring */
+ /* search for necessary credentials stashed in keyring */
currentaddress = addrstr;
nextaddress = strchr(currentaddress, ',');
if (nextaddress)
@@ -428,36 +428,6 @@ static int cifscreds_update(struct cmdarg *arg)
return EXIT_SUCCESS;
}

-static int
-check_session_keyring(void)
-{
- key_serial_t ses_key, uses_key;
-
- ses_key = keyctl_get_keyring_ID(KEY_SPEC_SESSION_KEYRING, 0);
- if (ses_key == -1) {
- if (errno == ENOKEY)
- fprintf(stderr, "Error: you have no session keyring. "
- "Consider using pam_keyinit to "
- "install one.\n");
- else
- fprintf(stderr, "Error: unable to query session "
- "keyring: %s\n", strerror(errno));
- return (int)ses_key;
- }
-
- /* A problem querying the user-session keyring isn't fatal. */
- uses_key = keyctl_get_keyring_ID(KEY_SPEC_USER_SESSION_KEYRING, 0);
- if (uses_key == -1)
- return 0;
-
- if (ses_key == uses_key)
- fprintf(stderr, "Warning: you have no persistent session "
- "keyring. cifscreds keys will not persist "
- "after this process exits. See "
- "pam_keyinit(8).\n");
- return 0;
-}
-
int main(int argc, char **argv)
{
struct command *cmd, *best;
@@ -531,8 +501,5 @@ int main(int argc, char **argv)
if (arg.user == NULL)
arg.user = getusername(getuid());

- if (check_session_keyring())
- return EXIT_FAILURE;
-
return best->action(&arg);
}
diff --git a/cifskey.h b/cifskey.h
index ed0c469..8217e58 100644
--- a/cifskey.h
+++ b/cifskey.h
@@ -36,7 +36,7 @@
#define DOMAIN_DISALLOWED_CHARS "\\/:*?\"<>|"

/* destination keyring */
-#define DEST_KEYRING KEY_SPEC_SESSION_KEYRING
+#define DEST_KEYRING KEY_SPEC_USER_KEYRING
#define CIFS_KEY_TYPE "logon"
#define CIFS_KEY_PERMS (KEY_POS_VIEW|KEY_POS_WRITE|KEY_POS_SEARCH| \
KEY_USR_VIEW|KEY_USR_WRITE|KEY_USR_SEARCH)
diff --git a/pam_cifscreds.c b/pam_cifscreds.c
index 5d99c2d..ba3aea9 100644
--- a/pam_cifscreds.c
+++ b/pam_cifscreds.c
@@ -25,13 +25,16 @@
#include <errno.h>
#include <stdio.h>
#include <stdlib.h>
+#include <stdbool.h>
#include <string.h>
#include <syslog.h>
#include <sys/types.h>
-/*
-#include <signal.h>
#include <unistd.h>
#include <sys/wait.h>
+#include <pwd.h>
+
+/*
+#include <signal.h>
*/

#include <keyutils.h>
@@ -192,72 +195,126 @@ static int cifscreds_pam_add(pam_handle_t *ph, const char *user, const char
*pas
return PAM_SERVICE_ERR;
}

- /* search for same credentials stashed for current host */
- currentaddress = addrstr;
- nextaddress = strchr(currentaddress, ',');
- if (nextaddress)
- *nextaddress++ = '\0';

- while (currentaddress) {
- if (key_search(currentaddress, keytype) > 0) {
- pam_syslog(ph, LOG_WARNING, "You already have stashed credentials "
- "for %s (%s)", currentaddress, hostdomain);
+ // Impersonate target user.
+ //
+ // PAM runs as root, so if DEST_KEYRING == KEY_SPEC_USER_KEYRING or
+ // KEY_SPEC_USER_SESSION_KEYRING, the credentials would be cached by root,
+ // uselessly. This isn't needed for DEST_KEYRING == KEY_SPEC_SESSION_KEYRING
+ // which gets inherited by the child process tree of the ultimate login shell.
+ //
+ // The main work has to be done in a subprocess, because only the real UID has rights to edit
that UID's keyring
+ // so we have to call setuid(), but we can't un-setuid() later, yet we still need to be root for
the other PAM modules to behave.
+ errno = 0;
+ struct passwd *pwnam = getpwnam(user);
+ if(pwnam == NULL) {
+ pam_syslog(ph, LOG_ERR, "getpwnam(): %s", strerror(errno));
+ return PAM_SERVICE_ERR;
+ }

- return PAM_SERVICE_ERR;
- }
+ pid_t keyring_process = fork();
+ if(keyring_process == -1) {
+ pam_syslog(ph, LOG_ERR, "fork(): %s", strerror(errno));
+ return PAM_SERVICE_ERR;
+ } else if(keyring_process == 0) {
+ // child process

- switch(errno) {
- case ENOKEY:
- /* success */
- break;
- default:
- pam_syslog(ph, LOG_ERR, "Unable to search keyring for %s (%s)",
- currentaddress, strerror(errno));
- return PAM_SERVICE_ERR;
+ if(setgid(pwnam->pw_gid) < 0) {
+ pam_syslog(ph, LOG_ERR, "setegid(): %s", strerror(errno));
+ exit(PAM_SERVICE_ERR);
}
-
- currentaddress = nextaddress;
- if (currentaddress) {
- *(currentaddress - 1) = ',';
- nextaddress = strchr(currentaddress, ',');
- if (nextaddress)
- *nextaddress++ = '\0';
+ if(setuid(pwnam->pw_uid) < 0) {
+ pam_syslog(ph, LOG_ERR, "seteuid(): %s", strerror(errno));
+ exit(PAM_SERVICE_ERR);
}
- }

- /* Set the password */
- currentaddress = addrstr;
- nextaddress = strchr(currentaddress, ',');
- if (nextaddress)
- *nextaddress++ = '\0';
-
- while (currentaddress) {
- key_serial_t key = key_add(currentaddress, user, password, keytype);
- if (key <= 0) {
- pam_syslog(ph, LOG_ERR, "error: Add credential key for %s: %s",
- currentaddress, strerror(errno));
- } else {
- if ((args & ARG_DEBUG) == ARG_DEBUG) {
- pam_syslog(ph, LOG_DEBUG, "credential key for \\\\%s\\%s added",
- currentaddress, user);
+ /* search for same credentials stashed for current host */
+ currentaddress = addrstr;
+ nextaddress = strchr(currentaddress, ',');
+ if (nextaddress)
+ *nextaddress++ = '\0';
+
+ while (currentaddress) {
+ if (key_search(currentaddress, keytype) > 0) {
+ pam_syslog(ph, LOG_WARNING, "You already have stashed credentials "
+ "for %s (%s)", currentaddress, hostdomain);
+
+ exit(PAM_SERVICE_ERR);
+ }
+
+ switch(errno) {
+ case ENOKEY:
+ /* success */
+ break;
+ default:
+ pam_syslog(ph, LOG_ERR, "Unable to search keyring for %s (%s)",
+ currentaddress, strerror(errno));
+ exit(PAM_SERVICE_ERR);
+ }
+
+ currentaddress = nextaddress;
+ if (currentaddress) {
+ *(currentaddress - 1) = ',';
+ nextaddress = strchr(currentaddress, ',');
+ if (nextaddress)
+ *nextaddress++ = '\0';
}
- if (keyctl(KEYCTL_SETPERM, key, CIFS_KEY_PERMS) < 0) {
- pam_syslog(ph, LOG_ERR,"error: Setting permissons "
- "on key, attempt to delete...");
-
- if (keyctl(KEYCTL_UNLINK, key, DEST_KEYRING) < 0) {
- pam_syslog(ph, LOG_ERR, "error: Deleting key from "
- "keyring for %s (%s)",
- currentaddress, hostdomain);
+ }
+
+ /* Set the password */
+ currentaddress = addrstr;
+ nextaddress = strchr(currentaddress, ',');
+ if (nextaddress)
+ *nextaddress++ = '\0';
+
+ while (currentaddress) {
+ key_serial_t key = key_add(currentaddress, user, password, keytype);
+ if (key <= 0) {
+ pam_syslog(ph, LOG_ERR, "error: Add credential key for %s: %s",
+ currentaddress, strerror(errno));
+ } else {
+ if ((args & ARG_DEBUG) == ARG_DEBUG) {
+ pam_syslog(ph, LOG_DEBUG, "credential key for \\\\%s\\%s added",
+ currentaddress, user);
+ }
+ if (keyctl(KEYCTL_SETPERM, key, CIFS_KEY_PERMS) < 0) {
+ pam_syslog(ph, LOG_ERR,"error: Setting permissons "
+ "on key, attempt to delete...");
+
+ if (keyctl(KEYCTL_UNLINK, key, DEST_KEYRING) < 0) {
+ pam_syslog(ph, LOG_ERR, "error: Deleting key from "
+ "keyring for %s (%s)",
+ currentaddress, hostdomain);
+ }
}
}
+
+ currentaddress = nextaddress;
+ if (currentaddress) {
+ nextaddress = strchr(currentaddress, ',');
+ if (nextaddress)
+ *nextaddress++ = '\0';
+ }
+
+ }
+
+ exit(0);
+ } else {
+ // parent process
+ int wstatus;
+ if(waitpid(keyring_process, &wstatus, 0) < 0) {
+ pam_syslog(ph, LOG_ERR, "waitpid(): %s", strerror(errno));
+ return PAM_SERVICE_ERR;
}

- currentaddress = nextaddress;
- if (currentaddress) {
- nextaddress = strchr(currentaddress, ',');
- if (nextaddress)
- *nextaddress++ = '\0';
+ if(WIFEXITED(wstatus)) {
+ if(WEXITSTATUS(wstatus) != 0) {
+ pam_syslog(ph, LOG_ERR, "keyring subprocess failed: %x", WEXITSTATUS(wstatus));
+ return WEXITSTATUS(wstatus);
+ }
+ } else {
+ pam_syslog(ph, LOG_ERR, "keyring subprocess terminated abnormally: %x", wstatus);
+ return PAM_SERVICE_ERR;
}
}

@@ -311,34 +368,86 @@ static int cifscreds_pam_update(pam_handle_t *ph, const char *user, const
char *
return PAM_SERVICE_ERR;
}

- /* search for necessary credentials stashed in session keyring */
- currentaddress = addrstr;
- nextaddress = strchr(currentaddress, ',');
- if (nextaddress)
- *nextaddress++ = '\0';
-
- while (currentaddress) {
- if (key_search(currentaddress, keytype) > 0)
- count++;
-
- currentaddress = nextaddress;
- if (currentaddress) {
- nextaddress = strchr(currentaddress, ',');
- if (nextaddress)
- *nextaddress++ = '\0';
- }
+ // Impersonate target user.
+ //
+ // PAM runs as root, so if DEST_KEYRING == KEY_SPEC_USER_KEYRING or
+ // KEY_SPEC_USER_SESSION_KEYRING, the credentials would be cached by root,
+ // uselessly. This isn't needed for DEST_KEYRING == KEY_SPEC_SESSION_KEYRING
+ // which gets inherited by the child process tree of the ultimate login shell.
+ //
+ // The main work has to be done in a subprocess, because only the real UID has rights to edit
that UID's keyring
+ // so we have to call setuid(), but we can't un-setuid() later, yet we still need to be root for
the other PAM modules to behave.
+ errno = 0;
+ struct passwd *pwnam = getpwnam(user);
+ if(pwnam == NULL) {
+ pam_syslog(ph, LOG_ERR, "getpwnam(): %s", strerror(errno));
+ return PAM_SERVICE_ERR;
}

- if (!count) {
- pam_syslog(ph, LOG_ERR, "You have no same stashed credentials for %s", hostdomain);
+ pid_t keyring_process = fork();
+ if(keyring_process == -1) {
+ pam_syslog(ph, LOG_ERR, "fork(): %s", strerror(errno));
return PAM_SERVICE_ERR;
- }
+ } else if(keyring_process == 0) {
+ // child process
+
+ if(setgid(pwnam->pw_gid) < 0) {
+ pam_syslog(ph, LOG_ERR, "setegid(): %s", strerror(errno));
+ exit(PAM_SERVICE_ERR);
+ }
+ if(setuid(pwnam->pw_uid) < 0) {
+ pam_syslog(ph, LOG_ERR, "seteuid(): %s", strerror(errno));
+ exit(PAM_SERVICE_ERR);
+ }

- for (id = 0; id < count; id++) {
- key_serial_t key = key_add(currentaddress, user, password, keytype);
- if (key <= 0) {
- pam_syslog(ph, LOG_ERR, "error: Update credential key for %s: %s",
- currentaddress, strerror(errno));
+ /* search for necessary credentials stashed in session keyring */
+ currentaddress = addrstr;
+ nextaddress = strchr(currentaddress, ',');
+ if (nextaddress)
+ *nextaddress++ = '\0';
+
+ while (currentaddress) {
+ if (key_search(currentaddress, keytype) > 0)
+ count++;
+
+ currentaddress = nextaddress;
+ if (currentaddress) {
+ nextaddress = strchr(currentaddress, ',');
+ if (nextaddress)
+ *nextaddress++ = '\0';
+ }
+ }
+
+ if (!count) {
+ pam_syslog(ph, LOG_ERR, "You have no same stashed credentials for %s", hostdomain);
+ exit(PAM_SERVICE_ERR);
+ }
+
+ for (id = 0; id < count; id++) {
+ key_serial_t key = key_add(currentaddress, user, password, keytype);
+ if (key <= 0) {
+ pam_syslog(ph, LOG_ERR, "error: Update credential key for %s: %s",
+ currentaddress, strerror(errno));
+ }
+ }
+
+ exit(0);
+ } else {
+ // parent process
+ int wstatus;
+ if(waitpid(keyring_process, &wstatus, 0) < 0) {
+ pam_syslog(ph, LOG_ERR, "waitpid(): %s", strerror(errno));
+ return PAM_SERVICE_ERR;
+ }
+
+ if(WIFEXITED(wstatus)) {
+ if(WEXITSTATUS(wstatus) != 0) {
+ pam_syslog(ph, LOG_ERR, "keyring subprocess failed: %x", WEXITSTATUS(wstatus));
+ return WEXITSTATUS(wstatus);
+ }
+ } else {
+ pam_syslog(ph, LOG_ERR, "keyring subprocess terminated abnormally: %x", wstatus);
+ return PAM_SERVICE_ERR;
}
}

@@ -426,7 +535,6 @@ PAM_EXTERN int pam_sm_open_session(pam_handle_t *ph, int flags
__attribute__((un
const char *hostdomain = NULL;
uint args;
int retval;
- key_serial_t ses_key, uses_key;

args = parse_args(ph, argc, argv, &hostdomain);

@@ -460,24 +568,6 @@ PAM_EXTERN int pam_sm_open_session(pam_handle_t *ph, int flags
__attribute__((un
return PAM_SERVICE_ERR;
}

- /* make sure there is a session keyring */
- ses_key = keyctl_get_keyring_ID(KEY_SPEC_SESSION_KEYRING, 0);
- if (ses_key == -1) {
- if (errno == ENOKEY)
- pam_syslog(ph, LOG_ERR, "you have no session keyring. "
- "Consider using pam_keyinit to "
- "install one.");
- else
- pam_syslog(ph, LOG_ERR, "unable to query session "
- "keyring: %s", strerror(errno));
- }
-
- /* A problem querying the user-session keyring isn't fatal. */
- uses_key = keyctl_get_keyring_ID(KEY_SPEC_USER_SESSION_KEYRING, 0);
- if ((uses_key >= 0) && (ses_key == uses_key))
- pam_syslog(ph, LOG_ERR, "you have no persistent session "
- "keyring. cifscreds keys will not persist.");
-
return cifscreds_pam_add(ph, user, password, args, hostdomain);
}

--
2.34.1
Nick Guenther Aug. 16, 2022, 7:14 p.m. UTC | #5
August 3, 2022 4:29 PM, "Nick Guenther" <nick.guenther@polymtl.ca> wrote:

> August 1, 2022 12:10 AM, "Steve French" <smfrench@gmail.com> wrote:
> 
>> On Thu, Jul 28, 2022 at 9:50 PM Nick Guenther <nick.guenther@polymtl.ca> wrote:
>>> 
>>> Would there be interest in switching to KEY_SPEC_USER_KEYRING? Would it be a good idea? Can I
>>> assume the kernel CIFS code would need a matching change?
>>
>> Adding Shyam to this discussion - interesting questions worth investigating.
>> 
>
> I've written a patch that switches cifs-utils to KEY_SPEC_USER_KEYRING. It seems to behave --
> `keylist list @u` shows the cifs key instead of `keyctl list @s` -- but it doesn't solve my problem
> without being combined with the 'revoke' workaround, because the kernel is still using
> KEY_SPEC_SESSION_KEYRING.

Hey all,

I'm going to unsubscribe from linux-cifs. It's mostly over my head and not really my interests. But if you find some time to review this patch and find interest in it, please make sure to CC me to keep me in the loop!

Thanks for your consideration!

-Nick
diff mbox series

Patch

diff --git a/cifscreds.c b/cifscreds.c
index 32f2ee4..9ba298d 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -446,7 +446,7 @@  check_session_keyring(void)
        }
 
        /* A problem querying the user-session keyring isn't fatal. */
-       uses_key = keyctl_get_keyring_ID(KEY_SPEC_USER_SESSION_KEYRING, 0);
+       uses_key = keyctl_get_keyring_ID(KEY_SPEC_USER_KEYRING, 0);
        if (uses_key == -1)
                return 0;
 
diff --git a/pam_cifscreds.c b/pam_cifscreds.c
index 5d99c2d..3c127bb 100644
--- a/pam_cifscreds.c
+++ b/pam_cifscreds.c
@@ -473,7 +473,7 @@  PAM_EXTERN int pam_sm_open_session(pam_handle_t *ph, int flags __attribute__((un
        }
 
        /* A problem querying the user-session keyring isn't fatal. */
-       uses_key = keyctl_get_keyring_ID(KEY_SPEC_USER_SESSION_KEYRING, 0);
+       uses_key = keyctl_get_keyring_ID(KEY_SPEC_USER_KEYRING, 0);
        if ((uses_key >= 0) && (ses_key == uses_key))
                pam_syslog(ph, LOG_ERR, "you have no persistent session "
                                "keyring. cifscreds keys will not persist.");