Relevant BUGIDs: 468724

Purpose of commit: bugfix

Commit summary:
---------------
Legacy behavior for pam_close_session and pam_setcred was not sufficient.
Basically, it appears to be common practice for some applications to call
these functions without first calling pam_authenticate and pam_open_session
which would have frozen the auth and session module stacks.

The new behavior is to treat the returns of these secondary functions as
authoritative when navigating the stack in the absence of a chain-freezing
first set of calls.

pam_chauthtok should not benefit from this behavior, and there does not
appear to be a justification for using an event like this to freeze the
stack outright - legacy behavior did not do that.

diff --git a/CHANGELOG b/CHANGELOG
index d9c2295fcb07a46a83ffb918ffec211c0003a256..e00ff4e705f9fb68704414f0ea6050a9b318f8ac 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -49,6 +49,9 @@ bug report - outstanding bugs are listed here:
 0.76: please submit patches for this section with actual code/doc
       patches!
 
+* fix for legacy behavior of pam_setcred and pam_close_session in
+  the case that pam_authenticate and pam_open_session hadn't been
+  called - bug report from S Park. (Bug 468724 - agmorgan)
 * some BSD updates and fixes from Mark Murray - including a slightly
   more robust conversation function and some minimization of gcc
   warnings. (Bugs 449203,463984 - agmorgan)

diff --git a/libpam/pam_dispatch.c b/libpam/pam_dispatch.c
index 2a6befd4177e1d53e774962819f0c0fc5ab6bfa9..3ebdb5ba797972dd4df29e5613bcb26cb4f5022b 100644 (file)
--- a/libpam/pam_dispatch.c
+++ b/libpam/pam_dispatch.c
@@ -22,6 +22,11 @@
 #define _PAM_POSITIVE +1
 #define _PAM_NEGATIVE -1
 
