Add grantor field to audit records

[Tomas Mraz]

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,

[Tomas Mraz]

On Čt, 2014-07-24 at 17:29 +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,

Ping? Had anybody chance to review the patch?

[Tomas Mraz]

On Po, 2014-08-04 at 18:28 +0200, Tomas Mraz wrote:
> On Čt, 2014-07-24 at 17:29 +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,
>
> Ping? Had anybody chance to review the patch?

Ping again? If nobody speaks up, I'd like to commit this patch soon.

[Dmitry V. Levin]

On Wed, Aug 13, 2014 at 05:53:55PM +0200, Tomas Mraz wrote:
> On Po, 2014-08-04 at 18:28 +0200, Tomas Mraz wrote:
> > On Čt, 2014-07-24 at 17:29 +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,
> >
> > Ping? Had anybody chance to review the patch?
>
> Ping again? If nobody speaks up, I'd like to commit this patch soon.

There are minor issues with some hunks of the patch, but I'd like to
review it as a whole, and it takes time.  Could it wait a bit more, say,
till the end of this week?

[Tomas Mraz]

On Čt, 2014-08-14 at 15:57 +0400, Dmitry V. Levin wrote:
> On Wed, Aug 13, 2014 at 05:53:55PM +0200, Tomas Mraz wrote:
> > On Po, 2014-08-04 at 18:28 +0200, Tomas Mraz wrote:
> > > On Čt, 2014-07-24 at 17:29 +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,
> > >
> > > Ping? Had anybody chance to review the patch?
> >
> > Ping again? If nobody speaks up, I'd like to commit this patch soon.
>
> There are minor issues with some hunks of the patch, but I'd like to
> review it as a whole, and it takes time.  Could it wait a bit more, say,
> till the end of this week?

Yes, sure, I just wanted to know whether anyone is looking at it.

[Tomas Mraz]

On Čt, 2014-08-14 at 15:57 +0400, Dmitry V. Levin wrote:
> On Wed, Aug 13, 2014 at 05:53:55PM +0200, Tomas Mraz wrote:
> > On Po, 2014-08-04 at 18:28 +0200, Tomas Mraz wrote:
> > > On Čt, 2014-07-24 at 17:29 +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,
> > >
> > > Ping? Had anybody chance to review the patch?
> >
> > Ping again? If nobody speaks up, I'd like to commit this patch soon.
>
> There are minor issues with some hunks of the patch, but I'd like to
> review it as a whole, and it takes time.  Could it wait a bit more, say,
> till the end of this week?

Would you have the time for the review now?

