On Thu, Mar 21, 2013 at 09:50:36AM +0100, Tomas Mraz wrote:
On Fri, 2013-03-01 at 16:28 +0100, Tomas Mraz wrote:
> 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?
Ping? Anybody can review the updated patch?
Forgot about it, sorry.
No need to
+#include <limits.h>
because PATH_MAX is no longer going to be used, all the rest looks OK.
Haven't tested it myself, though.
--
ldv