pam_tty_audit icanon log switch

* pam_tty_audit icanon log switch
@ 2013-03-22  5:46 Richard Guy Briggs
  2013-03-22  7:19 ` Tomas Mraz
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Richard Guy Briggs @ 2013-03-22  5:46 UTC (permalink / raw)
  To: Linux-Audit Mailing List

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

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@redhat.com>
Senior Software Engineer
AMER ENG Base Operating Systems
Remote, Canada, Ottawa
Voice: 1.647.777.2635
Internal: (81) 32635

[-- Attachment #2: 0001-pam_tty_audit-add-an-option-to-control-logging-of-pa.patch --]
[-- Type: text/plain, Size: 3410 bytes --]

>From d25d9bf48f62d35f88cee189fd3410dc1083167d Mon Sep 17 00:00:00 2001
From: Richard Guy Briggs <rgb@redhat.com>
Date: Thu, 21 Mar 2013 00:56:51 -0400
Subject: [PATCH] pam_tty_audit: add an option to control logging of passwords
To: linux-audit@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@redhat.com>
---
 modules/pam_tty_audit/pam_tty_audit.8.xml |   13 +++++++++++++
 modules/pam_tty_audit/pam_tty_audit.c     |    9 +++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/modules/pam_tty_audit/pam_tty_audit.8.xml b/modules/pam_tty_audit/pam_tty_audit.8.xml
index 447b845..deb494d 100644
--- a/modules/pam_tty_audit/pam_tty_audit.8.xml
+++ b/modules/pam_tty_audit/pam_tty_audit.8.xml
@@ -77,6 +77,17 @@
           </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.
+          </para>
+        </listitem>
+      </varlistentry>
     </variablelist>
   </refsect1>
 
@@ -161,6 +172,8 @@ session	required pam_tty_audit.so disable=* enable=root
       <para>
         pam_tty_audit was written by Miloslav Trma&ccaron;
 	&lt;mitr@redhat.com&gt;.
+        The log_icanon option was added by Richard Guy Briggs
+        &lt;rgb@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..8eb4234 100644
--- a/modules/pam_tty_audit/pam_tty_audit.c
+++ b/modules/pam_tty_audit/pam_tty_audit.c
@@ -200,7 +200,7 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char **argv)
   enum command command;
   struct audit_tty_status *old_status, new_status;
   const char *user;
-  int i, fd, open_only;
+  int i, fd, open_only, log_icanon;
 
   (void)flags;
 
@@ -212,6 +212,7 @@ pam_sm_open_session (pam_handle_t *pamh, int flags, int argc, const char **argv)
 
   command = CMD_NONE;
   open_only = 0;
+  log_icanon = 0;
   for (i = 0; i < argc; i++)
     {
       if (strncmp (argv[i], "enable=", 7) == 0
@@ -237,6 +238,8 @@ 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)
+        log_icanon = 1;
       else
 	{
 	  pam_syslog (pamh, LOG_ERR, "unknown option `%s'", argv[i]);
@@ -262,7 +265,9 @@ 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)
+  new_status.log_icanon = log_icanon;
+  if (old_status->enabled == new_status.enabled
+      && old_status->log_icanon == new_status.log_icanon)
     {
       open_only = 1; /* to clean up old_status */
       goto ok_fd;
-- 
1.7.1


[-- Attachment #3: 0001-tty-add-an-option-to-control-logging-of-passwords-wi.patch --]
[-- Type: text/plain, Size: 3734 bytes --]

>From 110971ad92ce8669f6dc18db9e6369e92afdd03e Mon Sep 17 00:00:00 2001
From: Richard Guy Briggs <rgb@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@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@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(-)

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(&current->sighand->siglock);
 	sig->audit_tty = current->signal->audit_tty;
+	sig->audit_tty_log_icanon = current->signal->audit_tty_log_icanon;
 	spin_unlock_irq(&current->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(&current->sighand->siglock);
+	audit_log_tty_icanon = current->signal->audit_tty_log_icanon;
+	spin_unlock_irq(&current->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..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


[-- Attachment #4: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply related	[flat|nested] 15+ messages in thread