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.