[Dmitry V. Levin]

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
> @@ -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.
> 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.

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).
> 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.
> }
> 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.
> 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
> 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.
> +}
>
> /*
> This function translates the module dispatch request into a pointer
> to the stack of modules that will actually be run.  the
>
> @@ -318,21 +332,21 @@ decision_made:     /* by getting  here we have made a decision */
>  int _pam_dispatch(pam_handle_t *pamh, int flags, int choice)
>  {
>      struct handler *h = NULL;
> int retval, use_cached_chain;
>
> int retval = PAM_SYSTEM_ERR, use_cached_chain;
> _pam_boolean resumed;
>
> IF_NO_PAMH("_pam_dispatch", pamh, PAM_SYSTEM_ERR);
>
> if (__PAM_FROM_MODULE(pamh)) {
> D(("called from a module!?"));
> return PAM_SYSTEM_ERR;
>
> goto end;
> }
>
> /* Load all modules, resolve all symbols */
>
> if ((retval = _pam_init_handlers(pamh)) != PAM_SUCCESS) {
> pam_syslog(pamh, LOG_ERR, "unable to dispatch function");
> return retval;
>
> goto end;
> }
>
> use_cached_chain = _PAM_PLEASE_FREEZE;
>
> @@ -360,7 +374,8 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice)
>    break;
>      default:
>    pam_syslog(pamh, LOG_ERR, "undefined fn choice; %d", choice);
> return PAM_ABORT;
>
> retval = PAM_ABORT;
>
> goto end;
> }
>
> if (h == NULL) {     /* there was no handlers.conf... entry; will use
>
> @@ -393,11 +408,13 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice)
>        pam_syslog(pamh, LOG_ERR,
>    		    "application failed to re-exec stack [%d:%d]",
>    		    pamh->former.choice, choice);
>    return PAM_ABORT;
>    retval = PAM_ABORT;
>    goto end;
>
> }
>  resumed = PAM_TRUE;
>    } else {
>  resumed = PAM_FALSE;
>
> _pam_clear_grantors(h);
> }
>
> __PAM_TO_MODULE(pamh);
>
> @@ -417,5 +434,13 @@ int _pam_dispatch(pam_handle_t *pamh, int flags, int choice)
>    pamh->former.choice = PAM_NOT_STACKED;
>      }
>
> +end:
>
> +#ifdef HAVE_LIBAUDIT
> if (choice != PAM_CHAUTHTOK || flags & PAM_UPDATE_AUTHTOK || retval != PAM_SUCCESS) {
> retval = _pam_auditlog(pamh, choice, retval, flags, h);
> }
>
> +#endif
> return retval;
>
> }
> diff --git a/libpam/pam_handlers.c b/libpam/pam_handlers.c
> index 02714f7..e3f8ff6 100644
> --- a/libpam/pam_handlers.c
> +++ b/libpam/pam_handlers.c
> @@ -889,6 +889,7 @@ int _pam_add_handler(pam_handle_t *pamh
>      (*handler_p)->argc = argc;
>      (*handler_p)->argv = argv;                       /* not a copy */
>      (*handler_p)->mod_name = extract_modulename(mod_path);
>
> (*handler_p)->grantor = 0;
> (*handler_p)->next = NULL;
>
> /* some of the modules have a second calling function */
>
> @@ -921,6 +922,7 @@ int _pam_add_handler(pam_handle_t *pamh
>        (*handler_p2)->argv = NULL;              /* no arguments */
>    }
>    (*handler_p2)->mod_name = extract_modulename(mod_path);
> (*handler_p2)->grantor = 0;
> (*handler_p2)->next = NULL;
> }
>
> diff --git a/libpam/pam_password.c b/libpam/pam_password.c
> index 75db5e5..592e01f 100644
> --- a/libpam/pam_password.c
> +++ b/libpam/pam_password.c
> @@ -57,9 +57,5 @@ int pam_chauthtok(pam_handle_t *pamh, int flags)
>    D(("will resume when ready", retval));
>      }
>
> -#ifdef HAVE_LIBAUDIT
> retval = _pam_auditlog(pamh, PAM_CHAUTHTOK, retval, flags);
>
> -#endif
> return retval;
>
> }
> diff --git a/libpam/pam_private.h b/libpam/pam_private.h
> index 134dc72..d93283c 100644
> --- a/libpam/pam_private.h
> +++ b/libpam/pam_private.h
> @@ -55,6 +55,7 @@ struct handler {
>      struct handler *next;
>      char *mod_name;
>      int stack_level;
> int grantor;
>
> };
>
> #define PAM_HT_MODULE       0
> @@ -316,7 +317,7 @@ if ((pamh) == NULL) {                             \
>          do { (pamh)->caller_is = _PAM_CALLED_FROM_APP; } while (0)
>
> #ifdef HAVE_LIBAUDIT
> -extern int _pam_auditlog(pam_handle_t *pamh, int action, int retval, int flags);
> +extern int _pam_auditlog(pam_handle_t *pamh, int action, int retval, int flags, struct handler *h);
>  extern int _pam_audit_end(pam_handle_t *pamh, int pam_status);
>  #endif
>
> diff --git a/libpam/pam_session.c b/libpam/pam_session.c
> index 512153f..cb393c1 100644
> --- a/libpam/pam_session.c
> +++ b/libpam/pam_session.c
> @@ -22,9 +22,6 @@ int pam_open_session(pam_handle_t *pamh, int flags)
>      }
>      retval = _pam_dispatch(pamh, flags, PAM_OPEN_SESSION);
>
> -#ifdef HAVE_LIBAUDIT
> retval = _pam_auditlog(pamh, PAM_OPEN_SESSION, retval, flags);
>
> -#endif
>      return retval;
>  }
>
> @@ -43,10 +40,6 @@ int pam_close_session(pam_handle_t *pamh, int flags)
>  retval = _pam_dispatch(pamh, flags, PAM_CLOSE_SESSION);
>
> -#ifdef HAVE_LIBAUDIT
> retval = _pam_auditlog(pamh, PAM_CLOSE_SESSION, retval, flags);
>
> -#endif
> return retval;
>
> }

