Bug 622700

Summary: common PAM patch for coreutils' su
Product: [Fedora] Fedora Reporter: Ludwig Nussel <ludwig.nussel>
Component: coreutilsAssignee: Ondrej Vasik <ovasik>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: aquini, kdudka, meyering, ovasik, tmraz, twaugh
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-11-15 07:29:52 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Attachments:
Description Flags
pam patch for su
none
pam patch for su, try 2 none

Description Ludwig Nussel 2010-08-10 03:58:50 EDT
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.
Comment 1 Ondrej Vasik 2010-08-16 10:41:18 EDT
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 :) ) ...
Comment 2 Tomas Mraz 2010-08-16 11:08:32 EDT
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.
Comment 3 Ludwig Nussel 2010-08-17 04:16:42 EDT
(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.
Comment 4 Tomas Mraz 2010-08-17 04:21:51 EDT
(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?
Comment 5 Ludwig Nussel 2010-08-17 04:36:01 EDT
(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.
Comment 6 Tomas Mraz 2010-08-17 05:59:25 EDT
(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 7 Kamil Dudka 2010-08-17 06:47:43 EDT
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...
Comment 8 Ludwig Nussel 2010-08-17 08:02:23 EDT
(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 :-)
Comment 9 Kamil Dudka 2010-08-17 08:23:05 EDT
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.
Comment 10 Ludwig Nussel 2010-08-17 09:36:33 EDT
(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.
Comment 11 Ludwig Nussel 2010-08-18 04:11:30 EDT
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
Comment 12 Kamil Dudka 2010-08-18 15:14:45 EDT
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 =
Comment 13 Ludwig Nussel 2010-08-19 08:23:16 EDT
ok
Comment 14 Ondrej Vasik 2010-11-15 07:29:52 EST
Added few lines of info documentation (about -l PAM file) and built as coreutils-8.7-1.fc15 . Closing RAWHIDE.