+/* frozen chain required codes */
+#define _PAM_PLEASE_FREEZE  0
+#define _PAM_MAY_BE_FROZEN  1
+#define _PAM_MUST_BE_FROZEN 2
+
 /*
  * walk a stack of modules.  Interpret the administrator's instructions
  * when combining the return code of each module.
@@ -99,9 +104,45 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h,
            return retval;
        }
 
-       if (use_cached_chain) {
-           /* a former stack execution has frozen the chain */
+       /*
+        * use_cached_chain is how we ensure that the setcred/close_session
+        * and chauthtok(2) modules are called in the same order as they did
+        * when they were invoked as auth/open_session/chauthtok(1). This
+        * feature was added in 0.75 to make the behavior of pam_setcred
+        * sane. It was debugged by release 0.76.
+        */
+       if (use_cached_chain != _PAM_PLEASE_FREEZE) {
+
+           /* a former stack execution should have frozen the chain */
+
            cached_retval = *(h->cached_retval_p);
+           if (cached_retval == _PAM_INVALID_RETVAL) {
+
+               /* This may be a problem condition. It implies that
+                  the application is running setcred, close_session,
+                  chauthtok(2nd) without having first run
+                  authenticate, open_session, chauthtok(1st)
+                  [respectively]. */
+
+               D(("use_cached_chain is set to [%d],"
+                  " but cached_retval == _PAM_INVALID_RETVAL",
+                  use_cached_chain));
+
+               /* In the case of close_session and setcred there is a
+                  backward compatibility reason for allowing this, in
+                  the chauthtok case we have encountered a bug in
+                  libpam! */
+
+               if (use_cached_chain == _PAM_MAY_BE_FROZEN) {
+                   /* (not ideal) force non-frozen stack control. */
+                   cached_retval = retval;
+               } else {
+                   D(("BUG in libpam -"
+                      " chain is required to be frozen but isn't"));
+
+                   /* cached_retval is already _PAM_INVALID_RETVAL */
+               }
+           }
        } else {
            /* this stack execution is defining the frozen chain */
            cached_retval = h->cached_retval = retval;
@@ -110,6 +151,7 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h,
        /* verify that the return value is a valid one */
        if ((cached_retval < PAM_SUCCESS)
            || (cached_retval >= _PAM_RETURN_VALUES)) {
+
            retval = PAM_MUST_FAIL_CODE;
            action = _PAM_ACTION_BAD;
        } else {
@@ -133,11 +175,6 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h,
        switch (action) {
        case _PAM_ACTION_RESET:
 
-           /* if (use_cached_chain) {
-                  XXX - we need to consider the use_cached_chain case
-                        do we want to trash accumulated info here..?
-              } */
-
            impression = _PAM_UNDEF;
            status = PAM_MUST_FAIL_CODE;
            break;
@@ -145,10 +182,6 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h,
        case _PAM_ACTION_OK:
        case _PAM_ACTION_DONE:
 
-           /* XXX - should we maintain cached_status and status in
-               the case of use_cached_chain? The same with BAD&DIE
-               below */
-
            if ( impression == _PAM_UNDEF
                 || (impression == _PAM_POSITIVE && status == PAM_SUCCESS) ) {
                impression = _PAM_POSITIVE;
@@ -178,11 +211,6 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h,
            break;
 
        case _PAM_ACTION_IGNORE:
-           /* if (use_cached_chain) {
-                   XXX - when evaluating a cached
-                   chain, do we still want to ignore the module's
-                   return value?
-              } */
            break;
 
         /* if we get here, we expect action is a positive number --
@@ -261,7 +289,7 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice)
        return retval;
     }
 
-    use_cached_chain = 0;   /* default to setting h->cached_retval */
+    use_cached_chain = _PAM_PLEASE_FREEZE;
 
     switch (choice) {
     case PAM_AUTHENTICATE:
@@ -269,7 +297,7 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice)
        break;
     case PAM_SETCRED:
        h = pamh->handlers.conf.setcred;
-       use_cached_chain = 1;
+       use_cached_chain = _PAM_MAY_BE_FROZEN;
        break;
     case PAM_ACCOUNT:
        h = pamh->handlers.conf.acct_mgmt;
@@ -279,12 +307,12 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice)
        break;
     case PAM_CLOSE_SESSION:
        h = pamh->handlers.conf.close_session;
-       use_cached_chain = 1;
+       use_cached_chain = _PAM_MAY_BE_FROZEN;
        break;
     case PAM_CHAUTHTOK:
        h = pamh->handlers.conf.chauthtok;
        if (flags & PAM_UPDATE_AUTHTOK) {
-           use_cached_chain = 1;
+           use_cached_chain = _PAM_MUST_BE_FROZEN;
        }
        break;
     default:

diff --git a/libpam/pam_handlers.c b/libpam/pam_handlers.c
index 8e32f8e80c7f5d070906216d9a44ac5454e01d94..d007ed9858f4bf92d50d4cbbf4e64af1d554cc56 100644 (file)
--- a/libpam/pam_handlers.c
+++ b/libpam/pam_handlers.c
@@ -751,7 +751,7 @@ int _pam_add_handler(pam_handle_t *pamh
     (*handler_p)->must_fail = must_fail;        /* failure forced? */
     (*handler_p)->func = func;
     memcpy((*handler_p)->actions,actions,sizeof((*handler_p)->actions));
-    (*handler_p)->cached_retval = -1;                     /* error */
+    (*handler_p)->cached_retval = _PAM_INVALID_RETVAL;
     (*handler_p)->cached_retval_p = &((*handler_p)->cached_retval);
     (*handler_p)->argc = argc;
     (*handler_p)->argv = argv;                       /* not a copy */
@@ -772,7 +772,7 @@ int _pam_add_handler(pam_handle_t *pamh
        (*handler_p2)->must_fail = must_fail;        /* failure forced? */
        (*handler_p2)->func = func2;
        memcpy((*handler_p2)->actions,actions,sizeof((*handler_p2)->actions));
-       (*handler_p2)->cached_retval = -1;            /* ignored */
+       (*handler_p2)->cached_retval =  _PAM_INVALID_RETVAL;     /* ignored */
        /* Note, this next entry points to the handler_p value! */
        (*handler_p2)->cached_retval_p = &((*handler_p)->cached_retval);
        (*handler_p2)->argc = argc;

diff --git a/libpam/pam_private.h b/libpam/pam_private.h
index 52f6c5e6157fff09689cddc38486e2acf5be515c..7afc4fa75b0e0242ca7909f086f4086f1b14596a 100644 (file)
--- a/libpam/pam_private.h
+++ b/libpam/pam_private.h
@@ -43,6 +43,8 @@
 
 /* components of the pam_handle structure */
 
+#define _PAM_INVALID_RETVAL  -1    /* default value for cached_retval */
+
 struct handler {
     int must_fail;
     int (*func)(pam_handle_t *pamh, int flags, int argc, char **argv);