[Tomas Mraz]

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.

[Dmitry V. Levin]

On Wed, Sep 03, 2014 at 10:30:26AM +0200, Tomas Mraz wrote:
> 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:

[...]
> > > @@ -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.

Still there is a slight chance of _pam_list_grantors returning NULL, so
there would be two kinds of messages, with and without mentioning
grantors, anyway.  For me "grantor=?" in the case when there are no
grantors just looks confusing.

[Richard Guy Briggs]

On 14/09/03, Dmitry V. Levin wrote:
> On Wed, Sep 03, 2014 at 10:30:26AM +0200, Tomas Mraz wrote:
> > 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:
>
> [...]
> > > > @@ -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.
>
> Still there is a slight chance of _pam_list_grantors returning NULL, so
> there would be two kinds of messages, with and without mentioning
> grantors, anyway.  For me "grantor=?" in the case when there are no
> grantors just looks confusing.

In that case, Steve Grubb has requested something like "grantor=(none)"
so that there are no disappearing fields in audit records.
> ldv

- RGB

--
Richard Guy Briggs 
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

[Dmitry V. Levin]

On Wed, Sep 03, 2014 at 08:45:20AM -0400, Richard Guy Briggs wrote:
> On 14/09/03, Dmitry V. Levin wrote:
> > On Wed, Sep 03, 2014 at 10:30:26AM +0200, Tomas Mraz wrote:
> > > 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:
> >
> > [...]
> > > > > @@ -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.
> >
> > Still there is a slight chance of _pam_list_grantors returning NULL, so
> > there would be two kinds of messages, with and without mentioning
> > grantors, anyway.  For me "grantor=?" in the case when there are no
> > grantors just looks confusing.
>
> In that case, Steve Grubb has requested something like "grantor=(none)"
> so that there are no disappearing fields in audit records.

No, mod_name can be almost anything, including "?", "(none)", and even "".
It doesn't contain spaces, though, so "grantor= " would be less confusing,
but how you are going to distinguish "grantor=" (grantor is a module
called ".so") from "grantor= " (no grantors) is beyond my imagination.

[Tomas Mraz]

On St, 2014-09-03 at 17:02 +0400, Dmitry V. Levin wrote:
> On Wed, Sep 03, 2014 at 08:45:20AM -0400, Richard Guy Briggs wrote:
> > On 14/09/03, Dmitry V. Levin wrote:
> > > On Wed, Sep 03, 2014 at 10:30:26AM +0200, Tomas Mraz wrote:
> > > > 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:
> > >
> > > [...]
> > > > > > @@ -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.
> > >
> > > Still there is a slight chance of _pam_list_grantors returning NULL, so
> > > there would be two kinds of messages, with and without mentioning
> > > grantors, anyway.  For me "grantor=?" in the case when there are no
> > > grantors just looks confusing.
> >
> > In that case, Steve Grubb has requested something like "grantor=(none)"
> > so that there are no disappearing fields in audit records.
>
> No, mod_name can be almost anything, including "?", "(none)", and even "".
> It doesn't contain spaces, though, so "grantor= " would be less confusing,
> but how you are going to distinguish "grantor=" (grantor is a module
> called ".so") from "grantor= " (no grantors) is beyond my imagination.

