[Pam-developers] Use different way to check for passwd accessibility by root in pam_rootok
Dmitry V. Levin
ldv at altlinux.org
Mon Feb 11 23:00:28 UTC 2013
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/pam-developers/attachments/20130212/d4f93595/attachment.sig>
More information about the Pam-developers
mailing list