From 547e42fc5cb7c0208eeb002809c3d270334af114 Mon Sep 17 00:00:00 2001 From: "Andrew G. Morgan" Date: Mon, 5 Feb 2001 06:50:41 +0000 Subject: Relevant BUGIDs: 129775 Purpose of commit: bugfix Commit summary: --------------- This bugfix leads to backwardly incompatable behavior with earlier releases of Linux-PAM. Note, this cleans up the setcred/session and chauthtok stacks in such a way that it is no longer preferred that the setcred module always return the same error code as the auth components of said modules did. This means behavior should be a great deal more sane. It also gives meaning to the unique return codes that are available to pam_sm_setcred. [I'm sure that when we add support for credential relevant events, this change will be critical.] --- CHANGELOG | 3 ++ doc/pam_appl.sgml | 10 ++--- doc/pam_modules.sgml | 35 +++++++++++----- examples/xsh.c | 34 +++++++++++---- libpam/include/security/pam_modules.h | 2 +- libpam/pam_dispatch.c | 78 +++++++++++++++++++++++++++++++---- libpam/pam_handlers.c | 5 +++ libpam/pam_private.h | 3 ++ 8 files changed, 137 insertions(+), 33 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 336d00bf..b0c0b35e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -35,6 +35,9 @@ Where you should replace XXXXX with a bug-id. 0.75: please submit patches for this section with actual code/doc patches! +* improved handling of the setcred/close_session and update chauthtok + stack. *Warning* This is a backwardly incompatable change, but 'more + sane' than before. (Bug 129775 - agmorgan). * bumped the version number, and added some code to assist in making documentation releases (Bug 129644 - agmorgan). diff --git a/doc/pam_appl.sgml b/doc/pam_appl.sgml index b1010203..9149ecd5 100644 --- a/doc/pam_appl.sgml +++ b/doc/pam_appl.sgml @@ -46,7 +46,7 @@ DAMAGE. The Linux-PAM Application Developers' Guide <author>Andrew G. Morgan, <tt>morgan@kernel.org</tt> -<date>DRAFT v0.74 2001/01/21 +<date>DRAFT v0.75 2001/02/04 <abstract> This manual documents what an application developer needs to know about the <bf>Linux-PAM</bf> library. It describes how an application @@ -218,9 +218,9 @@ PAM is also capable of setting and deleting the users credentials with the call <tt>pam_setcred()</tt>. This function should always be called after the user is authenticated and before service is offered to the user. By convention, this should be the last call to the PAM -library before service is given to the user. What exactly a -credential is, is not well defined. However, some examples are given -in the glossary below. +library before the PAM session is opened. What exactly a credential +is, is not well defined. However, some examples are given in the +glossary below. <sect>The public interface to <bf>Linux-PAM</bf> @@ -565,7 +565,7 @@ extern int pam_setcred(pam_handle_t *pamh, int flags); <p> This function is used to set the module-specific credentials of the user. It is usually called after the user has been authenticated, -after the account management function has been called and after a +after the account management function has been called but before a session has been opened for the user. <p> diff --git a/doc/pam_modules.sgml b/doc/pam_modules.sgml index 694eff85..8afd01fa 100644 --- a/doc/pam_modules.sgml +++ b/doc/pam_modules.sgml @@ -49,7 +49,7 @@ DAMAGE. <title>The Linux-PAM Module Writers' Guide <author>Andrew G. Morgan, <tt>morgan@kernel.org</tt> -<date>DRAFT v0.74 2001/01/21 +<date>DRAFT v0.75 2001/02/04 <abstract> This manual documents what a programmer needs to know in order to write a module that conforms to the <bf/Linux-PAM/ standard. It also @@ -693,7 +693,7 @@ scheme. Generally, an authentication module may have access to more information about a user than their authentication token. This function is used to make such information available to the application. It should only be called <em/after/ the user has been -authenticated and after a session has been established. +authenticated but before a session has been established. <p> Permitted flags, one of which, may be logically OR'd with @@ -711,16 +711,26 @@ Permitted flags, one of which, may be logically OR'd with </descrip> <p> -Generally, the module should attempt to return the same error code as -<tt/pam_sm_authenticate/ did. This will preserve the logic followed -by libpam as it executes the stack of <em/authentication/ modules, -when the application calls <tt/pam_authenticate()/ and -<tt/pam_setcred()/. Failing to do this, will lead to much confudion -on the part of the System administrator. +Prior to <bf/Linux-PAM-0.75/, and due to a deficiency with the way the +<tt/auth/ stack was handled in the case of the setcred stack being +processed, the module was required to attempt to return the same error +code as <tt/pam_sm_authenticate/ did. This was necessary to preserve +the logic followed by libpam as it executes the stack of +<em/authentication/ modules, when the application called either +<tt/pam_authenticate()/ or <tt/pam_setcred()/. Failing to do this, +led to confusion on the part of the System Administrator. <p> -<bf>The following text is depreciated. Some thought needs to be given to -how the credential setting modules are supposed to be stacked...</bf> +For <bf/Linux-PAM-0.75/ and later, libpam handles the credential stack +much more sanely. The way the <tt/auth/ stack is navigated in order to +evaluate the <tt/pam_setcred()/ function call, independent of the +<tt/pam_sm_setcred()/ return codes, is exactly the same way that it +was navigated when evaluating the <tt/pam_authenticate()/ library +call. Typically, if a stack entry was ignored in evaluating +<tt/pam_authenticate()/, it will be ignored when libpam evaluates the +<tt/pam_setcred()/ function call. Otherwise, the return codes from +each module specific <tt/pam_sm_setcred()/ call are treated as +<tt/required/. <p> Besides <tt/PAM_SUCCESS/, the module may return one of the following @@ -737,6 +747,11 @@ errors: This module was unable to set the credentials of the user. </descrip> +<p> +these, non-<tt/PAM_SUCCESS/, return values will typically lead to the +credential stack <em/failing/. The first such error will dominate in +the return value of <tt/pam_setcred()/. + </itemize> <sect1> Account management diff --git a/examples/xsh.c b/examples/xsh.c index 13971a2d..0987da26 100644 --- a/examples/xsh.c +++ b/examples/xsh.c @@ -33,19 +33,25 @@ static struct pam_conv conv = { int main(int argc, char **argv) { pam_handle_t *pamh=NULL; - char *username=NULL; + const char *username=NULL; + const char *service="xsh"; int retcode; - /* did the user call with a username as an argument ? */ + /* did the user call with a username as an argument ? + * did they also */ - if (argc > 2) { - fprintf(stderr,"usage: %s [username]\n",argv[0]); - } else if (argc == 2) { + if (argc > 3) { + fprintf(stderr,"usage: %s [username [service-name]]\n",argv[0]); + } + if (argc >= 2) { username = argv[1]; - } + } + if (argc == 3) { + service = argv[2]; + } /* initialize the Linux-PAM library */ - retcode = pam_start("xsh", username, &conv, &pamh); + retcode = pam_start(service, username, &conv, &pamh); bail_out(pamh,1,retcode,"pam_start"); /* to avoid using goto we abuse a loop here */ @@ -97,7 +103,10 @@ int main(int argc, char **argv) break; } - fprintf(stderr,"The user has been authenticated and `logged in'\n"); + pam_get_item(pamh, PAM_USER, (const void **) &username); + fprintf(stderr, + "The user [%s] has been authenticated and `logged in'\n", + username); /* this is always a really bad thing for security! */ system("/bin/sh"); @@ -113,6 +122,15 @@ int main(int argc, char **argv) break; } + /* `0' could be as above */ + retcode = pam_setcred(pamh, PAM_DELETE_CRED); + bail_out(pamh,0,retcode,"pam_setcred"); + if (retcode != PAM_SUCCESS) { + fprintf(stderr,"%s: problem deleting user credentials\n" + ,argv[0]); + break; + } + break; /* don't go on for ever! */ } diff --git a/libpam/include/security/pam_modules.h b/libpam/include/security/pam_modules.h index b555e8b2..4182ebd6 100644 --- a/libpam/include/security/pam_modules.h +++ b/libpam/include/security/pam_modules.h @@ -112,7 +112,7 @@ PAM_EXTERN int pam_sm_chauthtok(pam_handle_t *pamh, int flags, #define PAM_PRELIM_CHECK 0x4000 /* The password service should update passwords Note: PAM_PRELIM_CHECK - * and PAM_UPDATE_AUTHTOK can not both be set simultaneously! */ + * and PAM_UPDATE_AUTHTOK cannot both be set simultaneously! */ #define PAM_UPDATE_AUTHTOK 0x2000 diff --git a/libpam/pam_dispatch.c b/libpam/pam_dispatch.c index 02325665..6212ac87 100644 --- a/libpam/pam_dispatch.c +++ b/libpam/pam_dispatch.c @@ -1,7 +1,7 @@ /* pam_dispatch.c - handles module function dispatch */ /* - * Copyright (c) 1998 Andrew G. Morgan <morgan@linux.kernel.org> + * Copyright (c) 1998 Andrew G. Morgan <morgan@kernel.org> * * $Id$ */ @@ -28,7 +28,7 @@ */ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, - _pam_boolean resumed) + _pam_boolean resumed, int use_cached_chain) { int depth, impression, status, skip_depth; @@ -62,7 +62,7 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, /* Loop through module logic stack */ for (depth=0 ; h != NULL ; h = h->next, ++depth) { - int retval, action; + int retval, cached_retval, action; /* skip leading modules if they have already returned */ if (depth < skip_depth) { @@ -78,7 +78,7 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, retval = h->func(pamh, flags, h->argc, h->argv); D(("module returned: %s", pam_strerror(pamh, retval))); if (h->must_fail) { - D(("module poorly listed in pam.conf; forcing failure")); + D(("module poorly listed in PAM config; forcing failure")); retval = PAM_MUST_FAIL_CODE; } } @@ -99,23 +99,57 @@ 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 */ + cached_retval = *(h->cached_retval_p); + } else { + /* this stack execution is defining the frozen chain */ + cached_retval = h->cached_retval = retval; + } + /* verify that the return value is a valid one */ - if (retval < PAM_SUCCESS || retval >= _PAM_RETURN_VALUES) { + if ((cached_retval < PAM_SUCCESS) + || (cached_retval >= _PAM_RETURN_VALUES)) { retval = PAM_MUST_FAIL_CODE; action = _PAM_ACTION_BAD; } else { - action = h->actions[retval]; + /* We treat the current retval with some respect. It may + (for example, in the case of setcred) have a value that + needs to be propagated to the user. We want to use the + cached_retval to determine the modules to be executed + in the stacked chain, but we want to treat each + non-ignored module in the cached chain as now being + 'required'. We only need to treat the, + _PAM_ACTION_IGNORE, _PAM_ACTION_IS_JUMP and + _PAM_ACTION_RESET actions specially. */ + + action = h->actions[cached_retval]; } + D((stderr, + "use_cached_chain=%d action=%d cached_retval=%d retval=%d\n", + use_cached_chain, action, cached_retval, retval)); + /* decide what to do */ 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; 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; @@ -129,7 +163,7 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, case _PAM_ACTION_BAD: case _PAM_ACTION_DIE: #ifdef PAM_FAIL_NOW_ON - if ( retval == PAM_ABORT ) { + if ( cached_retval == PAM_ABORT ) { impression = _PAM_NEGATIVE; status = PAM_PERM_DENIED; goto decision_made; @@ -145,6 +179,11 @@ 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 -- @@ -152,6 +191,20 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags, struct handler *h, default: if ( _PAM_ACTION_IS_JUMP(action) ) { + + /* If we are evaluating a cached chain, we treat this + module as required (aka _PAM_ACTION_OK) as well as + executing the jump. */ + + if (use_cached_chain) { + if (impression == _PAM_UNDEF + || (impression == _PAM_POSITIVE + && status == PAM_SUCCESS) ) { + impression = _PAM_POSITIVE; + status = retval; + } + } + /* this means that we need to skip #action stacked modules */ do { h = h->next; @@ -192,7 +245,7 @@ decision_made: /* by getting here we have made a decision */ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice) { struct handler *h = NULL; - int retval; + int retval, use_cached_chain; _pam_boolean resumed; IF_NO_PAMH("_pam_dispatch", pamh, PAM_SYSTEM_ERR); @@ -209,12 +262,15 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice) return retval; } + use_cached_chain = 0; /* default to setting h->cached_retval */ + switch (choice) { case PAM_AUTHENTICATE: h = pamh->handlers.conf.authenticate; break; case PAM_SETCRED: h = pamh->handlers.conf.setcred; + use_cached_chain = 1; break; case PAM_ACCOUNT: h = pamh->handlers.conf.acct_mgmt; @@ -224,9 +280,13 @@ 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; break; case PAM_CHAUTHTOK: h = pamh->handlers.conf.chauthtok; + if (flags & PAM_UPDATE_AUTHTOK) { + use_cached_chain = 1; + } break; default: _pam_system_log(LOG_ERR, "undefined fn choice; %d", choice); @@ -273,7 +333,7 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice) __PAM_TO_MODULE(pamh); /* call the list of module functions */ - retval = _pam_dispatch_aux(pamh, flags, h, resumed); + retval = _pam_dispatch_aux(pamh, flags, h, resumed, use_cached_chain); resumed = PAM_FALSE; __PAM_TO_APP(pamh); diff --git a/libpam/pam_handlers.c b/libpam/pam_handlers.c index a22d66f6..b2065999 100644 --- a/libpam/pam_handlers.c +++ b/libpam/pam_handlers.c @@ -756,6 +756,8 @@ 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_p = &((*handler_p)->cached_retval); (*handler_p)->argc = argc; (*handler_p)->argv = argv; /* not a copy */ (*handler_p)->next = NULL; @@ -775,6 +777,9 @@ 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 */ + /* Note, this next entry points to the handler_p value! */ + (*handler_p2)->cached_retval_p = &((*handler_p)->cached_retval); (*handler_p2)->argc = argc; if (argv) { if (((*handler_p2)->argv = malloc(argvlen)) == NULL) { diff --git a/libpam/pam_private.h b/libpam/pam_private.h index 2aa90bd4..52f6c5e6 100644 --- a/libpam/pam_private.h +++ b/libpam/pam_private.h @@ -47,6 +47,9 @@ struct handler { int must_fail; int (*func)(pam_handle_t *pamh, int flags, int argc, char **argv); int actions[_PAM_RETURN_VALUES]; + /* set by authenticate, open_session, chauthtok(1st) + consumed by setcred, close_session, chauthtok(2nd) */ + int cached_retval; int *cached_retval_p; int argc; char **argv; struct handler *next; -- cgit v1.2.3