[Pam-developers] [PATCH] pam_tty_audit: add an option to control logging of passwords: log_passwd
Richard Guy Briggs
rgb at redhat.com
Tue Jun 11 15:30:43 UTC 2013
On Mon, Jun 10, 2013 at 04:59:37PM -0400, Richard Guy Briggs wrote:
> On Wed, Jun 05, 2013 at 02:54:09AM +0400, Dmitry V. Levin wrote:
> > On Thu, May 23, 2013 at 10:29:59AM -0400, Richard Guy Briggs wrote:
> > > Most commands are entered one line at a time and processed as complete lines
> > > in non-canonical mode. Commands that interactively require a password, enter
> > > canonical mode with echo set to off to do this. This feature (icanon and
> > > !echo) can be used to avoid logging passwords by audit while still logging the
> > > rest of the command.
> > >
> > > Adding a member to the struct audit_tty_status passed in by pam_tty_audit
> > > allows control of logging passwords per task.
> >
> > Sorry for the long delay with review. Please see my comments below.
>
> Ditto...
Please find a new patch at the end...
> > > --- a/configure.in
> > > +++ b/configure.in
> > > @@ -386,6 +386,19 @@ if test x"$WITH_LIBAUDIT" != xno ; then
> > > fi
> > > if test ! -z "$HAVE_AUDIT_TTY_STATUS" ; then
> > > AC_DEFINE([HAVE_AUDIT_TTY_STATUS], 1, [Define to 1 if struct audit_tty_status exists.])
> > > +
> > > + AC_CHECK_MEMBER(
> > > + [struct audit_tty_status.log_passwd],
> > > + [
> > > + HAVE_AUDIT_TTY_STATUS_LOG_PASSWD=yes
> > > + AC_DEFINE([HAVE_AUDIT_TTY_STATUS_LOG_PASSWD], 1, [Define to 1 if struct audit_tty_status.log_passwd exists.])
> > > + ],
> > > + [
> > > + HAVE_AUDIT_TTY_STATUS_LOG_PASSWD=""
> > > + AC_MSG_WARN([The struct audit_tty_status.log_passwd member is needed for the log_passwd option. The log_passwd option is disabled.])
> > > + ],
> > > + [[#include <libaudit.h>]]
> > > + )
> > > fi
> > > else
> > > LIBAUDIT=""
> > > @@ -393,6 +406,8 @@ fi
> > > AC_SUBST(LIBAUDIT)
> > > AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS],
> > > [test "x$HAVE_AUDIT_TTY_STATUS" = xyes])
> > > +AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS_LOG_PASSWD],
> > > + [test "x$HAVE_AUDIT_TTY_STATUS_LOG_PASSWD" = xyes])
> >
> > There is a shorter way to express this idea:
> >
> > AC_CHECK_MEMBER([struct audit_tty_status.log_passwd], [],
> > AC_MSG_WARN([audit_tty_status.log_passwd is not available, log_passwd option disabled.],
> > [[#include <libaudit.h>]])
> > ...
> > AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS_LOG_PASSWD],
> > [test "x$ac_cv_member_audit_tty_status_log_passwd" = xyes])
>
> Ok, so $ac_cv_member_audit_tty_status_log_passwd is set by
> AC_CHECK_MEMBER()?
Ok, I've used your suggestion, but switching to AC_CHECK_MEMBERS() (and
balancing the AC_MSG_WARN() parens).
> > > --- a/modules/pam_tty_audit/Makefile.am
> > > +++ b/modules/pam_tty_audit/Makefile.am
> > > @@ -16,6 +16,9 @@ XMLS = README.xml pam_tty_audit.8.xml
> > > securelibdir = $(SECUREDIR)
> > >
> > > AM_CFLAGS = -I$(top_srcdir)/libpam/include -I$(top_srcdir)/libpamc/include
> > > +if HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
> > > + AM_CFLAGS += -DHAVE_AUDIT_TTY_STATUS_LOG_PASSWD
> > > +endif
> >
> > This shouldn't be needed because of the side effect of AC_CHECK_MEMBER.
>
> I don't follow. I found I needed this latter one because
> HAVE_AUDIT_TTY_STATUS_LOG_PASSWD wasn't being propagated to gcc when
> compiling the C module.
Removed due to feature/side effect of AC_CHECK_MEMBERS().
> > > + else if (strcmp (argv[i], "log_passwd") == 0)
> > > +#ifdef HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
> > > + log_passwd = 1;
> > > +#else /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
> > > + pam_syslog (pamh, LOG_WARNING,
> > > + "pam_tty_audit: The log_passwd option was not available at compile time.");
> >
> > No need to prefix syslog messages with the module name,
> > you can rely on pam_syslog.
>
> Thanks.
Removed.
> > > +#warning "pam_tty_audit: The log_passwd option is not available. Please upgrade your kernel."
> >
> > I'm not sure the wording is correct: it's headers not the kernel
> > that is subject of the configure check.
>
> I pondered this wording. I originally used the header wording, but
> thought it better to refer to the kernel, presuming the header would be
> upgraded with a capable kernel.
I've changed the wording to "Please upgrade your headers/kernel."
Thanks for your feedback Dmitry.
> > --
> > ldv
From: Richard Guy Briggs <rgb at redhat.com>
Date: Thu, 21 Mar 2013 00:56:51 -0400
Subject: [PATCH] pam_tty_audit: add an option to control logging of passwords: log_passwd
Most commands are entered one line at a time and processed as complete lines
in non-canonical mode. Commands that interactively require a password, enter
canonical mode with echo set to off to do this. This feature (icanon and
!echo) can be used to avoid logging passwords by audit while still logging the
rest of the command.
Adding a member to the struct audit_tty_status passed in by pam_tty_audit
allows control of logging passwords per task.
This can be used with older kernels since it checks for the needed structure
members at compile time.
Signed-off-by: Richard Guy Briggs <rgb at redhat.com>
---
configure.in | 6 ++++++
modules/pam_tty_audit/pam_tty_audit.8.xml | 15 +++++++++++++++
modules/pam_tty_audit/pam_tty_audit.c | 23 ++++++++++++++++++++++-
3 files changed, 43 insertions(+), 1 deletions(-)
diff --git a/configure.in b/configure.in
index 515b301..b92d9ac 100644
--- a/configure.in
+++ b/configure.in
@@ -386,6 +386,10 @@ if test x"$WITH_LIBAUDIT" != xno ; then
fi
if test ! -z "$HAVE_AUDIT_TTY_STATUS" ; then
AC_DEFINE([HAVE_AUDIT_TTY_STATUS], 1, [Define to 1 if struct audit_tty_status exists.])
+
+ AC_CHECK_MEMBERS([struct audit_tty_status.log_passwd], [],
+ AC_MSG_WARN([audit_tty_status.log_passwd is not available. The log_passwd option is disabled.]),
+ [[#include <libaudit.h>]])
fi
else
LIBAUDIT=""
@@ -393,6 +397,8 @@ fi
AC_SUBST(LIBAUDIT)
AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS],
[test "x$HAVE_AUDIT_TTY_STATUS" = xyes])
+AM_CONDITIONAL([HAVE_AUDIT_TTY_STATUS_LOG_PASSWD],
+ [test "x$ac_cv_member_audit_tty_status_log_passwd" = xyes])
AC_CHECK_HEADERS(xcrypt.h crypt.h)
AS_IF([test "x$ac_cv_header_xcrypt_h" = "xyes"],
diff --git a/modules/pam_tty_audit/pam_tty_audit.8.xml b/modules/pam_tty_audit/pam_tty_audit.8.xml
index 447b845..552353c 100644
--- a/modules/pam_tty_audit/pam_tty_audit.8.xml
+++ b/modules/pam_tty_audit/pam_tty_audit.8.xml
@@ -77,6 +77,19 @@
</para>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>
+ <option>log_passwd</option>
+ </term>
+ <listitem>
+ <para>
+ Log keystrokes when ECHO mode is off but ICANON mode is active.
+ This is the mode in which the tty is placed during password entry.
+ By default, passwords are not logged. This option may not be
+ available on older kernels (3.9?).
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</refsect1>
@@ -161,6 +174,8 @@ session required pam_tty_audit.so disable=* enable=root
<para>
pam_tty_audit was written by Miloslav Trmač
<mitr at redhat.com>.
+ The log_passwd option was added by Richard Guy Briggs
+ <rgb at redhat.com>.
</para>
</refsect1>
diff --git a/modules/pam_tty_audit/pam_tty_audit.c b/modules/pam_tty_audit/pam_tty_audit.c
index 080f495..a3b590d 100644
--- a/modules/pam_tty_audit/pam_tty_audit.c
+++ b/modules/pam_tty_audit/pam_tty_audit.c
@@ -201,6 +201,9 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char **argv)
struct audit_tty_status *old_status, new_status;
const char *user;
int i, fd, open_only;
+#ifdef HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
+ int log_passwd;
+#endif /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
(void)flags;
@@ -212,6 +215,9 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char **argv)
command = CMD_NONE;
open_only = 0;
+#ifdef HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
+ log_passwd = 0;
+#endif /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
for (i = 0; i < argc; i++)
{
if (strncmp (argv[i], "enable=", 7) == 0
@@ -237,6 +243,14 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char **argv)
}
else if (strcmp (argv[i], "open_only") == 0)
open_only = 1;
+ else if (strcmp (argv[i], "log_passwd") == 0)
+#ifdef HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
+ log_passwd = 1;
+#else /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
+ pam_syslog (pamh, LOG_WARNING,
+ "The log_passwd option was not available at compile time.");
+#warning "pam_tty_audit: The log_passwd option is not available. Please upgrade your headers/kernel."
+#endif /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
else
{
pam_syslog (pamh, LOG_ERR, "unknown option `%s'", argv[i]);
@@ -262,7 +276,14 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char **argv)
}
new_status.enabled = (command == CMD_ENABLE ? 1 : 0);
- if (old_status->enabled == new_status.enabled)
+#ifdef HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
+ new_status.log_passwd = log_passwd;
+#endif /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
+ if (old_status->enabled == new_status.enabled
+#ifdef HAVE_AUDIT_TTY_STATUS_LOG_PASSWD
+ && old_status->log_passwd == new_status.log_passwd
+#endif /* HAVE_AUDIT_TTY_STATUS_LOG_PASSWD */
+ )
{
open_only = 1; /* to clean up old_status */
goto ok_fd;
--
1.7.1
> - RGB
- RGB
--
Richard Guy Briggs <rbriggs at redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: 1.647.777.2635
Internal: (81) 32635
More information about the Linux-audit
mailing list