Re: pam_tty_audit icanon log switch

----- Original Message ----- Patches as attachments are little harder to comment on. So inline preferred. From: Richard Guy Briggs <rgb(a)redhat.com&gt; 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&gt; --- 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