Well at least the pam module name cannot be /.so but grantor=/ could be
confusing as well. I'll stick with the original grantor=? as supporting
module named ?.so is something we do not really have to :).

So, I'll rather change the patch so there is no possibility of the
message not having the grantor field than remove the grantor=? case
completely.

The attached patch should contain this change and all the other minor
corrections.

[Dmitry V. Levin]

On Wed, Sep 03, 2014 at 03:33:57PM +0200, Tomas Mraz wrote:
[...]
> Well at least the pam module name cannot be /.so but grantor=/ could be
> confusing as well. I'll stick with the original grantor=? as supporting
> module named ?.so is something we do not really have to :).

Well, I've never had a module named ?.so before, but I have it now,
and it works perfectly well. :)
> So, I'll rather change the patch so there is no possibility of the
> message not having the grantor field than remove the grantor=? case
> completely.

OK, maybe call it "/?", so that the name will be distinguishable from
supported module names?
> The attached patch should contain this change and all the other minor
> corrections.

[...]
> if (asprintf(&buf, "PAM:%s%s%s", message, grantors?" grantors=":"", grantors?grantors:"") >= 0) {

Could you kindly make this expression more readable, please?

[...]
> if (_pam_audit_writelog(pamh, audit_fd, type, message, grantors?grantors:"?", retval) < 0)

And this one, too.

[Tomas Mraz]

On St, 2014-09-03 at 18:07 +0400, Dmitry V. Levin wrote:
> On Wed, Sep 03, 2014 at 03:33:57PM +0200, Tomas Mraz wrote:
> [...]
> > Well at least the pam module name cannot be /.so but grantor=/ could be
> > confusing as well. I'll stick with the original grantor=? as supporting
> > module named ?.so is something we do not really have to :).
>
> Well, I've never had a module named ?.so before, but I have it now,
> and it works perfectly well. :)
> > So, I'll rather change the patch so there is no possibility of the
> > message not having the grantor field than remove the grantor=? case
> > completely.
>
> OK, maybe call it "/?", so that the name will be distinguishable from
> supported module names?
> > The attached patch should contain this change and all the other minor
> > corrections.
>
> [...]
> > if (asprintf(&buf, "PAM:%s%s%s", message, grantors?" grantors=":"", grantors?grantors:"") >= 0) {
>
> Could you kindly make this expression more readable, please?

Do you accept just whitespace changes or do you want me also to replace
the ? expressions with ifs and variable assignments?
> [...]
> > if (_pam_audit_writelog(pamh, audit_fd, type, message, grantors?grantors:"?", retval) < 0)
>
> And this one, too.
>
> Pam-developers mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/pam-developers

[Dmitry V. Levin]

On Wed, Sep 03, 2014 at 04:41:40PM +0200, Tomas Mraz wrote:
> On St, 2014-09-03 at 18:07 +0400, Dmitry V. Levin wrote:
> > On Wed, Sep 03, 2014 at 03:33:57PM +0200, Tomas Mraz wrote:
> > [...]
> > > Well at least the pam module name cannot be /.so but grantor=/ could be
> > > confusing as well. I'll stick with the original grantor=? as supporting
> > > module named ?.so is something we do not really have to :).
> >
> > Well, I've never had a module named ?.so before, but I have it now,
> > and it works perfectly well. :)
> > > So, I'll rather change the patch so there is no possibility of the
> > > message not having the grantor field than remove the grantor=? case
> > > completely.
> >
> > OK, maybe call it "/?", so that the name will be distinguishable from
> > supported module names?
> > > The attached patch should contain this change and all the other minor
> > > corrections.
> >
> > [...]
> > > if (asprintf(&buf, "PAM:%s%s%s", message, grantors?" grantors=":"", grantors?grantors:"") >= 0) {
> >
> > Could you kindly make this expression more readable, please?
>
> Do you accept just whitespace changes or do you want me also to replace
> the ? expressions with ifs and variable assignments?

Whatever you like, just don't make new lines too long or too dense to read.

[Tomas Mraz]

On St, 2014-09-03 at 18:07 +0400, Dmitry V. Levin wrote:
> On Wed, Sep 03, 2014 at 03:33:57PM +0200, Tomas Mraz wrote:
> [...]
> > Well at least the pam module name cannot be /.so but grantor=/ could be
> > confusing as well. I'll stick with the original grantor=? as supporting
> > module named ?.so is something we do not really have to :).
>
> Well, I've never had a module named ?.so before, but I have it now,
> and it works perfectly well. :)

