On St, 2014-09-03 at 07:52 +0400, Dmitry V. Levin wrote:
On Thu, Jul 24, 2014 at 05:29:50PM +0200, Tomas Mraz wrote:
> Hi,
>
> this patch modifies the audit records of libpam to add a grantor field.
> This field records in case of the successful authentication (or any
> other successful pam module call) that the module granted the access.
> The PAM_SUCCESS return is recorded only for cases where the module
> really contributed to the access being granted. That means that if the
> action for success is different from ok and done, the module is not
> recorded.
>
> Please review, thanks,
Sorry for the long delay, see my comments below.
> diff --git a/libpam/pam_account.c b/libpam/pam_account.c
> index 572acc4..3a4fb1f 100644
> --- a/libpam/pam_account.c
> +++ b/libpam/pam_account.c
> @@ -19,9 +19,5 @@ int pam_acct_mgmt(pam_handle_t *pamh, int flags)
>
> retval = _pam_dispatch(pamh, flags, PAM_ACCOUNT);
>
> -#ifdef HAVE_LIBAUDIT
> - retval = _pam_auditlog(pamh, PAM_ACCOUNT, retval, flags);
> -#endif
> -
> return retval;
> }
> diff --git a/libpam/pam_audit.c b/libpam/pam_audit.c
> index 531746a..63a4ea5 100644
> --- a/libpam/pam_audit.c
> +++ b/libpam/pam_audit.c
First of all, libpam/pam_audit.c must include "pam_private.h" prior to all
other header files, otherwise some vital system header files are included
before config.h
OK, I will fix that
> @@ -28,14 +28,15 @@ _pam_audit_writelog(pam_handle_t *pamh, int
audit_fd, int type,
> const char *message, int retval)
> {
> static int old_errno = -1;
> - int rc;
> - char buf[32];
> -
> - snprintf(buf, sizeof(buf), "PAM:%s", message);
> -
> - rc = audit_log_acct_message (audit_fd, type, NULL, buf,
> - (retval != PAM_USER_UNKNOWN && pamh->user) ? pamh->user :
"?",
> - -1, pamh->rhost, NULL, pamh->tty, retval == PAM_SUCCESS );
> + int rc = -ENOMEM;
> + char *buf = NULL;
minor: no need to initialize buf before passing it to asprintf:
buf can be relied upon iff asprintf succeeded.
OK, I will fix that
> +
> + if (asprintf(&buf, "PAM:%s", message) >= 0) {
> + rc = audit_log_acct_message(audit_fd, type, NULL, buf,
> + (retval != PAM_USER_UNKNOWN && pamh->user) ? pamh->user :
"?",
> + -1, pamh->rhost, NULL, pamh->tty, retval == PAM_SUCCESS);
> + free(buf);
> + }
>
> /* libaudit sets errno to his own negative error code. This can be
> an official errno number, but must not. It can also be a audit
> @@ -78,12 +79,65 @@ _pam_audit_open(pam_handle_t *pamh)
> return audit_fd;
> }
>
> +static char *
> +_pam_list_grantors(struct handler *hlist, const char *message, int retval)
> +{
> + char *buf;
> + char *list = NULL;
> +
> + if (retval == PAM_SUCCESS) {
> + struct handler *h;
> + char *p = NULL;
> + size_t len = 0;
> +
> + h = hlist;
> +
> + while (h != NULL) {
> + if (h->grantor) {
> + len += strlen(h->mod_name) + 1;
> + }
> +
> + h = h->next;
> + }
minor: this is exactly the case where a "for" expression is more suitable.
OK, I'll change that.
major: please check for len == 0, otherwise you'll sooner or
later end up
with malloc(0) with subsequent garbage passed to asprintf. All you need
to reproduce this is an almost empty auth stack like this
auth [success=1 default=ignore] pam_permit.so
auth required pam_permit.so
and an application that calls pam_setcred without pam_authenticate
(as sshd often does).
Actually this cannot happen because with your configuration the retval
passed in will not be PAM_SUCCESS. The default return value is
PAM_PERM_DENIED and it is not changed on jumps - that means the
success=1 will not change it to PAM_SUCCESS. Nevertheless I'll add the
check for len == 0 as a defensive programming measure.
> +
> + list = malloc(len);
> + if (list == NULL) {
> + return NULL;
> + }
> +
> + h = hlist;
> +
> + while (h != NULL) {
> + if (h->grantor) {
> + if (p == NULL) {
> + p = list;
> + } else {
> + p = stpcpy(p, ",");
> + }
> +
> + p = stpcpy(p, h->mod_name);
> + }
> +
> + h = h->next;
> + }
minor: this is another case where a "for" expression would be appropriate.
OK, I'll change that.
> + }
> +
> + if (asprintf(&buf, "%s grantor=%s", message, list ? list :
"?") < 0) {
> + free(list);
> + return NULL;
> + }
> +
> + free(list);
> + return buf;
> +}
> +
> int
> -_pam_auditlog(pam_handle_t *pamh, int action, int retval, int flags)
> +_pam_auditlog(pam_handle_t *pamh, int action, int retval, int flags, struct handler
*h)
> {
> const char *message;
> int type;
> int audit_fd;
> + char *buf = NULL;
>
> if ((audit_fd=_pam_audit_open(pamh)) == -1) {
> return PAM_SYSTEM_ERR;
> @@ -134,9 +188,18 @@ _pam_auditlog(pam_handle_t *pamh, int action, int retval, int
flags)
> retval = PAM_SYSTEM_ERR;
> }
>
> - if (_pam_audit_writelog(pamh, audit_fd, type, message, retval) < 0)
> + buf = _pam_list_grantors(h, message, retval);
> + if (buf == NULL) {
> + /* allocation failure */
> + pam_syslog(pamh, LOG_CRIT, "_pam_list_grantors() failed: %m");
> + retval = PAM_SYSTEM_ERR;
> + }
Since there are no grantors when retval != PAM_SUCCESS, we can make
_pam_list_grantors simpler and save on extra asprintf/free by calling
_pam_list_grantors only if retval == PAM_SUCCESS.
No need to print "grantor=?" when there are no grantors.
This was explicitly requested to make the audit record more orthogonal.
So I will keep it as is.
> +
> + if (_pam_audit_writelog(pamh, audit_fd, type, buf ? buf : message, retval) <
0)
> retval = PAM_SYSTEM_ERR;
>
> + free(buf);
> +
> audit_close(audit_fd);
> return retval;
> }
> @@ -149,7 +212,7 @@ _pam_audit_end(pam_handle_t *pamh, int status UNUSED)
> * stacks having been run. Assume that this is sshd faking
> * things for an unknown user.
> */
> - _pam_auditlog(pamh, _PAM_ACTION_DONE, PAM_USER_UNKNOWN, 0);
> + _pam_auditlog(pamh, _PAM_ACTION_DONE, PAM_USER_UNKNOWN, 0, NULL);
> }
>
> return 0;
> diff --git a/libpam/pam_auth.c b/libpam/pam_auth.c
> index 5984fa5..1e7bc6e 100644
> --- a/libpam/pam_auth.c
> +++ b/libpam/pam_auth.c
> @@ -45,10 +45,6 @@ int pam_authenticate(pam_handle_t *pamh, int flags)
> prelude_send_alert(pamh, retval);
> #endif
>
> -#ifdef HAVE_LIBAUDIT
> - retval = _pam_auditlog(pamh, PAM_AUTHENTICATE, retval, flags);
> -#endif
> -
> return retval;
> }
>
> @@ -71,10 +67,6 @@ int pam_setcred(pam_handle_t *pamh, int flags)
>
> retval = _pam_dispatch(pamh, flags, PAM_SETCRED);
>
> -#ifdef HAVE_LIBAUDIT
> - retval = _pam_auditlog(pamh, PAM_SETCRED, retval, flags);
> -#endif
> -
> D(("pam_setcred exit"));
>
> return retval;
> diff --git a/libpam/pam_dispatch.c b/libpam/pam_dispatch.c
> index eb52c82..ccfc372 100644
> --- a/libpam/pam_dispatch.c
> +++ b/libpam/pam_dispatch.c
> @@ -217,8 +217,14 @@ static int _pam_dispatch_aux(pam_handle_t *pamh, int flags,
struct handler *h,
> status = retval;
> }
> }
> - if ( impression == _PAM_POSITIVE && action == _PAM_ACTION_DONE ) {
> - goto decision_made;
> + if ( impression == _PAM_POSITIVE ) {
> + if ( retval == PAM_SUCCESS ) {
> + h->grantor = 1;
> + }
Technically, this is the only place where grantor has to be set,
but in practice there are many configs like
auth [success=1 default=ignore] pam_unix.so ...
auth required pam_something_else.so
In such cases, actual grantors like pam_unix will not be registered
because technically they are not grantors.
A workaround is to rewrite configs this way:
auth [success=ok default=1] pam_unix.so ...
auth [success=1] pam_permit.so
auth required pam_something_else.so
As I wrote above the first configuration requires some grantor module
after the pam_something_else.so anyway, otherwise the stack will still
keep the PAM_PERM_DENIED initial value. So the pam_unix.so is not really
grantor in the first case.
> +
> + if ( action == _PAM_ACTION_DONE ) {
> + goto decision_made;
> + }
> }
> break;
>
> @@ -308,6 +314,14 @@ decision_made: /* by getting here we have made a decision
*/
> return status;
> }
>
> +static void _pam_clear_grantors(struct handler *h)
> +{
> + while (h != NULL) {
> + h->grantor = 0;
> + h = h->next;
> + }
minor: this is another case where a "for" expression would be appropriate.
OK, I'll change that.
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
(You'll never know whether the road is wrong though.)