logging changes in tty logging status

logging changes in tty logging status

[Richard Guy Briggs]

Hi Steve,

I'm reviewing audit_receive_msg() and noticing that the AUDIT_TTY_SET
case doesn't log a configuration change.  Should it?


- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: logging changes in tty logging status

[Steve Grubb]

On Wednesday, November 13, 2013 03:04:18 PM Richard Guy Briggs wrote:
> Hi Steve,
> 
> I'm reviewing audit_receive_msg() and noticing that the AUDIT_TTY_SET
> case doesn't log a configuration change.  Should it?

Yes, it should. Any change in config should be recorded with subject, old 
value, new value, and results. It should match other config change events.

-Steve

Re: logging changes in tty logging status

[Richard Guy Briggs]

On Wed, Nov 13, 2013 at 03:22:49PM -0500, Steve Grubb wrote:
> On Wednesday, November 13, 2013 03:04:18 PM Richard Guy Briggs wrote:
> > Hi Steve,
> > 
> > I'm reviewing audit_receive_msg() and noticing that the AUDIT_TTY_SET
> > case doesn't log a configuration change.  Should it?
> 
> Yes, it should. Any change in config should be recorded with subject, old 
> value, new value, and results. It should match other config change events.

So perhaps something like this, but should probably re-structure the
code to make it cleaner and re-factor a formatting function...

Any opinion on the labels/tags?

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..cba0109 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -829,18 +829,36 @@ 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_buffer	*ab;
 
 		memset(&s, 0, sizeof(s));
 		/* guard against past and future API changes */
 		memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
+		audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+		audit_log_format(ab, " old.audit_tty_status.enabled=%d"
+				 " old.audit_tty_status.log_passwd=%d",
+				 tsk->signal->audit_tty,
+				 tsk->signal->audit_tty_log_passwd);
+		audit_log_format(ab, " new.audit_tty_status.enabled=%d"
+				 " new.audit_tty_status.log_passwd=%d",
+				 s.enabled, s.log_passwd);
 		if ((s.enabled != 0 && s.enabled != 1) ||
 		    (s.log_passwd != 0 && s.log_passwd != 1))
-			return -EINVAL;
+{
+			audit_log_format(ab, " res=0");
+			audit_log_end(ab);
 
+			return -EINVAL;
+}
 		spin_lock(&tsk->sighand->siglock);
 		tsk->signal->audit_tty = s.enabled;
 		tsk->signal->audit_tty_log_passwd = s.log_passwd;
 		spin_unlock(&tsk->sighand->siglock);
+
+		audit_log_format(ab, " res=1");
+		audit_log_end(ab);
+
+
 		break;
 	}
 	default:

> -Steve

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: logging changes in tty logging status

[Richard Guy Briggs]

On Thu, Nov 14, 2013 at 09:16:15AM -0500, Richard Guy Briggs wrote:
> On Wed, Nov 13, 2013 at 03:22:49PM -0500, Steve Grubb wrote:
> > On Wednesday, November 13, 2013 03:04:18 PM Richard Guy Briggs wrote:
> > > Hi Steve,
> > > 
> > > I'm reviewing audit_receive_msg() and noticing that the AUDIT_TTY_SET
> > > case doesn't log a configuration change.  Should it?
> > 
> > Yes, it should. Any change in config should be recorded with subject, old 
> > value, new value, and results. It should match other config change events.
> 
> So perhaps something like this, but should probably re-structure the
> code to make it cleaner and re-factor a formatting function...
> 
> Any opinion on the labels/tags?

Here's a cleaner go at it.  I'm still not completely happy with the
redundancy and non-standard old/new labels.  This is all one command,
but changes two values.  Should they be in one line, or in two?  One
would be much simpler.

Would it be acceptable to do something like:
	" op=tty_set-enabled/log_passwd old=0/1 new=1/1 res=1"

diff --git a/kernel/audit.c b/kernel/audit.c
index 7b0e23a..1e74576 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -827,20 +827,42 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		break;
 	}
 	case AUDIT_TTY_SET: {
-		struct audit_tty_status s;
+		struct audit_tty_status s, old;
 		struct task_struct *tsk = current;
+		struct audit_buffer	*ab;
+		int res = 0;
+
+		spin_lock(&tsk->sighand->siglock);
+		old.enabled = tsk->signal->audit_tty;
+		old.log_passwd = tsk->signal->audit_tty_log_passwd;
+		spin_unlock(&tsk->sighand->siglock);
 
 		memset(&s, 0, sizeof(s));
 		/* guard against past and future API changes */
 		memcpy(&s, data, min(sizeof(s), (size_t)nlh->nlmsg_len));
-		if ((s.enabled != 0 && s.enabled != 1) ||
-		    (s.log_passwd != 0 && s.log_passwd != 1))
+		if ((s.enabled == 0 || s.enabled == 1) &&
+		    (s.log_passwd == 0 || s.log_passwd == 1))
+			res = 1;
+		audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+		audit_log_format(ab, " op=tty_set"
+				 " old.enabled=%d"
+				 " new.enabled=%d"
+				 " old.log_passwd=%d"
+				 " new.log_passwd=%d"
+				 " res=%d",
+				 old.enabled,
+				 s.enabled,
+				 old.log_passwd,
+				 s.log_passwd,
+				 res);
+		audit_log_end(ab);
+		if (res) {
+			spin_lock(&tsk->sighand->siglock);
+			tsk->signal->audit_tty = s.enabled;
+			tsk->signal->audit_tty_log_passwd = s.log_passwd;
+			spin_unlock(&tsk->sighand->siglock);
+		} else
 			return -EINVAL;
-
-		spin_lock(&tsk->sighand->siglock);
-		tsk->signal->audit_tty = s.enabled;
-		tsk->signal->audit_tty_log_passwd = s.log_passwd;
-		spin_unlock(&tsk->sighand->siglock);
 		break;
 	}
 	default:

> > -Steve
> 
> - RGB
> 
> --
> Richard Guy Briggs <rbriggs@redhat.com>
> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545