But should it? What if we rejected such module name explicitly? Would
you accept that?

[Dmitry V. Levin]

On Wed, Sep 03, 2014 at 04:45:17PM +0200, Tomas Mraz wrote:
> On St, 2014-09-03 at 18:07 +0400, Dmitry V. Levin wrote:
> > On Wed, Sep 03, 2014 at 03:33:57PM +0200, Tomas Mraz wrote:
> > [...]
> > > Well at least the pam module name cannot be /.so but grantor=/ could be
> > > confusing as well. I'll stick with the original grantor=? as supporting
> > > module named ?.so is something we do not really have to :).
> >
> > Well, I've never had a module named ?.so before, but I have it now,
> > and it works perfectly well. :)
>
> But should it? What if we rejected such module name explicitly? Would
> you accept that?

As we don't pass mod_name to glob(3), fnmatch(3), wordexp(3), etc.,
there is no risk to accept a module named "?.so".
So what would be the rationale for this change?

Does "?.so" look too suspicious for a valid pam module name? :)

[Tomas Mraz]

On St, 2014-09-03 at 18:59 +0400, Dmitry V. Levin wrote:
> On Wed, Sep 03, 2014 at 04:45:17PM +0200, Tomas Mraz wrote:
> > On St, 2014-09-03 at 18:07 +0400, Dmitry V. Levin wrote:
> > > On Wed, Sep 03, 2014 at 03:33:57PM +0200, Tomas Mraz wrote:
> > > [...]
> > > > Well at least the pam module name cannot be /.so but grantor=/ could be
> > > > confusing as well. I'll stick with the original grantor=? as supporting
> > > > module named ?.so is something we do not really have to :).
> > >
> > > Well, I've never had a module named ?.so before, but I have it now,
> > > and it works perfectly well. :)
> >
> > But should it? What if we rejected such module name explicitly? Would
> > you accept that?
>
> As we don't pass mod_name to glob(3), fnmatch(3), wordexp(3), etc.,
> there is no risk to accept a module named "?.so".
> So what would be the rationale for this change?

That it confuses the audit trail?
> Does "?.so" look too suspicious for a valid pam module name? :)

And that as well.

[Dmitry V. Levin]

On Wed, Sep 03, 2014 at 05:13:55PM +0200, Tomas Mraz wrote:
> On St, 2014-09-03 at 18:59 +0400, Dmitry V. Levin wrote:
> > On Wed, Sep 03, 2014 at 04:45:17PM +0200, Tomas Mraz wrote:
> > > On St, 2014-09-03 at 18:07 +0400, Dmitry V. Levin wrote:
> > > > On Wed, Sep 03, 2014 at 03:33:57PM +0200, Tomas Mraz wrote:
> > > > [...]
> > > > > Well at least the pam module name cannot be /.so but grantor=/ could be
> > > > > confusing as well. I'll stick with the original grantor=? as supporting
> > > > > module named ?.so is something we do not really have to :).
> > > >
> > > > Well, I've never had a module named ?.so before, but I have it now,
> > > > and it works perfectly well. :)
> > >
> > > But should it? What if we rejected such module name explicitly? Would
> > > you accept that?
> >
> > As we don't pass mod_name to glob(3), fnmatch(3), wordexp(3), etc.,
> > there is no risk to accept a module named "?.so".
> > So what would be the rationale for this change?
>
> That it confuses the audit trail?
> > Does "?.so" look too suspicious for a valid pam module name? :)
>
> And that as well.

Fair enough.