[sudo-workers] PAM and return of init_session ignored ?

[aurel]

On 10/08/2015 18:29, Todd C. Miller wrote:

> You should add some debugging to print the return value of
> pam_open_session() to make sure it is not actually returning
> PAM_SUCCESS.  If memory serves, when you have multiple session
> modules you can end up with PAM_SUCCESS as long as the later ones
> succeed.
>
>   - todd

After debugging, the return value of pam_open_session() is 14 (PAM_SESSION_ERR).
With multiple session modules, if one 'required' failed (before a 'sufficient'),
PAM will return an error to sudo.

In sudo_pam_begin_session(), status is set to 1 (AUTH_FAILURE) if an error is returned by pam_open_session()


pam.c

|sudo_pam_begin_session(struct passwd *pw, char **user_envp[], sudo_auth *auth)||
||{||
||	...||
||	if (def_pam_session) {
         *pam_status = pam_open_session(pamh, 0);
         if (*pam_status != PAM_SUCCESS) {  		 /* PAM_SUCCESS = 0  (PAM_SESSION_ERR = 14) */
             (void) pam_end(pamh, *pam_status | PAM_DATA_SILENT);
             pamh = NULL;
             status = AUTH_FAILURE;  			 /* AUTH_FAILURE = 1 */
             goto done;
         }
     }
	...
	done:		
||  	    debug_return_int(status);||   /* return 1 */
||}|

The problem is in pam_auth.c.|  inside|sudo_auth_begin_session(), the return of sudo_pam_begin_session() is compared
with AUTH_FATAL whether there is an error. Problem: the error status is 1 and AUTH_FATAL is 3.
So, if module returned a success, it's ok, else, it's ok too.


pam_auth.c

int sudo_auth_begin_session(struct passwd *pw, char **user_env[])
{
     sudo_auth *auth;
     int status = AUTH_SUCCESS;
     debug_decl(sudo_auth_begin_session, SUDO_DEBUG_AUTH)

     for (auth = auth_switch; auth->name; auth++) {
         if (auth->begin_session && !IS_DISABLED(auth)) {
             status = (auth->begin_session)(pw, user_env, auth);  	/* call sudo_pam_begin_session */
             if (status == AUTH_FATAL) 		/* error status = 1 and AUTH_FATAL = 3,  module error will not be detected */
                 break;
         }
     }
     debug_return_int(status == AUTH_FATAL ? -1 : 1);  /* always return 1 */
}

The solution is to change in pam.c :
	status = AUTH_FAILURE;
To :
	status = AUTH_FAIL;


I await your opinion before making a patch