12:46 a.m.
Hi folks,
There's been a couple of requests to add a switch to pam_tty_audit to
*not* log passwords when logging user commands.
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 to do this. This feature (icanon) 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 canonical mode per task.
Note: The original patch added a sysctl to control this system-wide,
which did work fine as expected, but it was recommended to keep the
switch with the module invocation, turning it into a per-task switch.
This method has also been tested.
Here are two patches, the first to pam to add the switch to
the pam_tty_audit module. The second is to the kernel to add the
necessary bits in audit and tty:
pam_tty_audit: add an option to control logging of passwords
tty: add an option to control logging of passwords with pam_tty_audit
Please have a quick look and with some initial feedback I'll post them
upstream. I'd normally use git send-email and in-line it, but since
they were patches for two different entities, thought it best to do it
this way instead.
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
2:19 a.m.
On Fri, 2013-03-22 at 01:46 -0400, Richard Guy Briggs wrote:
Hi folks,
There's been a couple of requests to add a switch to pam_tty_audit to
*not* log passwords when logging user commands.
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 to do this. This feature (icanon) 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 canonical mode per task.
For the upstream inclusion of the pam_tty_audit patch you will need to
add a detection of the new member of the struct audit_tty_status in the
configure.in and #ifdef the code properly. The new option can be kept
even in the case the new member is not available, but it should log a
warning into the syslog with pam_syslog() when used. The documentation
should reflect the fact that the option might not be available on old
kernels as well.
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
12:42 p.m.
On Fri, Mar 22, 2013 at 08:19:31AM +0100, Tomas Mraz wrote:
On Fri, 2013-03-22 at 01:46 -0400, Richard Guy Briggs wrote:
> Hi folks,
>
> There's been a couple of requests to add a switch to pam_tty_audit to
> *not* log passwords when logging user commands.
>
> 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 to do this. This feature (icanon) 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 canonical mode per task.
>
For the upstream inclusion of the pam_tty_audit patch you will need to
add a detection of the new member of the struct audit_tty_status in the
configure.in and #ifdef the code properly. The new option can be kept
even in the case the new member is not available, but it should log a
warning into the syslog with pam_syslog() when used. The documentation
should reflect the fact that the option might not be available on old
kernels as well.
Tomas,
Please have a look at this patch and see if this addresses the issues
you raised:
---
configure.in | 15 +++++++++++++++
modules/pam_tty_audit/Makefile.am | 3 +++
modules/pam_tty_audit/pam_tty_audit.8.xml | 14 ++++++++++++++
modules/pam_tty_audit/pam_tty_audit.c | 23 ++++++++++++++++++++++-
4 files changed, 54 insertions(+), 1 deletions(-)
diff --git a/configure.in b/configure.in
index 515b301..c9c1c5f 100644
--- 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_icanon],
+ [
+ HAVE_AUDIT_TTY_STATUS_LOG_ICANON=yes
+ AC_DEFINE([HAVE_AUDIT_TTY_STATUS_LOG_ICANON], 1, [Define to
1 if struct audit_tty_status.log_icanon exists.])
+ ],
+ [
+ HAVE_AUDIT_TTY_STATUS_LOG_ICANON=""
+ AC_MSG_WARN([The struct audit_tty_status.log_icanon member
is needed for the log_icanon option. The log_icanon 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_ICANON],
+ [test "x$HAVE_AUDIT_TTY_STATUS_LOG_ICANON" = 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/Makefile.am b/modules/pam_tty_audit/Makefile.am
index 6378483..b67d2e5 100644
--- 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_ICANON
+ AM_CFLAGS += -DHAVE_AUDIT_TTY_STATUS_LOG_ICANON
+endif
AM_LDFLAGS = -no-undefined -avoid-version -module
if HAVE_VERSIONING
AM_LDFLAGS += -Wl,--version-script=$(srcdir)/../modules.map
diff --git a/modules/pam_tty_audit/pam_tty_audit.8.xml
b/modules/pam_tty_audit/pam_tty_audit.8.xml
index 447b845..f451f45 100644
--- a/modules/pam_tty_audit/pam_tty_audit.8.xml
+++ b/modules/pam_tty_audit/pam_tty_audit.8.xml
@@ -77,6 +77,18 @@
</para>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term>
+ <option>log_icanon</option>
+ </term>
+ <listitem>
+ <para>
+ Log keystrokes in ICANON mode. By default, keystrokes in ICANON
+ mode are not logged to avoid logging passwords. This option may not
+ be available on older kernels (3.9?).
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</refsect1>
@@ -161,6 +173,8 @@ session required pam_tty_audit.so disable=* enable=root
<para>
pam_tty_audit was written by Miloslav Trmač
&lt;mitr(a)redhat.com&gt;.
+ The log_icanon option was added by Richard Guy Briggs
+ &lt;rgb(a)redhat.com&gt;.
</para>
</refsect1>
diff --git a/modules/pam_tty_audit/pam_tty_audit.c
b/modules/pam_tty_audit/pam_tty_audit.c
index 080f495..7be914b 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_ICANON
+ int log_icanon;
+#endif
(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_ICANON
+ log_icanon = 0;
+#endif
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_icanon") == 0)
+#ifdef HAVE_AUDIT_TTY_STATUS_LOG_ICANON
+ log_icanon = 1;
+#else
+ pam_syslog (pamh, LOG_WARNING,
+ "pam_tty_audit: The log_icanon option was not available at
compile time.");
+#warning "pam_tty_audit: The log_icanon option is not available. Please upgrade
your kernel."
+#endif
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_ICANON
+ new_status.log_icanon = log_icanon;
+#endif
+ if (old_status->enabled == new_status.enabled
+#ifdef HAVE_AUDIT_TTY_STATUS_LOG_ICANON
+ && old_status->log_icanon == new_status.log_icanon
+#endif
+ )
{
open_only = 1; /* to clean up old_status */
goto ok_fd;
--
1.7.1
Tomas Mraz
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
2:14 a.m.
On Fri, 2013-04-26 at 13:42 -0400, Richard Guy Briggs wrote:
On Fri, Mar 22, 2013 at 08:19:31AM +0100, Tomas Mraz wrote:
> On Fri, 2013-03-22 at 01:46 -0400, Richard Guy Briggs wrote:
> > Hi folks,
> >
> > There's been a couple of requests to add a switch to pam_tty_audit to
> > *not* log passwords when logging user commands.
> >
> > 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 to do this. This feature (icanon) 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 canonical mode per task.
> >
>
> For the upstream inclusion of the pam_tty_audit patch you will need to
> add a detection of the new member of the struct audit_tty_status in the
> configure.in and #ifdef the code properly. The new option can be kept
> even in the case the new member is not available, but it should log a
> warning into the syslog with pam_syslog() when used. The documentation
> should reflect the fact that the option might not be available on old
> kernels as well.
Tomas,
Please have a look at this patch and see if this addresses the issues
you raised:
Yes, this is fine and can be submitted to Linux-PAM upstream for review
once the whole patch is final.
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
11:05 a.m.
----- Original Message -----
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 to do this. This feature (icanon) can be
used to avoid logging passwords by audit while still logging the rest of
the command.
There was an earlier discussion about the correctness of using ICANON for this. Is ICANON
really the right variable?
AFAICT the seeings are used like this:
(cat) and other programs that just take standard input: ICANON && ECHO
(bash), (vi) and other interactive programs: !ICANON && !ECHO
password prompts: ICANON && !ECHO
and we want to exclude only password prompts.
Mirk
3:43 p.m.
----- Original Message -----
Hi folks,
There's been a couple of requests to add a switch to pam_tty_audit to
*not* log passwords when logging user commands.
Here are two patches, the first to pam to add the switch to
the pam_tty_audit module. The second is to the kernel to add the
necessary bits in audit and tty:
Patches as attachments are little harder to comment on. So inline preferred.
From 110971ad92ce8669f6dc18db9e6369e92afdd03e Mon Sep 17 00:00:00 2001
From: Richard Guy Briggs <rgb(a)redhat.com>
Date: Thu, 21 Mar 2013 00:52:37 -0400
Subject: [PATCH] tty: add an option to control logging of passwords with pam_tty_audit
To: linux-audit(a)redhat.com
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 to do this. This feature (icanon) 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 canonical mode per task.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
drivers/tty/tty_audit.c | 8 ++++++++
include/linux/sched.h | 1 +
include/uapi/linux/audit.h | 3 ++-
kernel/audit.c | 5 ++++-
4 files changed, 15 insertions(+), 2 deletions(-)
[snip]
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -369,7 +369,8 @@ struct audit_status {
};
struct audit_tty_status {
- __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 log_icanon; /* 1 = enabled, 0 = disabled */
};
/* audit_rule_data supports filter rules with both integer and string
diff --git a/kernel/audit.c b/kernel/audit.c
index d596e53..98d43c6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -873,6 +873,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
spin_lock_irq(&tsk->sighand->siglock);
s.enabled = tsk->signal->audit_tty != 0;
+ s.log_icanon = tsk->signal->audit_tty_log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
audit_send_reply(NETLINK_CB(skb).portid, seq,
@@ -886,11 +887,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
if (nlh->nlmsg_len < sizeof(struct audit_tty_status))
return -EINVAL;
s = data;
what happens if this comes from an old pam stack that didn't set/send log_icanon?
We'd just be reading past the end of the allocation, right? Maybe something like
unsigned log_icanon
if (nlmsg_len(nlh) < sizeof(struct audit_tty_status))
log_icanon = 0;
else
log_icanon = s->log_icanon
- if (s->enabled != 0 && s->enabled != 1)
+ if (s->enabled != 0 && s->enabled != 1
+ && s->log_icanon != 0 && s->log_icanon != 1)
Shouldn't this be
if ((s->enabled != 0 && s->enabled != 1) || log_icanon != 0 &&
log_icanon != 1))
return -EINVAL;
spin_lock_irq(&tsk->sighand->siglock);
tsk->signal->audit_tty = s->enabled != 0;
+ tsk->signal->audit_tty_log_icanon = s->log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
break;
}
--
1.7.1
2:14 p.m.
On Thu, Apr 11, 2013 at 04:43:45PM -0400, Eric Paris wrote:
----- Original Message -----
> Hi folks,
>
> There's been a couple of requests to add a switch to pam_tty_audit to
> *not* log passwords when logging user commands.
> Here are two patches, the first to pam to add the switch to
> the pam_tty_audit module. The second is to the kernel to add the
> necessary bits in audit and tty:
Patches as attachments are little harder to comment on. So inline preferred.
I agree. I wasn't sure of the best way to present both userspace and
kernel patches in the same email so that any automatic patcher won't get
confused. In this case, since it is an RFC, it isn't as critical, so
convenience for commenting overrides.
(more inline below)
>From 110971ad92ce8669f6dc18db9e6369e92afdd03e Mon Sep 17 00:00:00
2001
From: Richard Guy Briggs <rgb(a)redhat.com>
Date: Thu, 21 Mar 2013 00:52:37 -0400
Subject: [PATCH] tty: add an option to control logging of passwords with pam_tty_audit
To: linux-audit(a)redhat.com
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 to do this. This feature (icanon) 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 canonical mode per task.
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
drivers/tty/tty_audit.c | 8 ++++++++
include/linux/sched.h | 1 +
include/uapi/linux/audit.h | 3 ++-
kernel/audit.c | 5 ++++-
4 files changed, 15 insertions(+), 2 deletions(-)
[snip]
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -369,7 +369,8 @@ struct audit_status {
};
struct audit_tty_status {
- __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 log_icanon; /* 1 = enabled, 0 = disabled */
};
/* audit_rule_data supports filter rules with both integer and string
diff --git a/kernel/audit.c b/kernel/audit.c
index d596e53..98d43c6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -873,6 +873,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
spin_lock_irq(&tsk->sighand->siglock);
s.enabled = tsk->signal->audit_tty != 0;
+ s.log_icanon = tsk->signal->audit_tty_log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
audit_send_reply(NETLINK_CB(skb).portid, seq,
@@ -886,11 +887,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
if (nlh->nlmsg_len < sizeof(struct audit_tty_status))
return -EINVAL;
s = data;
what happens if this comes from an old pam stack that didn't set/send log_icanon?
We'd just be reading past the end of the allocation, right? Maybe something like
unsigned log_icanon
if (nlmsg_len(nlh) < sizeof(struct audit_tty_status))
log_icanon = 0;
else
log_icanon = s->log_icanon
Good point. In fact it is more complicated than that. The previous
statement takes care of that, simply bouncing the request, so no danger
of reading past the allocation. The request could in fact be legal for
the previous version of userspace and it would not get to this point,
and would simply exit with -EINVAL, failing on what would have
previously been fine. It could try and see if it matches the length of
the previous version of the struct and behave accordingly, but that is
a messy kludge. Is it time to bump an audit API version number (I don't
find one.)?
- if (s->enabled != 0 && s->enabled != 1)
+ if (s->enabled != 0 && s->enabled != 1
+ && s->log_icanon != 0 && s->log_icanon != 1)
Shouldn't this be
if ((s->enabled != 0 && s->enabled != 1) || log_icanon != 0 &&
log_icanon != 1))
Missing a paren, but yes, you are correct. Thank you:
if ((s->enabled != 0 && s->enabled != 1) || (log_icanon != 0 &&
log_icanon != 1))
return -EINVAL;
spin_lock_irq(&tsk->sighand->siglock);
tsk->signal->audit_tty = s->enabled != 0;
+ tsk->signal->audit_tty_log_icanon = s->log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
break;
}
--
1.7.1
Here's an incremental diff, full replacement patch below that:
====8<=============
diff --git a/kernel/audit.c b/kernel/audit.c
index 98d43c6..89b9b9c 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -883,17 +883,28 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
case AUDIT_TTY_SET: {
struct audit_tty_status *s;
struct task_struct *tsk = current;
+ struct audit_tty_status_old {
+ __u32 enabled; /* 1 = enabled, 0 = disabled */
+ };
+ unsigned log_icanon;
- if (nlh->nlmsg_len < sizeof(struct audit_tty_status))
- return -EINVAL;
s = data;
- if (s->enabled != 0 && s->enabled != 1
- && s->log_icanon != 0 && s->log_icanon != 1)
+ if (nlh->nlmsg_len < sizeof(struct audit_tty_status)) {
+ if (nlh->nlmsg_len < sizeof(struct audit_tty_status_old)) {
+ return -EINVAL;
+ } else {
+ log_icanon = 0;
+ }
+ } else {
+ log_icanon = s->log_icanon
+ }
+ if ((s->enabled != 0 && s->enabled != 1)
+ || (log_icanon != 0 && log_icanon != 1))
return -EINVAL;
spin_lock_irq(&tsk->sighand->siglock);
tsk->signal->audit_tty = s->enabled != 0;
- tsk->signal->audit_tty_log_icanon = s->log_icanon != 0;
+ tsk->signal->audit_tty_log_icanon = log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
break;
}
====8<=============
Full replacement patch:
====8<=============
diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index 6953dc8..cf3f4d9 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -153,6 +153,7 @@ void tty_audit_fork(struct signal_struct *sig)
{
spin_lock_irq(¤t->sighand->siglock);
sig->audit_tty = current->signal->audit_tty;
+ sig->audit_tty_log_icanon = current->signal->audit_tty_log_icanon;
spin_unlock_irq(¤t->sighand->siglock);
}
@@ -292,10 +293,17 @@ void tty_audit_add_data(struct tty_struct *tty, unsigned char
*data,
{
struct tty_audit_buf *buf;
int major, minor;
+ int audit_log_tty_icanon;
if (unlikely(size == 0))
return;
+ spin_lock_irq(¤t->sighand->siglock);
+ audit_log_tty_icanon = current->signal->audit_tty_log_icanon;
+ spin_unlock_irq(¤t->sighand->siglock);
+ if (!audit_log_tty_icanon && icanon)
+ return;
+
if (tty->driver->type == TTY_DRIVER_TYPE_PTY
&& tty->driver->subtype == PTY_TYPE_MASTER)
return;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d35d2b6..031aa39 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -606,6 +606,7 @@ struct signal_struct {
#endif
#ifdef CONFIG_AUDIT
unsigned audit_tty;
+ unsigned audit_tty_log_icanon;
struct tty_audit_buf *tty_audit_buf;
#endif
#ifdef CONFIG_CGROUPS
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..a863669 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -369,7 +369,8 @@ struct audit_status {
};
struct audit_tty_status {
- __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 log_icanon; /* 1 = enabled, 0 = disabled */
};
/* audit_rule_data supports filter rules with both integer and string
diff --git a/kernel/audit.c b/kernel/audit.c
index d596e53..89b9b9c 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -873,6 +873,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
spin_lock_irq(&tsk->sighand->siglock);
s.enabled = tsk->signal->audit_tty != 0;
+ s.log_icanon = tsk->signal->audit_tty_log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
audit_send_reply(NETLINK_CB(skb).portid, seq,
@@ -882,15 +883,28 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
case AUDIT_TTY_SET: {
struct audit_tty_status *s;
struct task_struct *tsk = current;
+ struct audit_tty_status_old {
+ __u32 enabled; /* 1 = enabled, 0 = disabled */
+ };
+ unsigned log_icanon;
- if (nlh->nlmsg_len < sizeof(struct audit_tty_status))
- return -EINVAL;
s = data;
- if (s->enabled != 0 && s->enabled != 1)
+ if (nlh->nlmsg_len < sizeof(struct audit_tty_status)) {
+ if (nlh->nlmsg_len < sizeof(struct audit_tty_status_old)) {
+ return -EINVAL;
+ } else {
+ log_icanon = 0;
+ }
+ } else {
+ log_icanon = s->log_icanon
+ }
+ if ((s->enabled != 0 && s->enabled != 1)
+ || (log_icanon != 0 && log_icanon != 1))
return -EINVAL;
spin_lock_irq(&tsk->sighand->siglock);
tsk->signal->audit_tty = s->enabled != 0;
+ tsk->signal->audit_tty_log_icanon = log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
break;
}
====8<=============
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
2:31 p.m.
Hello,
----- Original Message -----
Full replacement patch:
I'm still convinced that icanon is not the correct condition, see
https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..a863669 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -369,7 +369,8 @@ struct audit_status {
};
struct audit_tty_status {
- __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 log_icanon; /* 1 = enabled, 0 = disabled */
};
Also, would it make sense for the user-space API to be more general about expressing the
intent ("log passwords")? I don't know, being precise about the exact
effect of the option is also beneficial.
Mirek
3:07 p.m.
On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
Hello,
----- Original Message -----
> Full replacement patch:
I'm still convinced that icanon is not the correct condition, see
https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .
That's a seperate issue. :)
I'll come back to that...
> diff --git a/include/uapi/linux/audit.h
b/include/uapi/linux/audit.h
> index 9f096f1..a863669 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -369,7 +369,8 @@ struct audit_status {
> };
>
> struct audit_tty_status {
> - __u32 enabled; /* 1 = enabled, 0 = disabled */
> + __u32 enabled; /* 1 = enabled, 0 = disabled */
> + __u32 log_icanon; /* 1 = enabled, 0 = disabled */
> };
Also, would it make sense for the user-space API to be more general
about expressing the intent ("log passwords")? I don't know, being
precise about the exact effect of the option is also beneficial.
Hmmm, I'll have to ponder that...
Mirek
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
12:16 p.m.
On Thu, Apr 18, 2013 at 04:07:08PM -0400, Richard Guy Briggs wrote:
On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
> Hello,
>
> ----- Original Message -----
> > Full replacement patch:
>
> I'm still convinced that icanon is not the correct condition, see
> https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .
That's a seperate issue. :)
I'll come back to that...
Ok, thank you for bringing me back to that. And thank you for the test
case suggestions. You are correct, !echo is needed too. I've added it
back.
> > diff --git a/include/uapi/linux/audit.h
b/include/uapi/linux/audit.h
> > index 9f096f1..a863669 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -369,7 +369,8 @@ struct audit_status {
> > };
> >
> > struct audit_tty_status {
> > - __u32 enabled; /* 1 = enabled, 0 = disabled */
> > + __u32 enabled; /* 1 = enabled, 0 = disabled */
> > + __u32 log_icanon; /* 1 = enabled, 0 = disabled */
> > };
>
> Also, would it make sense for the user-space API to be more general
> about expressing the intent ("log passwords")? I don't know, being
> precise about the exact effect of the option is also beneficial.
Hmmm, I'll have to ponder that...
I am inclined to leave it named as is for precision. The reason for the
option is covered in the manpage. Can you suggest a better wording for
the manpage if you don't think it is clear enough? A comment in the
source code wouldn't hurt though, now that you mention it.
> Mirek
- RGB
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
12:28 p.m.
----- Original Message -----
On Thu, Apr 18, 2013 at 04:07:08PM -0400, Richard Guy Briggs wrote:
> On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index 9f096f1..a863669 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -369,7 +369,8 @@ struct audit_status {
> > > };
> > >
> > > struct audit_tty_status {
> > > - __u32 enabled; /* 1 = enabled, 0 = disabled */
> > > + __u32 enabled; /* 1 = enabled, 0 = disabled */
> > > + __u32 log_icanon; /* 1 = enabled, 0 = disabled */
> > > };
> >
> > Also, would it make sense for the user-space API to be more general
> > about expressing the intent ("log passwords")? I don't know,
being
> > precise about the exact effect of the option is also beneficial.
>
> Hmmm, I'll have to ponder that...
I am inclined to leave it named as is for precision.
I suggested it might be better to change the name because with (icanon & !echo) being
the discriminator, "log_icanon" is no longer precise. I can't think of an
identifier that is both precise and understandable - neither log_icanon_noecho or
log_passwords are the obvious possibilities, neither makes me enthusiastic.
Perhaps it doesn't matter that much what the audit_tty_status member is called -
that's an implementation aspect and anybody touching this needs to understand the
precise effects regardless of the name of the member. The pam_tty_audit option name is
user-visible and should be easy to understand and use. (I'm not sure changing the
topic like this is an improvement - it seems natural to use the same name for both, and
even the users may need to understand the implications of icanon & !echo; I'm just
hoping this might lead to better suggestions.)
Mirek
1:29 p.m.
On Mon, Apr 22, 2013 at 01:28:05PM -0400, Miloslav Trmač wrote:
----- Original Message -----
> On Thu, Apr 18, 2013 at 04:07:08PM -0400, Richard Guy Briggs wrote:
> > On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
> > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > > index 9f096f1..a863669 100644
> > > > --- a/include/uapi/linux/audit.h
> > > > +++ b/include/uapi/linux/audit.h
> > > > @@ -369,7 +369,8 @@ struct audit_status {
> > > > };
> > > >
> > > > struct audit_tty_status {
> > > > - __u32 enabled; /* 1 = enabled, 0 = disabled */
> > > > + __u32 enabled; /* 1 = enabled, 0 = disabled */
> > > > + __u32 log_icanon; /* 1 = enabled, 0 = disabled */
> > > > };
> > >
> > > Also, would it make sense for the user-space API to be more general
> > > about expressing the intent ("log passwords")? I don't
know, being
> > > precise about the exact effect of the option is also beneficial.
> >
> > Hmmm, I'll have to ponder that...
>
> I am inclined to leave it named as is for precision.
I suggested it might be better to change the name because with (icanon
& !echo) being the discriminator, "log_icanon" is no longer precise.
Good point. I was one thought behind...
I can't think of an identifier that is both precise and
understandable
- neither log_icanon_noecho or log_passwords are the obvious
possibilities, neither makes me enthusiastic.
Of these, I prefer log_passwords.
Perhaps it doesn't matter that much what the audit_tty_status
member
is called - that's an implementation aspect and anybody touching this
needs to understand the precise effects regardless of the name of the
member. The pam_tty_audit option name is user-visible and should be
easy to understand and use. (I'm not sure changing the topic like
this is an improvement - it seems natural to use the same name for
both, and even the users may need to understand the implications of
icanon & !echo; I'm just hoping this might lead to better
suggestions.)
Given the original intent, and the user-visible effects, I'd lean to
log_passwords, and let the kernel implement the details necessary to
carry out that intent.
Mirek
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
12:23 p.m.
On Mon, Apr 22, 2013 at 02:29:09PM -0400, Richard Guy Briggs wrote:
On Mon, Apr 22, 2013 at 01:28:05PM -0400, Miloslav Trmač wrote:
> ----- Original Message -----
> > On Thu, Apr 18, 2013 at 04:07:08PM -0400, Richard Guy Briggs wrote:
> > > On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
> > > > > diff --git a/include/uapi/linux/audit.h
b/include/uapi/linux/audit.h
> > > > > index 9f096f1..a863669 100644
> > > > > --- a/include/uapi/linux/audit.h
> > > > > +++ b/include/uapi/linux/audit.h
> > > > > @@ -369,7 +369,8 @@ struct audit_status {
> > > > > };
> > > > >
> > > > > struct audit_tty_status {
> > > > > - __u32 enabled; /* 1 = enabled, 0 = disabled */
> > > > > + __u32 enabled; /* 1 = enabled, 0 = disabled */
> > > > > + __u32 log_icanon; /* 1 = enabled, 0 = disabled */
> > > > > };
> > > >
> > > > Also, would it make sense for the user-space API to be more general
> > > > about expressing the intent ("log passwords")? I don't
know, being
> > > > precise about the exact effect of the option is also beneficial.
> > >
> > > Hmmm, I'll have to ponder that...
> >
> > I am inclined to leave it named as is for precision.
>
> I suggested it might be better to change the name because with (icanon
> & !echo) being the discriminator, "log_icanon" is no longer precise.
Good point. I was one thought behind...
> I can't think of an identifier that is both precise and understandable
> - neither log_icanon_noecho or log_passwords are the obvious
> possibilities, neither makes me enthusiastic.
Of these, I prefer log_passwords.
> Perhaps it doesn't matter that much what the audit_tty_status member
> is called - that's an implementation aspect and anybody touching this
> needs to understand the precise effects regardless of the name of the
> member. The pam_tty_audit option name is user-visible and should be
> easy to understand and use. (I'm not sure changing the topic like
> this is an improvement - it seems natural to use the same name for
> both, and even the users may need to understand the implications of
> icanon & !echo; I'm just hoping this might lead to better
> suggestions.)
Given the original intent, and the user-visible effects, I'd lean to
log_passwords, and let the kernel implement the details necessary to
carry out that intent.
Shall I go ahead and spin a new patch that replaces most uses of
"log_icanon" with "log_pw" or "log_passwords"?
The kernel and pam_tty_audit patches are otherwise ready. I got the
autoconf bits working.
I've not heard from Eric Paris if I've addressed the userspace structure
versioning concerns adequately.
> Mirek
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
--
Linux-audit mailing list
Linux-audit(a)redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
- RGB
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
12:37 p.m.
On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
Hello,
Mirek,
----- Original Message -----
> Full replacement patch:
I'm still convinced that icanon is not the correct condition, see
https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .
As I indicated in a previous email, from my testing, you are correct.
This patch appended should address that. Comments?
Mirek
- RGB
drivers/tty/tty_audit.c | 8 ++++++++
include/linux/sched.h | 1 +
include/uapi/linux/audit.h | 3 ++-
kernel/audit.c | 5 ++++-
4 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
index 6953dc8..dc1bf78 100644
--- a/drivers/tty/tty_audit.c
+++ b/drivers/tty/tty_audit.c
@@ -153,6 +153,7 @@ void tty_audit_fork(struct signal_struct *sig)
{
spin_lock_irq(¤t->sighand->siglock);
sig->audit_tty = current->signal->audit_tty;
+ sig->audit_tty_log_icanon = current->signal->audit_tty_log_icanon;
spin_unlock_irq(¤t->sighand->siglock);
}
@@ -292,10 +293,17 @@ void tty_audit_add_data(struct tty_struct *tty, unsigned char
*data,
{
struct tty_audit_buf *buf;
int major, minor;
+ int audit_log_tty_icanon;
if (unlikely(size == 0))
return;
+ spin_lock_irq(¤t->sighand->siglock);
+ audit_log_tty_icanon = current->signal->audit_tty_log_icanon;
+ spin_unlock_irq(¤t->sighand->siglock);
+ if (!audit_log_tty_icanon && icanon && !L_ECHO(tty))
+ return;
+
if (tty->driver->type == TTY_DRIVER_TYPE_PTY
&& tty->driver->subtype == PTY_TYPE_MASTER)
return;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index d35d2b6..031aa39 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -606,6 +606,7 @@ struct signal_struct {
#endif
#ifdef CONFIG_AUDIT
unsigned audit_tty;
+ unsigned audit_tty_log_icanon;
struct tty_audit_buf *tty_audit_buf;
#endif
#ifdef CONFIG_CGROUPS
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..a863669 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -369,7 +369,8 @@ struct audit_status {
};
struct audit_tty_status {
- __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 enabled; /* 1 = enabled, 0 = disabled */
+ __u32 log_icanon; /* 1 = enabled, 0 = disabled */
};
/* audit_rule_data supports filter rules with both integer and string
diff --git a/kernel/audit.c b/kernel/audit.c
index d596e53..98d43c6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -873,6 +873,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
spin_lock_irq(&tsk->sighand->siglock);
s.enabled = tsk->signal->audit_tty != 0;
+ s.log_icanon = tsk->signal->audit_tty_log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
audit_send_reply(NETLINK_CB(skb).portid, seq,
@@ -886,11 +887,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
if (nlh->nlmsg_len < sizeof(struct audit_tty_status))
return -EINVAL;
s = data;
- if (s->enabled != 0 && s->enabled != 1)
+ if (s->enabled != 0 && s->enabled != 1
+ && s->log_icanon != 0 && s->log_icanon != 1)
return -EINVAL;
spin_lock_irq(&tsk->sighand->siglock);
tsk->signal->audit_tty = s->enabled != 0;
+ tsk->signal->audit_tty_log_icanon = s->log_icanon != 0;
spin_unlock_irq(&tsk->sighand->siglock);
break;
}
--
1.7.1
--
Richard Guy Briggs <rbriggs(a)redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635
4:02 p.m.
----- Original Message -----
On Thu, Apr 18, 2013 at 03:31:36PM -0400, Miloslav Trmač wrote:
> ----- Original Message -----
> I'm still convinced that icanon is not the correct condition, see
> https://www.redhat.com/archives/linux-audit/2013-March/msg00052.html .
As I indicated in a previous email, from my testing, you are correct.
This patch appended should address that. Comments?
Thanks, this seems fine to me (but I didn't actually test it).
Please rename the field from log_icanon, I don't think it matters whether it's to
log_pw, log_passwords or something similar.
Mirek
4254
days inactive
4292
days old
linux-audit@lists.linux-audit.osci.io
14 comments
5 participants
participants (5)
-
Eric Paris -
Miloslav Trmac -
Miloslav Trmač
-
Richard Guy Briggs -
Tomas Mraz