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.
+ 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?
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);
+ 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?
+
+ 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)
#endif
retval = PAM_SUCCESS;
--
ldv