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

On Thu, 2013-02-28 at 17:17 +0100, Tomas Mraz wrote: > 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; > > > So, here is the patch with the changes I agreed with attached. OK to commit?