Re: pam_tty_audit icanon log switch

[Richard Guy Briggs <rgb@redhat.com>]

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