Created attachment 437804 [details] pam patch for su upstream coreutils' su doesn't support PAM. Distributions add support for PAM as patch. RedHat and SUSE's patches apparently share a common ancestor but diverged over time. This is another attempt to agree on a common patch that could potentially be sent upstream. The previous attempt failed due to inactivity, see bug 240117. Here's a cleaned up version of the previous patch. I've removed the getdef features and the setting of the default path which are unrelated to the pam feature. Compared to the current patch in Fedora this one - does not require a tty. Requiring a tty doesn't make sense to me and could be enforced via PAM module if really needed. - calls pam_setcred() after initgroups() to allow PAM modules change supplementary groups - does not handle DISPLAY and XAUTHORITY. pam_xauth does that for us.
It makes sense to at least consolidate these downstream patches - especially in such security related area like PAM. Adding our PAM maintainer to CC as he may have some comments to this. I think at least the info about pam support in su (and where to configure it) from our patch should be kept in coreutils.texi . I'll check the rest of the patch later (hopefully later this week :) ) ...
About the DISPLAY and XAUTHORITY handling - I did not read the current RH PAM patch for su, but there might be cases which would be broken with just using pam_xauth. This should be seriously tested for regressions before this part is removed. The other two changes mentioned above make sense to me.
(In reply to comment #1) > I think at least the info about pam support in su (and where to configure it) > from our patch should be kept in coreutils.texi . Ah, yes. It could be kept in a separate patch though. Otherwise the modified paragraphs need to depend on configure settings. (In reply to comment #2) > About the DISPLAY and XAUTHORITY handling - I did not read the current RH PAM > patch for su, but there might be cases which would be broken with just using > pam_xauth. This should be seriously tested for regressions before this part is > removed. It works :-) The difference is that the old pam patch called modify_environment() before pam_open_session() ie pam_xauth had no chance to look at the original environment. My patch does it the other way around.
(In reply to comment #3) > (In reply to comment #2) > > About the DISPLAY and XAUTHORITY handling - I did not read the current RH PAM > > patch for su, but there might be cases which would be broken with just using > > pam_xauth. This should be seriously tested for regressions before this part is > > removed. > > It works :-) The difference is that the old pam patch called > modify_environment() before pam_open_session() ie pam_xauth had no chance to > look at the original environment. My patch does it the other way around. Hmm, but does not it clean the DISPLAY and XAUTHORITY set by the pam_xauth then?
(In reply to comment #4) > (In reply to comment #3) > > It works :-) The difference is that the old pam patch called > > modify_environment() before pam_open_session() ie pam_xauth had no chance to > > look at the original environment. My patch does it the other way around. > > Hmm, but does not it clean the DISPLAY and XAUTHORITY set by the pam_xauth > then? No, pam_xauth uses pam_putenv to set those variables in the environment that is associated with the pam handle. su then uses pam_getenvlist to export the variables into the real environment again.
(In reply to comment #5) > (In reply to comment #4) > > Hmm, but does not it clean the DISPLAY and XAUTHORITY set by the pam_xauth > > then? > > No, pam_xauth uses pam_putenv to set those variables in the > environment that is associated with the pam handle. su then uses > pam_getenvlist to export the variables into the real environment > again. Ah yes, I forgot about that. You're right then and the new patch should work fine in regards to X environment variables.
Comment on attachment 437804 [details] pam patch for su >diff --git a/configure.ac b/configure.ac >index acd397e..2a2f0fc 100644 >--- a/configure.ac >+++ b/configure.ac >@@ -128,6 +128,20 @@ fi > > AC_FUNC_FORK > >+AC_ARG_ENABLE(pam, AS_HELP_STRING([--disable-pam], >+ [Enable PAM support in su (default=auto)]), , [enable_pam=yes]) The configure help string sounds confusingly to me: --disable-pam Enable PAM support in su (default=auto) >diff --git a/src/Makefile.am b/src/Makefile.am >index 1a19fa6..5f202d5 100644 >--- a/src/Makefile.am >+++ b/src/Makefile.am Looks sane. >diff --git a/src/su.c b/src/su.c >index f8f5b61..22e9b0e 100644 >--- a/src/su.c >+++ b/src/su.c ... >+# define SYSLOG_SUCCESS 1 >+# define SYSLOG_FAILURE 1 >+# define SYSLOG_NON_ROOT 1 Why are the defines above hard-wired? Shouldn't they be handled as compile-time options? See the comment above: Compile-time options: -DSYSLOG_SUCCESS Log successful su's (by default, to root) with syslog. -DSYSLOG_FAILURE Log failed su's (by default, to root) with syslog. -DSYSLOG_NON_ROOT Log all su's, not just those to root (UID 0). >@@ -125,6 +147,13 @@ static bool simulate_login; > /* If true, change some environment vars to indicate the user su'd to. */ > static bool change_environment; > >+#ifdef USE_PAM >+static bool _pam_session_opened; >+static bool _pam_cred_established; >+static void export_pamenv (void); >+static void create_watching_parent (void); The declarations of export_pamenv() and create_watching_parent() are redundant. >@@ -200,7 +229,162 @@ log_su (struct passwd const *pw, bool successful) > } > #endif > >+#ifdef USE_PAM >+#define PAM_SERVICE_NAME PROGRAM_NAME >+#define PAM_SERVICE_NAME_L PROGRAM_NAME "-l" >+static bool caught_signal = false; As the variable is alternated from a signal handler, the type should be 'volatile sig_atomic_t'. The rest of the patch will take me more time to digest since I am new to PAM...
(In reply to comment #7) > >+AC_ARG_ENABLE(pam, AS_HELP_STRING([--disable-pam], > >+ [Enable PAM support in su (default=auto)]), , [enable_pam=yes]) > > The configure help string sounds confusingly to me: > > --disable-pam Enable PAM support in su (default=auto) ack > >+# define SYSLOG_SUCCESS 1 > >+# define SYSLOG_FAILURE 1 > >+# define SYSLOG_NON_ROOT 1 > > Why are the defines above hard-wired? Shouldn't they be handled as > compile-time options? Either that or they could be treated distro specific. I'll put them in a separate patch for now. > >+static void export_pamenv (void); > >+static void create_watching_parent (void); > > The declarations of export_pamenv() and create_watching_parent() are redundant. ok > >+#ifdef USE_PAM > >+#define PAM_SERVICE_NAME PROGRAM_NAME > >+#define PAM_SERVICE_NAME_L PROGRAM_NAME "-l" > >+static bool caught_signal = false; > > As the variable is alternated from a signal handler, the type should be > 'volatile sig_atomic_t'. Well, ok :-)
I am not sure I understand the following change: /* su without pam support does not have a helper that keeps sitting on any directory so let's go to /. */ if (chdir ("/") != 0) error (0, errno, _("warning: cannot change directory to %s"), "/"); What exactly is it supposed to do? The return value of waitpid() is not checked, so chances are it may operate on uninitialized local variable 'status'. Otherwise, the patch looks good.
(In reply to comment #9) > I am not sure I understand the following change: > > /* su without pam support does not have a helper that keeps > sitting on any directory so let's go to /. */ > if (chdir ("/") != 0) > error (0, errno, _("warning: cannot change directory to %s"), "/"); > > What exactly is it supposed to do? Avoid keeping some directory/mountpoint busy. If you run exec su ... in e.g. an autofs mounted nfs directory the watching process would prevent the directory from getting umounted. > The return value of waitpid() is not checked, so chances are it may operate on > uninitialized local variable 'status'. > > Otherwise, the patch looks good. Ok, I'll attach an updated patch ASAP.
Created attachment 439316 [details] pam patch for su, try 2 - changed --disable-pam to --enable-pam - removed hunk that enables all logging features - removed redundant function declarations - use sig_atomic_t - fix use of uninitialized variable after waitpid fail
Looks good to me. Here is my tiny follow-up: diff --git a/configure.ac b/configure.ac index 1fb5839..86b3acd 100644 --- a/configure.ac +++ b/configure.ac @@ -128,8 +128,8 @@ fi AC_FUNC_FORK -AC_ARG_ENABLE(pam, AS_HELP_STRING([--enable-pam], - [Enable PAM support in su (default=auto)]), , [enable_pam=yes]) +AC_ARG_ENABLE(pam, AS_HELP_STRING([--disable-pam], + [Disable PAM support in su (default=auto)]), , [enable_pam=yes]) if test "x$enable_pam" != xno; then AC_CHECK_LIB([pam], [pam_start], [enable_pam=yes], [enable_pam=no]) AC_CHECK_LIB([pam_misc], [misc_conv], [:], [enable_pam=no]) diff --git a/src/su.c b/src/su.c index e54fad7..4434cf4 100644 --- a/src/su.c +++ b/src/su.c @@ -236,7 +236,7 @@ log_su (struct passwd const *pw, bool successful) #ifdef USE_PAM #define PAM_SERVICE_NAME PROGRAM_NAME #define PAM_SERVICE_NAME_L PROGRAM_NAME "-l" -static sig_atomic_t caught_signal = false; +static sig_atomic_t volatile caught_signal = false; static pam_handle_t *pamh = NULL; static int retval; static struct pam_conv conv =
ok
Added few lines of info documentation (about -l PAM file) and built as coreutils-8.7-1.fc15 . Closing RAWHIDE.