[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