[Pam-developers] Use different way to check for passwd accessibility by root in pam_rootok

Tomas Mraz tmraz at redhat.com
Thu Feb 28 16:17:54 UTC 2013


On Tue, 2013-02-12 at 03:00 +0400, Dmitry V. Levin wrote: 
> On Thu, Feb 07, 2013 at 05:15:11PM +0100, Tomas Mraz wrote:
> > The attached patch uses different way to check for passwd accessibility
> > by root in pam_rootok module. This method allows for proper auditing the
> > denial of access, so the user can find the user AVC message in the
> > audit.log.
> > --- a/modules/pam_rootok/pam_rootok.c
> > +++ b/modules/pam_rootok/pam_rootok.c
> [...]
> > @@ -55,6 +60,54 @@ _pam_parse (const pam_handle_t *pamh, int argc, const char **argv)
> >      return ctrl;
> >  }
> >  
> > +#ifdef WITH_SELINUX
> > +static int
> > +log_callback (int type, const char *fmt, ...) 
> > +{
> > +    int audit_fd;
> > +    va_list ap;
> > +
> > +    va_start(ap, fmt);
> > +#ifdef HAVE_LIBAUDIT
> > +    audit_fd = audit_open();
> > +
> > +    if (audit_fd >= 0) {
> > +	char buf[PATH_MAX*2];
> > +
> > +	vsnprintf(buf, sizeof(buf), fmt, ap);
> 
> Why PATH_MAX*2?  We use vasprintf(3) in libpam/pam_syslog.c already, and
> it seems to be suitable here.
OK, I'll change that.

> > +	audit_log_user_avc_message(audit_fd, AUDIT_USER_AVC, buf, NULL, NULL,
> > +				   NULL, 0);
> 
> If we had a pam_handle_t* here, we could pass pamh->rhost and pamh->tty
> like libpam/pam_audit.c does.
> 
> > +	audit_close(audit_fd);
> > +	return 0;
> > +    }
> > +
> > +#endif
> > +    vsyslog (LOG_USER | LOG_INFO, fmt, ap);
> 
> If we had a pam_handle_t* here, we could use pam_vsyslog() instead.
> 
> > +    va_end(ap);
> > +    return 0;
> > +}
> > +
> > +static int
> > +selinux_check_root (void)
> > +{
> > +    int status = -1;
> > +    security_context_t user_context;
> > +
> > +    if (is_selinux_enabled() < 1)
> > +	return 0;
> > +
> > +    /* setup callbacks */
> > +    selinux_set_callback(SELINUX_CB_LOG, (union selinux_callback) &log_callback);
> 
> Shouldn't we save current callback using
> selinux_get_callback(SELINUX_CB_LOG) and restore it afterwards?
Perhaps we should. I'll do that.

> Btw, we can introduce a trampoline function so that pam_handle_t* could be
> passed down to the real callback where it may be needed, e.g.
> 
> 	int callback (int type, const char *fmt, ...)
> 	{
> 		va_list ap;
> 		va_start(ap, fmt);
> 		log_callback(pamh, av);
> 		va_end(ap);
> 		return 0;
> 	}
> 	
> 	union selinux_callback old_cb, new_cb;
> 	old_cb = selinux_get_callback(SELINUX_CB_LOG);
> 	new_cb.func_log = callback;
> 	selinux_set_callback(SELINUX_CB_LOG, new_cb);
> 	...
> 	selinux_set_callback(SELINUX_CB_LOG, old_cb);

Don't trampolines imply having executable stack? I don't think we can
afford such thing in PAM modules. And I am not sure the few improvements
we could get with trampoline are even worth it.

> 
> > +    if ((status = getcon(&user_context)) < 0)
> > +	return status;
> > +
> > +    status = selinux_check_access(user_context, user_context, "passwd", "passwd", NULL);
> 
> There used to be selinux_check_passwd_access() before which differs from
> this selinux_check_access() by using getprevcon() instead of getcon().
> What was the intention of this change?

That was suggested by Dan Walsh but to keep the behavior backwards
compatible we should probably use getprevcon() as that is what
checkPasswdAccess() internally does. I will do that.

> > +
> > +    freecon(user_context);
> > +    return status;
> > +}
> > +#endif
> > +
> >  static int
> >  check_for_root (pam_handle_t *pamh, int ctrl)
> >  {
> > @@ -62,7 +115,7 @@ check_for_root (pam_handle_t *pamh, int ctrl)
> >  
> >      if (getuid() == 0)
> >  #ifdef WITH_SELINUX
> > -      if (is_selinux_enabled()<1 || checkPasswdAccess(PASSWD__ROOTOK)==0)
> > +      if (selinux_check_root() != 0)
> 
> Yes, as I said before, it has to be changed to
> 	if (selinux_check_root() == 0)
Yes, sure.

> >  #endif
> >  	retval = PAM_SUCCESS;
> >  
> 
> 
> _______________________________________________
> Pam-developers mailing list
> Pam-developers at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/pam-developers

-- 
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
                                              Turkish proverb



More information about the Pam-developers mailing list