Audit: Add TTY input auditing

PATCH Superseded Miloslav Trmac 7 participants 19 messages Jun 6, 2007 – Jun 8, 2007

Subsystems (9)

Networking MM core Scheduler TTY layer and serial drivers +5 more subsystems

lore

Share & tools

lore

Miloslav Trmac

author

2007-06-06 09:51 UTC #0 lore raw

PATCH 1/1 Audit: Add TTY input auditing

From: Miloslav Trmac <[email protected]>

Add TTY input auditing, used to audit system administrator's actions. TTY input auditing works on a higher level than auditing all system calls within the session, which would produce an overwhelming amount of mostly useless audit events.

Add an "audit_tty" attribute, inherited across fork (). Data read from TTYs by process with the attribute is sent to the audit subsystem by the kernel. The audit netlink interface is extended to allow modifying the audit_tty attribute, and to allow sending explanatory audit events from user-space (for example, a shell might send an event containing the final command, after the interactive command-line editing and history expansion is performed, which might be difficult to decipher from the TTY input alone).

Because the "audit_tty" attribute is inherited across fork (), it would be set e.g. for sshd restarted within an audited session. To prevent this, the audit_tty attribute is cleared when a process with no open TTY file descriptors (e.g. after daemon startup) opens a TTY.

See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.

Miloslav Trmac

author

2007-06-06 10:12 UTC #1 lore raw

I'm sorry, I forgot the patch...

From: Miloslav Trmac <[email protected]>

See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.

Signed-off-by Miloslav Trmac <[email protected]>

---
 drivers/char/n_tty.c        |  359 +++++++++++++++++++++++++++++++++-
 drivers/char/tty_io.c       |   11 -
 include/linux/audit.h       |   11 +
 include/linux/sched.h       |    2 
 include/linux/tty.h         |   15 +
 kernel/audit.c              |   97 ++++++++-
 kernel/audit.h              |    1 
 kernel/auditsc.c            |    3 
 kernel/exit.c               |    2 
 kernel/fork.c               |    5 
 security/selinux/nlmsgtab.c |    2 
 11 files changed, 488 insertions(+), 20 deletions(-)

drivers/char/n_tty.c +355 -4

index 154f422..be92b2d 100644
@@ -45,6 +45,8 @@
 #include <linux/slab.h>
 #include <linux/poll.h>
 #include <linux/bitops.h>
+#include <linux/audit.h>
+#include <linux/file.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>

@@ -78,6 +80,352 @@ static inline void free_buf(unsigned char *buf)
 		free_page((unsigned long) buf);
 }
 
+#ifdef CONFIG_AUDIT
+struct tty_audit_buf {
+	atomic_t count;
+	struct mutex mutex;	/* Protects all data below */
+	int major, minor;	/* The TTY which the data is from */
+	unsigned icanon:1;
+	size_t valid;
+	unsigned char *data;	/* Allocated size N_TTY_BUF_SIZE */
+};
+
+static struct tty_audit_buf *
+tty_audit_buf_alloc(int major, int minor, int icanon)
+{
+	struct tty_audit_buf *buf;
+
+	buf = kmalloc(sizeof (*buf), GFP_KERNEL);
+	if (!buf)
+		goto err;
+	buf->data = alloc_buf();
+	if (!buf->data)
+		goto err_buf;
+	atomic_set(&buf->count, 1);
+	mutex_init(&buf->mutex);
+	buf->major = major;
+	buf->minor = minor;
+	buf->icanon = icanon;
+	buf->valid = 0;
+	return buf;
+
+err_buf:
+	kfree(buf);
+err:
+	return NULL;
+}
+
+static void
+tty_audit_buf_free(struct tty_audit_buf *buf)
+{
+	WARN_ON(buf->valid != 0);
+	free_buf(buf->data);
+	kfree(buf);
+}
+
+static void
+tty_audit_buf_put(struct tty_audit_buf *buf)
+{
+	if (atomic_dec_and_test(&buf->count))
+		tty_audit_buf_free(buf);
+}
+
+/**
+ *	tty_audit_buf_push	-	Push buffered data out
+ *
+ *	Generate an audit message from the contents of @buf, which is owned by
+ *	@tsk with @loginuid.  @buf->mutex must be locked.
+ */
+static void
+tty_audit_buf_push(struct task_struct *tsk, uid_t loginuid,
+		   struct tty_audit_buf *buf)
+{
+	struct audit_buffer *ab;
+
+	if (buf->valid == 0)
+		return;
+	if (audit_enabled == 0)
+		return;
+	ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
+	if (ab) {
+		char name[sizeof(tsk->comm)];
+
+		audit_log_format(ab, "tty pid=%u uid=%u auid=%u major=%d "
+				 "minor=%d comm=", tsk->pid, tsk->uid,
+				 loginuid, buf->major, buf->minor);
+		get_task_comm(name, tsk);
+		audit_log_untrustedstring(ab, name);
+		audit_log_format(ab, " data=");
+		audit_log_n_untrustedstring(ab, buf->valid, buf->data);
+		audit_log_end(ab);
+	}
+	buf->valid = 0;
+}
+
+/**
+ *	tty_audit_buf_push_current	-	Push buffered data out
+ *
+ *	Generate an audit message from the contents of @buf, which is owned by
+ *	the current task.  @buf->mutex must be locked.
+ */
+static void
+tty_audit_buf_push_current(struct tty_audit_buf *buf)
+{
+	tty_audit_buf_push(current, audit_get_loginuid(current->audit_context),
+			   buf);
+}
+
+/**
+ *	tty_audit_exit	-	Handle a task exit
+ *
+ *	Make sure all buffered data is written out and deallocate the buffer.
+ *	Only needs to be called if current->signal->tty_audit_buf != %NULL.
+ */
+void
+tty_audit_exit(void)
+{
+	struct tty_audit_buf *buf;
+
+	spin_lock(&current->sighand->siglock);
+	buf = current->signal->tty_audit_buf;
+	current->signal->tty_audit_buf = NULL;
+	spin_unlock(&current->sighand->siglock);
+	if (!buf)
+		return;
+
+	mutex_lock(&buf->mutex);
+	tty_audit_buf_push_current(buf);
+	mutex_unlock(&buf->mutex);
+
+	tty_audit_buf_put(buf);
+}
+
+/**
+ *	tty_audit_push_task	-	Flush task's pending audit data
+ */
+void
+tty_audit_push_task(struct task_struct *tsk, uid_t loginuid)
+{
+	struct tty_audit_buf *buf;
+
+	spin_lock(&tsk->sighand->siglock);
+	buf = tsk->signal->tty_audit_buf;
+	if (buf)
+		atomic_inc(&buf->count);
+	spin_unlock(&tsk->sighand->siglock);
+	if (!buf)
+		return;
+
+	mutex_lock(&buf->mutex);
+	tty_audit_buf_push(tsk, loginuid, buf);
+	mutex_unlock(&buf->mutex);
+
+	tty_audit_buf_put(buf);
+}
+
+/**
+ *	tty_audit_buf_get	-	Get an audit buffer.
+ *
+ *	Get an audit buffer for @tty, allocate it if necessary.  Return %NULL
+ *	if TTY auditing is disabled or out of memory.  Otherwise, return a new
+ *	reference to the buffer.
+ */
+static struct tty_audit_buf *
+tty_audit_buf_get(struct tty_struct *tty)
+{
+	struct tty_audit_buf *buf, *buf2;
+
+	buf = NULL;
+	buf2 = NULL;
+	spin_lock(&current->sighand->siglock);
+	if (likely(!current->signal->audit_tty))
+		goto out;
+	buf = current->signal->tty_audit_buf;
+	if (buf) {
+		atomic_inc(&buf->count);
+		goto out;
+	}
+	spin_unlock(&current->sighand->siglock);
+
+	buf2 = tty_audit_buf_alloc(tty->driver->major,
+				   tty->driver->minor_start + tty->index,
+				   tty->icanon);
+	if (buf2 == NULL) {
+		audit_log_lost("out of memory in TTY auditing");
+		return NULL;
+	}
+
+	spin_lock(&current->sighand->siglock);
+	if (!current->signal->audit_tty)
+		goto out;
+	buf = current->signal->tty_audit_buf;
+	if (!buf) {
+		current->signal->tty_audit_buf = buf2;
+		buf = buf2;
+		buf2 = NULL;
+	}
+	atomic_inc(&buf->count);
+	/* Fall through */
+ out:
+	spin_unlock(&current->sighand->siglock);
+	if (buf2)
+		tty_audit_buf_free(buf2);
+	return buf;
+}
+
+/**
+ *	tty_audit_add_data	-	Add data for TTY auditing.
+ *
+ *	Audit @data of @size from @tty, if necessary.
+ */
+static void
+tty_audit_add_data(struct tty_struct *tty, unsigned char *data, size_t size)
+{
+	struct tty_audit_buf *buf;
+	int major, minor;
+
+	if (unlikely(size == 0))
+		return;
+
+	buf = tty_audit_buf_get(tty);
+	if (!buf)
+		return;
+
+	mutex_lock(&buf->mutex);
+	major = tty->driver->major;
+	minor = tty->driver->minor_start + tty->index;
+	if (buf->major != major || buf->minor != minor
+	    || buf->icanon != tty->icanon) {
+		tty_audit_buf_push_current(buf);
+		buf->major = major;
+		buf->minor = minor;
+		buf->icanon = tty->icanon;
+	}
+	do {
+	  size_t run;
+
+	  run = N_TTY_BUF_SIZE - buf->valid;
+	  if (run > size)
+	    run = size;
+	  memcpy(buf->data + buf->valid, data, run);
+	  buf->valid += run;
+	  data += run;
+	  size -= run;
+	  if (buf->valid == N_TTY_BUF_SIZE)
+		  tty_audit_buf_push_current(buf);
+	} while (size != 0);
+	mutex_unlock(&buf->mutex);
+	tty_audit_buf_put(buf);
+}
+
+/**
+ *	tty_audit_push	-	Push buffered data out
+ *
+ *	Make sure no audit data is pending for @tty on the current process.
+ */
+static void
+tty_audit_push(struct tty_struct *tty)
+{
+	struct tty_audit_buf *buf;
+
+	spin_lock(&current->sighand->siglock);
+	if (likely(!current->signal->audit_tty)) {
+		spin_unlock(&current->sighand->siglock);
+		return;
+	}
+	buf = current->signal->tty_audit_buf;
+	if (buf)
+		atomic_inc(&buf->count);
+	spin_unlock(&current->sighand->siglock);
+
+	if (buf) {
+		int major, minor;
+
+		major = tty->driver->major;
+		minor = tty->driver->minor_start + tty->index;
+		mutex_lock(&buf->mutex);
+		if (buf->major == major && buf->minor == minor)
+			tty_audit_buf_push_current(buf);
+		mutex_unlock(&buf->mutex);
+		tty_audit_buf_put(buf);
+	}
+}
+
+/* For checking whether a file is a TTY */
+extern ssize_t tty_read(struct file * file, char __user * buf, size_t count,
+			loff_t *ppos);
+
+/**
+ *	tty_audit_opening	-	A TTY is being opened.
+ *
+ *	As a special hack, tasks that close all their TTYs and open new ones
+ *	are assumed to be system daemons (e.g. getty) and auditing is
+ *	automatically disabled for them.
+ */
+void
+tty_audit_opening(void)
+{
+	int disable;
+
+	disable = 1;
+	spin_lock(&current->sighand->siglock);
+	if (current->signal->audit_tty == 0)
+		disable = 0;
+	spin_unlock(&current->sighand->siglock);
+	if (!disable)
+		return;
+
+	task_lock(current);
+	if (current->files) {
+		struct fdtable *fdt;
+		unsigned i;
+
+		/*
+		 * We don't take a ref to the file, so we must hold ->file_lock
+		 * instead.
+		 */
+		spin_lock(&current->files->file_lock);
+		fdt = files_fdtable(current->files);
+		for (i = 0; i < fdt->max_fds; i++) {
+			struct file *filp;
+
+			filp = fcheck_files(current->files, i);
+			if (!filp)
+				continue;
+			if (filp->f_op->read == tty_read) {
+				disable = 0;
+				break;
+			}
+		}
+		spin_unlock(&current->files->file_lock);
+	}
+	task_unlock(current);
+	if (!disable)
+		return;
+
+	spin_lock(&current->sighand->siglock);
+	current->signal->audit_tty = 0;
+	spin_unlock(&current->sighand->siglock);
+}
+#else
+inline static void
+tty_audit_add_data(struct tty_struct *tty, unsigned char *data, size_t size)
+{
+}
+
+static void
+tty_audit_push(struct tty_struct *tty)
+{
+}
+#endif
+
+static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
+			       unsigned char __user *ptr)
+{
+	tty_audit_add_data(tty, &x, 1);
+	return put_user(x, ptr);
+}
+
 /**
  *	n_tty_set__room	-	receive space
  *	@tty: terminal

@@ -1153,6 +1501,7 @@ static int copy_from_read_buf(struct tty_struct *tty,
 	if (n) {
 		retval = copy_to_user(*b, &tty->read_buf[tty->read_tail], n);
 		n -= retval;
+		tty_audit_add_data(tty, &tty->read_buf[tty->read_tail], n);
 		spin_lock_irqsave(&tty->read_lock, flags);
 		tty->read_tail = (tty->read_tail + n) & (N_TTY_BUF_SIZE-1);
 		tty->read_cnt -= n;

@@ -1279,7 +1628,7 @@ do_it_again:
 				break;
 			cs = tty->link->ctrl_status;
 			tty->link->ctrl_status = 0;
-			if (put_user(cs, b++)) {
+			if (tty_put_user(tty, cs, b++)) {
 				retval = -EFAULT;
 				b--;
 				break;

@@ -1321,7 +1670,7 @@ do_it_again:
 
 		/* Deal with packet mode. */
 		if (tty->packet && b == buf) {
-			if (put_user(TIOCPKT_DATA, b++)) {
+			if (tty_put_user(tty, TIOCPKT_DATA, b++)) {
 				retval = -EFAULT;
 				b--;
 				break;

@@ -1352,15 +1701,17 @@ do_it_again:
 				spin_unlock_irqrestore(&tty->read_lock, flags);
 
 				if (!eol || (c != __DISABLED_CHAR)) {
-					if (put_user(c, b++)) {
+					if (tty_put_user(tty, c, b++)) {
 						retval = -EFAULT;
 						b--;
 						break;
 					}
 					nr--;
 				}
-				if (eol)
+				if (eol) {
+					tty_audit_push(tty);
 					break;
+				}
 			}
 			if (retval)
 				break;

drivers/char/tty_io.c +7 -4

index 3752edc..9ed2f23 100644
@@ -143,7 +143,7 @@ static int ptmx_open(struct inode *, struct file *);
 
 static void initialize_tty_struct(struct tty_struct *tty);
 
-static ssize_t tty_read(struct file *, char __user *, size_t, loff_t *);
+ssize_t tty_read(struct file *, char __user *, size_t, loff_t *);
 static ssize_t tty_write(struct file *, const char __user *, size_t, loff_t *);
 ssize_t redirected_tty_write(struct file *, const char __user *, size_t, loff_t *);
 static unsigned int tty_poll(struct file *, poll_table *);

@@ -1690,8 +1690,8 @@ EXPORT_SYMBOL(start_tty);
  *	in new code. Multiple read calls may be outstanding in parallel.
  */
 
-static ssize_t tty_read(struct file * file, char __user * buf, size_t count, 
-			loff_t *ppos)
+ssize_t tty_read(struct file * file, char __user * buf, size_t count,
+		 loff_t *ppos)
 {
 	int i;
 	struct tty_struct * tty;

@@ -2654,6 +2654,7 @@ got_driver:
 		__proc_set_tty(current, tty);
 	spin_unlock_irq(&current->sighand->siglock);
 	mutex_unlock(&tty_mutex);
+	tty_audit_opening();
 	return 0;
 }

@@ -2716,8 +2717,10 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 
 	check_tty_count(tty, "tty_open");
 	retval = ptm_driver->open(tty, filp);
-	if (!retval)
+	if (!retval) {
+		tty_audit_opening();
 		return 0;
+	}
 out1:
 	release_dev(filp);
 	return retval;

include/linux/audit.h +11 -0

index fccc6e5..dbf2125 100644
@@ -63,9 +63,12 @@
 #define AUDIT_ADD_RULE		1011	/* Add syscall filtering rule */
 #define AUDIT_DEL_RULE		1012	/* Delete syscall filtering rule */
 #define AUDIT_LIST_RULES	1013	/* List syscall filtering rules */
+#define AUDIT_TTY_GET		1014	/* Get TTY auditing status */
+#define AUDIT_TTY_SET		1015	/* Set TTY auditing status */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */
+#define AUDIT_USER_TTY		1124	/* Non-ICANON TTY input meaning */
 #define AUDIT_LAST_USER_MSG	1199
 #define AUDIT_FIRST_USER_MSG2	2100	/* More user space messages */
 #define AUDIT_LAST_USER_MSG2	2999

@@ -92,6 +95,7 @@
 #define AUDIT_KERNEL_OTHER	1316	/* For use by 3rd party modules */
 #define AUDIT_FD_PAIR		1317    /* audit record for pipe/socketpair */
 #define AUDIT_OBJ_PID		1318	/* ptrace target */
+#define AUDIT_TTY		1319	/* Input on an administrative TTY */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */

@@ -289,6 +293,10 @@ struct audit_status {
 	__u32		backlog;	/* messages waiting in queue */
 };
 
+struct audit_tty_status {
+	__u32		enabled; /* 1 = enabled, 0 = disabled */
+};
+
 /* audit_rule_data supports filter rules with both integer and string
  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
  * AUDIT_LIST_RULES requests.

@@ -455,6 +463,7 @@ static inline int audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
 		return __audit_mq_getsetattr(mqdes, mqstat);
 	return 0;
 }
+extern int audit_enabled;
 extern int audit_n_rules;
 extern int audit_signals;
 #else

@@ -487,6 +496,7 @@ extern int audit_signals;
 #define audit_mq_notify(d,n) ({ 0; })
 #define audit_mq_getsetattr(d,s) ({ 0; })
 #define audit_ptrace(t) ((void)0)
+#define audit_enabled 0
 #define audit_n_rules 0
 #define audit_signals 0
 #endif

@@ -515,6 +525,7 @@ extern void		    audit_log_d_path(struct audit_buffer *ab,
 					     const char *prefix,
 					     struct dentry *dentry,
 					     struct vfsmount *vfsmnt);
+extern void		    audit_log_lost(const char *message);
 				/* Private API (for audit.c only) */
 extern int audit_filter_user(struct netlink_skb_parms *cb, int type);
 extern int audit_filter_type(int type);

include/linux/sched.h +2 -0

index d58e74b..3ae4904 100644
@@ -506,6 +506,8 @@ struct signal_struct {
 #ifdef CONFIG_TASKSTATS
 	struct taskstats *stats;
 #endif
+	unsigned audit_tty:1;
+	struct tty_audit_buf *tty_audit_buf;
 };
 
 /* Context switch must be unlocked if interrupts are to be enabled */

include/linux/tty.h +15 -0

index bb45760..d33f8dd 100644
@@ -340,6 +340,21 @@ extern struct mutex tty_mutex;
 
 /* n_tty.c */
 extern struct tty_ldisc tty_ldisc_N_TTY;
+#ifdef CONFIG_AUDIT
+extern void tty_audit_exit(void);
+extern void tty_audit_push_task(struct task_struct *tsk, uid_t loginuid);
+extern void tty_audit_opening(void);
+#else
+static inline void tty_audit_exit(void)
+{
+}
+static inline void tty_audit_push_task(struct task_struct *tsk, uid_t loginuid)
+{
+}
+static inline void tty_audit_opening(void)
+{
+}
+#endif
 
 /* tty_ioctl.c */
 extern int n_tty_ioctl(struct tty_struct * tty, struct file * file,

kernel/audit.c +89 -8

159 lines not on this page · raw message

kernel/audit.h +0 -1

index 815d6f5..9587743 100644
@@ -115,7 +115,6 @@ extern struct sk_buff *	    audit_make_reply(int pid, int seq, int type,
 extern void		    audit_send_reply(int pid, int seq, int type,
 					     int done, int multi,
 					     void *payload, int size);
-extern void		    audit_log_lost(const char *message);
 extern void		    audit_panic(const char *message);
 
 struct audit_netlink_list {

kernel/auditsc.c +0 -3

index e36481e..7ccc3da 100644
@@ -71,9 +71,6 @@
 
 extern struct list_head audit_filter_list[];
 
-/* No syscall auditing will take place unless audit_enabled != 0. */
-extern int audit_enabled;
-
 /* AUDIT_NAMES is the number of slots we reserve in the audit_context
  * for saving names from getname(). */
 #define AUDIT_NAMES    20

kernel/exit.c +2 -0

index 5b888c2..413f6df 100644
@@ -922,6 +922,8 @@ fastcall NORET_TYPE void do_exit(long code)
 	if (unlikely(tsk->compat_robust_list))
 		compat_exit_robust_list(tsk);
 #endif
+	if (group_dead && unlikely(tsk->signal->tty_audit_buf))
+		tty_audit_exit();
 	if (unlikely(tsk->audit_context))
 		audit_free(tsk);

kernel/fork.c +5 -0

index 73ad5cd..52c449f 100644
@@ -897,6 +897,11 @@ static inline int copy_signal(unsigned long clone_flags, struct task_struct * ts
 	}
 	acct_init_pacct(&sig->pacct);
 
+	spin_lock(&current->sighand->siglock);
+	sig->audit_tty = current->signal->audit_tty;
+	spin_unlock(&current->sighand->siglock);
+	sig->tty_audit_buf = NULL;
+
 	return 0;
 }

security/selinux/nlmsgtab.c +2 -0

index ccfe875..eddc7b4 100644
@@ -110,6 +110,8 @@ static struct nlmsg_perm nlmsg_audit_perms[] =
 	{ AUDIT_DEL_RULE,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 	{ AUDIT_USER,		NETLINK_AUDIT_SOCKET__NLMSG_RELAY    },
 	{ AUDIT_SIGNAL_INFO,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
+	{ AUDIT_TTY_GET,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
+	{ AUDIT_TTY_SET,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 };

Andrew Morton

maintainer

2007-06-07 00:41 UTC #2 lore raw

Quoted 26 lines · On Wed, 06 Jun 2007 12:10:28 +0200 Miloslav Trmac <[email protected]> wrote:

From: Miloslav Trmac <[email protected]>
Add TTY input auditing, used to audit system administrator's actions. TTY input auditing works on a higher level than auditing all system calls within the session, which would produce an overwhelming amount of mostly useless audit events.
Add an "audit_tty" attribute, inherited across fork (). Data read from TTYs by process with the attribute is sent to the audit subsystem by the kernel. The audit netlink interface is extended to allow modifying the audit_tty attribute, and to allow sending explanatory audit events from user-space (for example, a shell might send an event containing the final command, after the interactive command-line editing and history expansion is performed, which might be difficult to decipher from the TTY input alone).
Because the "audit_tty" attribute is inherited across fork (), it would be set e.g. for sshd restarted within an audited session. To prevent this, the audit_tty attribute is cleared when a process with no open TTY file descriptors (e.g. after daemon startup) opens a TTY.
See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.
...

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 120

Quoted 3 lines

drivers/char/n_tty.c +3 -0

+static void
+tty_audit_buf_free(struct tty_audit_buf *buf)
+{

The usual kernel style is

static void tty_audit_buf_free(struct tty_audit_buf *buf) {

and the style which you've used here is usually only employed if its use prevents an 80-column overflow.

There are plenty of exceptions to this, and I understand (and actually agree with) the reason for the style which you've chosen, but standardisation wins out.

The patch adds a lot of new code to n_tty.c, I suspect it would be neater to put it all into a new file if possible?

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 189

Quoted 12 lines

drivers/char/n_tty.c +12 -0

+/**
+ *	tty_audit_exit	-	Handle a task exit
+ *
+ *	Make sure all buffered data is written out and deallocate the buffer.
+ *	Only needs to be called if current->signal->tty_audit_buf != %NULL.
+ */
+void
+tty_audit_exit(void)
+{
+	struct tty_audit_buf *buf;
+
+	spin_lock(&current->sighand->siglock);

I think you have a bug here. ->siglock is taken elsewhere in an irq-safe fashion (multiple instances)

Quoted 31 lines

drivers/char/n_tty.c +31 -0

+/**
+ *	tty_audit_add_data	-	Add data for TTY auditing.
+ *
+ *	Audit @data of @size from @tty, if necessary.
+ */
+static void
+tty_audit_add_data(struct tty_struct *tty, unsigned char *data, size_t size)
+{
+	struct tty_audit_buf *buf;
+	int major, minor;
+
+	if (unlikely(size == 0))
+		return;
+
+	buf = tty_audit_buf_get(tty);
+	if (!buf)
+		return;
+
+	mutex_lock(&buf->mutex);
+	major = tty->driver->major;
+	minor = tty->driver->minor_start + tty->index;
+	if (buf->major != major || buf->minor != minor
+	    || buf->icanon != tty->icanon) {
+		tty_audit_buf_push_current(buf);
+		buf->major = major;
+		buf->minor = minor;
+		buf->icanon = tty->icanon;
+	}
+	do {
+	  size_t run;
+

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 316

Quoted 10 lines

drivers/char/n_tty.c +10 -0

+	  run = N_TTY_BUF_SIZE - buf->valid;
+	  if (run > size)
+	    run = size;
+	  memcpy(buf->data + buf->valid, data, run);
+	  buf->valid += run;
+	  data += run;
+	  size -= run;
+	  if (buf->valid == N_TTY_BUF_SIZE)
+		  tty_audit_buf_push_current(buf);
+	} while (size != 0);

the whitespace went bad here.

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 356

Quoted 11 lines

drivers/char/n_tty.c +8 -0

+	mutex_unlock(&buf->mutex);
+	tty_audit_buf_put(buf);
+}
+
 
...
 
+
+/* For checking whether a file is a TTY */
+extern ssize_t tty_read(struct file * file, char __user * buf, size_t count,
+			loff_t *ppos);

Nope, please don't add extern declarations to C files. Do it via header files.

Quoted 20 lines

drivers/char/n_tty.c +20 -0

+/**
+ *	tty_audit_opening	-	A TTY is being opened.
+ *
+ *	As a special hack, tasks that close all their TTYs and open new ones
+ *	are assumed to be system daemons (e.g. getty) and auditing is
+ *	automatically disabled for them.
+ */
+void
+tty_audit_opening(void)
+{
+	int disable;
+
+	disable = 1;
+	spin_lock(&current->sighand->siglock);
+	if (current->signal->audit_tty == 0)
+		disable = 0;
+	spin_unlock(&current->sighand->siglock);
+	if (!disable)
+		return;
+

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 387

Quoted 10 lines

drivers/char/n_tty.c +10 -0

+	task_lock(current);
+	if (current->files) {
+		struct fdtable *fdt;
+		unsigned i;
+
+		/*
+		 * We don't take a ref to the file, so we must hold ->file_lock
+		 * instead.
+		 */
+		spin_lock(&current->files->file_lock);

So we make file_lock nest inside task_lock(). Was that lock ranking already being used elsewhere in the kernel, or is it a new association?

Has this code had full coverage testing with all lockdep features enabled?

(I suspect not - lockdep should have gone wild over the siglock thing)

Quoted 17 lines

drivers/char/n_tty.c +17 -0

+		fdt = files_fdtable(current->files);
+		for (i = 0; i < fdt->max_fds; i++) {
+			struct file *filp;
+
+			filp = fcheck_files(current->files, i);
+			if (!filp)
+				continue;
+			if (filp->f_op->read == tty_read) {
+				disable = 0;
+				break;
+			}
+		}
+		spin_unlock(&current->files->file_lock);
+	}
+	task_unlock(current);
+	if (!disable)
+		return;

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 414

Quoted 10 lines

drivers/char/n_tty.c +10 -0

+
+	spin_lock(&current->sighand->siglock);
+	current->signal->audit_tty = 0;
+	spin_unlock(&current->sighand->siglock);
+}
+#else
+inline static void
+tty_audit_add_data(struct tty_struct *tty, unsigned char *data, size_t size)
+{
+}

Please use static inline' rather that inline static'. Just a consistency thing.

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 335

Quoted 4 lines

drivers/char/n_tty.c +4 -0

+static void
+tty_audit_push(struct tty_struct *tty)
+{
+}

Probably you meant `static inline' here too.

Quoted 33 lines

drivers/char/n_tty.c +11 -0

+#endif
+
+static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
+			       unsigned char __user *ptr)
+{
+	tty_audit_add_data(tty, &x, 1);
+	return put_user(x, ptr);
+}
+
 
...

@@ -487,6 +496,7 @@ extern int audit_signals;
 #define audit_mq_notify(d,n) ({ 0; })
 #define audit_mq_getsetattr(d,s) ({ 0; })
 #define audit_ptrace(t) ((void)0)
+#define audit_enabled 0
 #define audit_n_rules 0
 #define audit_signals 0
 #endif

@@ -515,6 +525,7 @@ extern void		    audit_log_d_path(struct audit_buffer *ab,
 					     const char *prefix,
 					     struct dentry *dentry,
 					     struct vfsmount *vfsmnt);
+extern void		    audit_log_lost(const char *message);
 				/* Private API (for audit.c only) */
 extern int audit_filter_user(struct netlink_skb_parms *cb, int type);
 extern int audit_filter_type(int type);

include/linux/sched.h +0 -0

index d58e74b..3ae4904 100644

include/linux/sched.h struct signal_struct { · line 511

Quoted 6 lines

include/linux/sched.h +2 -0

@@ -506,6 +506,8 @@ struct signal_struct {
 #ifdef CONFIG_TASKSTATS
 	struct taskstats *stats;
 #endif
+	unsigned audit_tty:1;
+	struct tty_audit_buf *tty_audit_buf;
 };

hm, bitfields are risky. If someone adds another one, it will land in the same word and external locking will be needed. You do seem to be using ->siglock to cover this - was that to address the bitfield non-atomicity problem?

A suitable (but somewhat less pretty) way to resolve all this is to not use bitfields at all: add `unsigned long flags' and use set_bit/clear_bit/etc.

Quoted 24 lines

include/linux/sched.h +0 -0

 extern int n_tty_ioctl(struct tty_struct * tty, struct file * file,

kernel/audit.c +8 -0

index d13276d..a071a96 100644
@@ -58,6 +58,7 @@
 #include <linux/selinux.h>
 #include <linux/inotify.h>
 #include <linux/freezer.h>
+#include <linux/tty.h>
 
 #include "audit.h"

@@ -423,6 +424,32 @@ static int kauditd_thread(void *dummy)
 	return 0;
 }
 
+static int
+audit_prepare_user_tty(pid_t pid, uid_t loginuid)
+{
+	struct task_struct *tsk;
+	int err;
+
+	read_lock(&tasklist_lock);

kernel/audit.c static int kauditd_thread(void *dummy) · line 443

Quoted 10 lines

kernel/audit.c +10 -0

+	tsk = find_task_by_pid(pid);
+	err = -ESRCH;
+	if (!tsk)
+		goto out;
+	err = 0;
+
+	spin_lock(&tsk->sighand->siglock);
+	if (!tsk->signal->audit_tty)
+		err = -EPERM;
+	spin_unlock(&tsk->sighand->siglock);

So siglock nests inside tasklist_lock? Sounds reasonable. Is this a preexisting association, or did this patch just create it?

Quoted 15 lines

kernel/audit.c +11 -0

+	if (err)
+		goto out;
+
+	tty_audit_push_task(tsk, loginuid);
+out:
+	read_unlock(&tasklist_lock);
+	return err;
+}
+
 
...
 
 		break;
+	case AUDIT_TTY_GET: {
+		struct audit_tty_status s;

kernel/audit.c static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) · line 787

Quoted 10 lines

kernel/audit.c +10 -0

+		struct task_struct *tsk;
+
+		read_lock(&tasklist_lock);
+		tsk = find_task_by_pid(pid);
+		if (!tsk)
+			err = -ESRCH;
+		else {
+			spin_lock(&tsk->sighand->siglock);
+			s.enabled = tsk->signal->audit_tty != 0;
+			spin_unlock(&tsk->sighand->siglock);

The locking here looks dubious. tsk->signal->audit_tty can change state the instant ->siglock gets unlocked, in which case s.enabled is now wrong.

If that is acceptable then we didn't need that locking at all.

Alan Cox

2007-06-07 10:05 UTC #3 lore raw

Quoted 3 lines

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 397
Quoted 3 lines
drivers/char/n_tty.c +3 -0
+			if (filp->f_op->read == tty_read) {
+				disable = 0;
+				break;

Why says a tty will always have f->op->read == tty_read ?

Miloslav Trmac

author

2007-06-07 14:30 UTC #4 lore raw

Alan Cox napsal(a):

Quoted 4 lines

Quoted 3 lines
drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 397
Quoted 3 lines
drivers/char/n_tty.c +3 -0
+			if (filp->f_op->read == tty_read) {
+				disable = 0;
+				break;
Why says a tty will always have f->op->read == tty_read ?

AFAICS from tty_io.c, it will always be tty_read or hung_up_tty_read. Normal user processes would exit after SIGHUP and not reopen a TTY.

(I have copied the condition from __do_SAK(). That of course doesn't mean it's correct.)
Mirek

Alan Cox

2007-06-07 21:54 UTC #5 lore raw

Quoted 11 lines · On Thu, 07 Jun 2007 16:20:07 +0200 Miloslav Trmac <[email protected]> wrote:

Alan Cox napsal(a):
Quoted 4 lines
Quoted 3 lines
drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 397
Quoted 3 lines
drivers/char/n_tty.c +3 -0
+			if (filp->f_op->read == tty_read) {
+				disable = 0;
+				break;
Why says a tty will always have f->op->read == tty_read ?
AFAICS from tty_io.c, it will always be tty_read or hung_up_tty_read. Normal user processes would exit after SIGHUP and not reopen a TTY.
(I have copied the condition from __do_SAK(). That of course doesn't mean it's correct.)
Mirek

Right it may be hung_up_tty_read that was what bothered me. I've had a think through the different scenarios and I can't think of a simple one where I can abuse this as the vhangup() path is current root triggered and loses the tty (so I can't reopen on it)

There are more complex questions - what happens when the much needed revoke() goes mainstream [and we fix all the security issues its lack causes], and the case where I do

On tty1 run a process which sets nohup and causes a vhangup then opens tty2 while tty2 command line is running some long running program that doesn't take input that I could plausibly run legitimately (eg a long complex sql query, or a slow security check etc)

Alan

Miloslav Trmac

author

2007-06-08 04:20 UTC #6 lore raw

In reply to Andrew Morton

Thanks for the review.
Andrew Morton napsal(a):

Quoted 26 lines · On Wed, 06 Jun 2007 12:10:28 +0200 Miloslav Trmac <[email protected]> wrote:

Quoted 25 lines

drivers/char/n_tty.c +25 -0

+/**
+ *	tty_audit_opening	-	A TTY is being opened.
+ *
+ *	As a special hack, tasks that close all their TTYs and open new ones
+ *	are assumed to be system daemons (e.g. getty) and auditing is
+ *	automatically disabled for them.
+ */
+void
+tty_audit_opening(void)
+{
+	int disable;
+
+	disable = 1;
+	spin_lock(&current->sighand->siglock);
+	if (current->signal->audit_tty == 0)
+		disable = 0;
+	spin_unlock(&current->sighand->siglock);
+	if (!disable)
+		return;
+
+	task_lock(current);
+	if (current->files) {
+		struct fdtable *fdt;
+		unsigned i;
+

Quoted 8 lines

drivers/char/n_tty.c static inline void free_buf(unsigned char *buf) · line 387
Quoted 5 lines
drivers/char/n_tty.c +5 -0
+		/*
+		 * We don't take a ref to the file, so we must hold ->file_lock
+		 * instead.
+		 */
+		spin_lock(&current->files->file_lock);
So we make file_lock nest inside task_lock(). Was that lock ranking already being used elsewhere in the kernel, or is it a new association?

It is used in __do_SAK ().

Quoted 3 lines

Has this code had full coverage testing with all lockdep features enabled?
(I suspect not - lockdep should have gone wild over the siglock thing)

It was not. The new version will be.

Quoted 19 lines

kernel/audit.c +11 -0

index d13276d..a071a96 100644
@@ -423,6 +424,32 @@ static int kauditd_thread(void *dummy)
 	return 0;
 }
 
+static int
+audit_prepare_user_tty(pid_t pid, uid_t loginuid)
+{
+	struct task_struct *tsk;
+	int err;
+
+	read_lock(&tasklist_lock);
+	tsk = find_task_by_pid(pid);
+	err = -ESRCH;
+	if (!tsk)
+		goto out;

Quoted 8 lines

kernel/audit.c static int kauditd_thread(void *dummy) · line 443
Quoted 6 lines
kernel/audit.c +6 -0
+	err = 0;
+
+	spin_lock(&tsk->sighand->siglock);
+	if (!tsk->signal->audit_tty)
+		err = -EPERM;
+	spin_unlock(&tsk->sighand->siglock);
So siglock nests inside tasklist_lock? Sounds reasonable. Is this a preexisting association, or did this patch just create it?

This is used in send_sig_info() and several other functions in kernel/signal.c.

Quoted 8 lines

include/linux/sched.h +0 -0

index d58e74b..3ae4904 100644
@@ -506,6 +506,8 @@ struct signal_struct {
 #ifdef CONFIG_TASKSTATS
 	struct taskstats *stats;
 #endif

Quoted 8 lines

include/linux/sched.h struct signal_struct { · line 511
Quoted 3 lines
include/linux/sched.h +2 -0
+	unsigned audit_tty:1;
+	struct tty_audit_buf *tty_audit_buf;
 };
hm, bitfields are risky. If someone adds another one, it will land in the same word and external locking will be needed. You do seem to be using ->siglock to cover this - was that to address the bitfield non-atomicity problem?

I don't know what the memory access atomicity assumptions are in the kernel, so I have used the basic rule that any write<->read conflict on a variable with type other than atomic_t must be prevented by a lock. This happens to work for the bit field as well.

Quoted 2 lines

A suitable (but somewhat less pretty) way to resolve all this is to not use bitfields at all: add `unsigned long flags' and use set_bit/clear_bit/etc.

The new patch replaces the bit field by a simple "unsigned", a whole word is allocated for the bit field anyway.

Quoted 17 lines

Quoted 4 lines
include/linux/sched.h +2 -0
 		break;
+	case AUDIT_TTY_GET: {
+		struct audit_tty_status s;
include/linux/sched.h struct signal_struct {
Quoted 10 lines
include/linux/sched.h +10 -0
+		struct task_struct *tsk;
+
+		read_lock(&tasklist_lock);
+		tsk = find_task_by_pid(pid);
+		if (!tsk)
+			err = -ESRCH;
+		else {
+			spin_lock(&tsk->sighand->siglock);
+			s.enabled = tsk->signal->audit_tty != 0;
+			spin_unlock(&tsk->sighand->siglock);
The locking here looks dubious. tsk->signal->audit_tty can change state the instant ->siglock gets unlocked, in which case s.enabled is now wrong.

The user-space process must avoid concurrent AUDIT_TTY_SET to get reasonable results. There's nothing better the kernel can do.

Quoted 1 line

If that is acceptable then we didn't need that locking at all.

So I can assume that int-sized reads are always atomic with respect to concurrent writes?
Mirek

Miloslav Trmac

author

2007-06-08 04:25 UTC #7 lore raw

PATCH v2 1/1 Audit: Add TTY input auditing

In reply to Andrew Morton

From: Miloslav Trmac <[email protected]>

See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.

---
Changes since the previous patch:
* use spin_lock_irq() for siglock
* add an is_tty() function instead of checking f_op->read from n_tty.c; handle hung TTYs
* replace the audit_tty bit field by a whole word to avoid the risk of incorrect locking
* move most new code from n_tty.c to a separate file
* fix coding style violations
* fix compilation with !CONFIG_AUDIT

From: Miloslav Trmac <[email protected]>

See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.

Signed-off-by Miloslav Trmac <[email protected]>

---
Changes since the previous patch:
* use spin_lock_irq() for siglock
* add an is_tty() function instead of checking f_op->read from n_tty.c; handle hung TTYs
* replace the audit_tty bit field by a whole word to avoid the risk of incorrect locking
* most code from n_tty.c split to a separate file
* fix coding style violations
* fix compilation with !CONFIG_AUDIT

 drivers/char/Makefile        |    1 
 drivers/char/n_tty.c         |   20 +
 drivers/char/tty_audit.c     |  332 +++++++++++++++++++++++++++++++++
 drivers/char/tty_io.c        |   14 +
 include/linux/audit.h        |   11 +
 include/linux/sched.h        |    2 
 include/linux/tty.h          |   28 ++
 kernel/audit.c               |   96 ++++++++-
 kernel/audit.h               |    1 
 kernel/auditsc.c             |    3 
 kernel/exit.c                |    2 
 kernel/fork.c                |    5 
 net/netlabel/netlabel_user.c |    4 
 security/selinux/nlmsgtab.c  |    2

drivers/char/Makefile +1 -0

index 2f56ecc..f2996a9 100644
@@ -15,6 +15,7 @@ obj-y				+= misc.o
 obj-$(CONFIG_VT)		+= vt_ioctl.o vc_screen.o consolemap.o \
 				   consolemap_deftbl.o selection.o keyboard.o
 obj-$(CONFIG_HW_CONSOLE)	+= vt.o defkeymap.o
+obj-$(CONFIG_AUDIT)		+= tty_audit.o
 obj-$(CONFIG_MAGIC_SYSRQ)	+= sysrq.o
 obj-$(CONFIG_ESPSERIAL)		+= esp.o
 obj-$(CONFIG_MVME147_SCC)	+= generic_serial.o vme_scc.o

drivers/char/n_tty.c +16 -4

index 154f422..4e71791 100644
@@ -45,6 +45,8 @@
 #include <linux/slab.h>
 #include <linux/poll.h>
 #include <linux/bitops.h>
+#include <linux/audit.h>
+#include <linux/file.h>
 
 #include <asm/uaccess.h>
 #include <asm/system.h>

@@ -78,6 +80,13 @@ static inline void free_buf(unsigned char *buf)
 		free_page((unsigned long) buf);
 }
 
+static inline int tty_put_user(struct tty_struct *tty, unsigned char x,
+			       unsigned char __user *ptr)
+{
+	tty_audit_add_data(tty, &x, 1);
+	return put_user(x, ptr);
+}
+
 /**
  *	n_tty_set__room	-	receive space
  *	@tty: terminal

@@ -1153,6 +1162,7 @@ static int copy_from_read_buf(struct tty_struct *tty,
 	if (n) {
 		retval = copy_to_user(*b, &tty->read_buf[tty->read_tail], n);
 		n -= retval;
+		tty_audit_add_data(tty, &tty->read_buf[tty->read_tail], n);
 		spin_lock_irqsave(&tty->read_lock, flags);
 		tty->read_tail = (tty->read_tail + n) & (N_TTY_BUF_SIZE-1);
 		tty->read_cnt -= n;

@@ -1279,7 +1289,7 @@ do_it_again:
 				break;
 			cs = tty->link->ctrl_status;
 			tty->link->ctrl_status = 0;
-			if (put_user(cs, b++)) {
+			if (tty_put_user(tty, cs, b++)) {
 				retval = -EFAULT;
 				b--;
 				break;

@@ -1321,7 +1331,7 @@ do_it_again:
 
 		/* Deal with packet mode. */
 		if (tty->packet && b == buf) {
-			if (put_user(TIOCPKT_DATA, b++)) {
+			if (tty_put_user(tty, TIOCPKT_DATA, b++)) {
 				retval = -EFAULT;
 				b--;
 				break;

@@ -1352,15 +1362,17 @@ do_it_again:
 				spin_unlock_irqrestore(&tty->read_lock, flags);
 
 				if (!eol || (c != __DISABLED_CHAR)) {
-					if (put_user(c, b++)) {
+					if (tty_put_user(tty, c, b++)) {
 						retval = -EFAULT;
 						b--;
 						break;
 					}
 					nr--;
 				}
-				if (eol)
+				if (eol) {
+					tty_audit_push(tty);
 					break;
+				}
 			}
 			if (retval)
 				break;

drivers/char/tty_audit.c new file +332 -0

332 lines not on this page · raw message

drivers/char/tty_io.c +13 -1

index 3752edc..e57567d 100644
@@ -1497,6 +1497,15 @@ int tty_hung_up_p(struct file * filp)
 
 EXPORT_SYMBOL(tty_hung_up_p);
 
+/**
+ * is_tty	-	checker whether file is a TTY
+ */
+int is_tty(struct file *filp)
+{
+	return filp->f_op->read == tty_read
+		|| filp->f_op->read == hung_up_tty_read;
+}
+
 static void session_clear_tty(struct pid *session)
 {
 	struct task_struct *p;

@@ -2654,6 +2663,7 @@ got_driver:
 		__proc_set_tty(current, tty);
 	spin_unlock_irq(&current->sighand->siglock);
 	mutex_unlock(&tty_mutex);
+	tty_audit_opening();
 	return 0;
 }

@@ -2716,8 +2726,10 @@ static int ptmx_open(struct inode * inode, struct file * filp)
 
 	check_tty_count(tty, "tty_open");
 	retval = ptm_driver->open(tty, filp);
-	if (!retval)
+	if (!retval) {
+		tty_audit_opening();
 		return 0;
+	}
 out1:
 	release_dev(filp);
 	return retval;

include/linux/audit.h +11 -0

index fccc6e5..dbf2125 100644
@@ -63,9 +63,12 @@
 #define AUDIT_ADD_RULE		1011	/* Add syscall filtering rule */
 #define AUDIT_DEL_RULE		1012	/* Delete syscall filtering rule */
 #define AUDIT_LIST_RULES	1013	/* List syscall filtering rules */
+#define AUDIT_TTY_GET		1014	/* Get TTY auditing status */
+#define AUDIT_TTY_SET		1015	/* Set TTY auditing status */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */
+#define AUDIT_USER_TTY		1124	/* Non-ICANON TTY input meaning */
 #define AUDIT_LAST_USER_MSG	1199
 #define AUDIT_FIRST_USER_MSG2	2100	/* More user space messages */
 #define AUDIT_LAST_USER_MSG2	2999

@@ -92,6 +95,7 @@
 #define AUDIT_KERNEL_OTHER	1316	/* For use by 3rd party modules */
 #define AUDIT_FD_PAIR		1317    /* audit record for pipe/socketpair */
 #define AUDIT_OBJ_PID		1318	/* ptrace target */
+#define AUDIT_TTY		1319	/* Input on an administrative TTY */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */

@@ -289,6 +293,10 @@ struct audit_status {
 	__u32		backlog;	/* messages waiting in queue */
 };
 
+struct audit_tty_status {
+	__u32		enabled; /* 1 = enabled, 0 = disabled */
+};
+
 /* audit_rule_data supports filter rules with both integer and string
  * fields.  It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
  * AUDIT_LIST_RULES requests.

@@ -455,6 +463,7 @@ static inline int audit_mq_getsetattr(mqd_t mqdes, struct mq_attr *mqstat)
 		return __audit_mq_getsetattr(mqdes, mqstat);
 	return 0;
 }
+extern int audit_enabled;
 extern int audit_n_rules;
 extern int audit_signals;
 #else

@@ -487,6 +496,7 @@ extern int audit_signals;
 #define audit_mq_notify(d,n) ({ 0; })
 #define audit_mq_getsetattr(d,s) ({ 0; })
 #define audit_ptrace(t) ((void)0)
+#define audit_enabled 0
 #define audit_n_rules 0
 #define audit_signals 0
 #endif

@@ -515,6 +525,7 @@ extern void		    audit_log_d_path(struct audit_buffer *ab,
 					     const char *prefix,
 					     struct dentry *dentry,
 					     struct vfsmount *vfsmnt);
+extern void		    audit_log_lost(const char *message);
 				/* Private API (for audit.c only) */
 extern int audit_filter_user(struct netlink_skb_parms *cb, int type);
 extern int audit_filter_type(int type);

include/linux/sched.h +2 -0

index d58e74b..d9d734c 100644
@@ -506,6 +506,8 @@ struct signal_struct {
 #ifdef CONFIG_TASKSTATS
 	struct taskstats *stats;
 #endif
+	unsigned audit_tty;
+	struct tty_audit_buf *tty_audit_buf;
 };
 
 /* Context switch must be unlocked if interrupts are to be enabled */

include/linux/tty.h +28 -0

index bb45760..1d19dae 100644
@@ -310,6 +310,7 @@ extern void tty_hangup(struct tty_struct * tty);
 extern void tty_vhangup(struct tty_struct * tty);
 extern void tty_unhangup(struct file *filp);
 extern int tty_hung_up_p(struct file * filp);
+extern int is_tty(struct file *filp);
 extern void do_SAK(struct tty_struct *tty);
 extern void __do_SAK(struct tty_struct *tty);
 extern void disassociate_ctty(int priv);

@@ -341,6 +342,33 @@ extern struct mutex tty_mutex;
 /* n_tty.c */
 extern struct tty_ldisc tty_ldisc_N_TTY;
 
+/* tty_audit.c */
+#ifdef CONFIG_AUDIT
+extern void tty_audit_add_data(struct tty_struct *tty, unsigned char *data,
+			       size_t size);
+extern void tty_audit_exit(void);
+extern void tty_audit_push(struct tty_struct *tty);
+extern void tty_audit_push_task(struct task_struct *tsk, uid_t loginuid);
+extern void tty_audit_opening(void);
+#else
+static inline void tty_audit_add_data(struct tty_struct *tty,
+				      unsigned char *data, size_t size)
+{
+}
+static inline void tty_audit_exit(void)
+{
+}
+static inline void tty_audit_push(struct tty_struct *tty)
+{
+}
+static inline void tty_audit_push_task(struct task_struct *tsk, uid_t loginuid)
+{
+}
+static inline void tty_audit_opening(void)
+{
+}
+#endif
+
 /* tty_ioctl.c */
 extern int n_tty_ioctl(struct tty_struct * tty, struct file * file,
 		       unsigned int cmd, unsigned long arg);

kernel/audit.c +88 -8

158 lines not on this page · raw message

kernel/audit.h +0 -1

index 815d6f5..9587743 100644
@@ -115,7 +115,6 @@ extern struct sk_buff *	    audit_make_reply(int pid, int seq, int type,
 extern void		    audit_send_reply(int pid, int seq, int type,
 					     int done, int multi,
 					     void *payload, int size);
-extern void		    audit_log_lost(const char *message);
 extern void		    audit_panic(const char *message);
 
 struct audit_netlink_list {

kernel/auditsc.c +0 -3

index e36481e..7ccc3da 100644
@@ -71,9 +71,6 @@
 
 extern struct list_head audit_filter_list[];
 
-/* No syscall auditing will take place unless audit_enabled != 0. */
-extern int audit_enabled;
-
 /* AUDIT_NAMES is the number of slots we reserve in the audit_context
  * for saving names from getname(). */
 #define AUDIT_NAMES    20

kernel/exit.c +2 -0

index 5b888c2..413f6df 100644
@@ -922,6 +922,8 @@ fastcall NORET_TYPE void do_exit(long code)
 	if (unlikely(tsk->compat_robust_list))
 		compat_exit_robust_list(tsk);
 #endif
+	if (group_dead && unlikely(tsk->signal->tty_audit_buf))
+		tty_audit_exit();
 	if (unlikely(tsk->audit_context))
 		audit_free(tsk);

kernel/fork.c +5 -0

index 73ad5cd..8d6135c 100644
@@ -897,6 +897,11 @@ static inline int copy_signal(unsigned long clone_flags, struct task_struct * ts
 	}
 	acct_init_pacct(&sig->pacct);
 
+	spin_lock_irq(&current->sighand->siglock);
+	sig->audit_tty = current->signal->audit_tty;
+	spin_unlock_irq(&current->sighand->siglock);
+	sig->tty_audit_buf = NULL;
+
 	return 0;
 }

net/netlabel/netlabel_user.c +0 -4

index 42f12bd..89dcc48 100644
@@ -46,10 +46,6 @@
 #include "netlabel_cipso_v4.h"
 #include "netlabel_user.h"
 
-/* do not do any auditing if audit_enabled == 0, see kernel/audit.c for
- * details */
-extern int audit_enabled;
-
 /*
  * NetLabel NETLINK Setup Functions
  */

security/selinux/nlmsgtab.c +2 -0

index ccfe875..eddc7b4 100644
@@ -110,6 +110,8 @@ static struct nlmsg_perm nlmsg_audit_perms[] =
 	{ AUDIT_DEL_RULE,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 	{ AUDIT_USER,		NETLINK_AUDIT_SOCKET__NLMSG_RELAY    },
 	{ AUDIT_SIGNAL_INFO,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
+	{ AUDIT_TTY_GET,	NETLINK_AUDIT_SOCKET__NLMSG_READ     },
+	{ AUDIT_TTY_SET,	NETLINK_AUDIT_SOCKET__NLMSG_WRITE    },
 };

Andrew Morton

maintainer

2007-06-08 06:32 UTC #8 lore raw

Quoted 41 lines · On Fri, 08 Jun 2007 06:23:23 +0200 Miloslav Trmac <[email protected]> wrote:

From: Miloslav Trmac <[email protected]>
Add TTY input auditing, used to audit system administrator's actions. TTY input auditing works on a higher level than auditing all system calls within the session, which would produce an overwhelming amount of mostly useless audit events.
Add an "audit_tty" attribute, inherited across fork (). Data read from TTYs by process with the attribute is sent to the audit subsystem by the kernel. The audit netlink interface is extended to allow modifying the audit_tty attribute, and to allow sending explanatory audit events from user-space (for example, a shell might send an event containing the final command, after the interactive command-line editing and history expansion is performed, which might be difficult to decipher from the TTY input alone).
Because the "audit_tty" attribute is inherited across fork (), it would be set e.g. for sshd restarted within an audited session. To prevent this, the audit_tty attribute is cleared when a process with no open TTY file descriptors (e.g. after daemon startup) opens a TTY.
See https://www.redhat.com/archives/linux-audit/2007-June/msg00000.html
for a more detailed rationale document for an older version of this patch.
---
Changes since the previous patch:
* use spin_lock_irq() for siglock
* add an is_tty() function instead of checking f_op->read from n_tty.c; handle hung TTYs
* replace the audit_tty bit field by a whole word to avoid the risk of incorrect locking
* move most new code from n_tty.c to a separate file
* fix coding style violations
* fix compilation with !CONFIG_AUDIT
include/linux/sched.h +0 -0
index d58e74b..d9d734c 100644

include/linux/sched.h struct signal_struct { · line 511

Quoted 6 lines

include/linux/sched.h +2 -0

@@ -506,6 +506,8 @@ struct signal_struct {
 #ifdef CONFIG_TASKSTATS
 	struct taskstats *stats;
 #endif
+	unsigned audit_tty;
+	struct tty_audit_buf *tty_audit_buf;
 };

Can we ifdef these?

Miloslav Trmac

author

2007-06-08 16:07 UTC #9 lore raw

Andrew Morton napsal(a):

Quoted 6 lines · On Fri, 08 Jun 2007 06:23:23 +0200 Miloslav Trmac <[email protected]> wrote:

Quoted 5 lines
include/linux/sched.h +0 -0
index d58e74b..d9d734c 100644
@@ -506,6 +506,8 @@ struct signal_struct {

Quoted 8 lines

include/linux/sched.h struct signal_struct { · line 511
Quoted 6 lines
include/linux/sched.h +2 -0
 #ifdef CONFIG_TASKSTATS
 	struct taskstats *stats;
 #endif
+	unsigned audit_tty;
+	struct tty_audit_buf *tty_audit_buf;
 };
Can we ifdef these?

Sure, here's an incremental patch.
Mirek

From: Miloslav Trmac <[email protected]>

Only add TTY audit state to struct signal_struct if CONFIG_AUDIT. Move the copying of TTY audit state on fork () to tty_audit.c.

Signed-off-by Miloslav Trmac <[email protected]>

---
 drivers/char/tty_audit.c |   13 +++++++++++++
 include/linux/sched.h    |    2 ++
 include/linux/tty.h      |    5 +++++
 kernel/exit.c            |    2 +-
 kernel/fork.c            |    6 ++----
 5 files changed, 23 insertions(+), 5 deletions(-)

drivers/char/tty_audit.c +13 -0

diff -u b/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
@@ -134,6 +134,19 @@
 }
 
 /**
+ *	tty_audit_fork	-	Copy TTY audit state for a new task
+ *
+ *	Set up TTY audit state in @sig from current.  @sig needs no locking.
+ */
+void tty_audit_fork(struct signal_struct *sig)
+{
+	spin_lock_irq(&current->sighand->siglock);
+	sig->audit_tty = current->signal->audit_tty;
+	spin_unlock_irq(&current->sighand->siglock);
+	sig->tty_audit_buf = NULL;
+}
+
+/**
  *	tty_audit_push_task	-	Flush task's pending audit data
  */
 void tty_audit_push_task(struct task_struct *tsk, uid_t loginuid)

include/linux/sched.h +2 -0

diff -u b/include/linux/sched.h b/include/linux/sched.h
@@ -506,8 +506,10 @@
 #ifdef CONFIG_TASKSTATS
 	struct taskstats *stats;
 #endif
+#ifdef CONFIG_AUDIT
 	unsigned audit_tty;
 	struct tty_audit_buf *tty_audit_buf;
+#endif
 };
 
 /* Context switch must be unlocked if interrupts are to be enabled */

include/linux/tty.h +5 -0

diff -u b/include/linux/tty.h b/include/linux/tty.h
@@ -178,6 +178,7 @@
 #define L_IEXTEN(tty)	_L_FLAG((tty),IEXTEN)
 
 struct device;
+struct signal_struct;
 /*
  * Where all of the state associated with a tty is kept while the tty
  * is open.  Since the termios state should be kept even if the tty

@@ -347,6 +348,7 @@
 extern void tty_audit_add_data(struct tty_struct *tty, unsigned char *data,
 			       size_t size);
 extern void tty_audit_exit(void);
+extern void tty_audit_fork(struct signal_struct *sig);
 extern void tty_audit_push(struct tty_struct *tty);
 extern void tty_audit_push_task(struct task_struct *tsk, uid_t loginuid);
 extern void tty_audit_opening(void);

@@ -358,6 +360,9 @@
 static inline void tty_audit_exit(void)
 {
 }
+static inline void tty_audit_fork(struct signal_struct *sig)
+{
+}
 static inline void tty_audit_push(struct tty_struct *tty)
 {
 }

kernel/exit.c +1 -1

diff -u b/kernel/exit.c b/kernel/exit.c
@@ -922,7 +922,7 @@
 	if (unlikely(tsk->compat_robust_list))
 		compat_exit_robust_list(tsk);
 #endif
-	if (group_dead && unlikely(tsk->signal->tty_audit_buf))
+	if (group_dead)
 		tty_audit_exit();
 	if (unlikely(tsk->audit_context))
 		audit_free(tsk);

kernel/fork.c +2 -4

diff -u b/kernel/fork.c b/kernel/fork.c
@@ -49,6 +49,7 @@
 #include <linux/delayacct.h>
 #include <linux/taskstats_kern.h>
 #include <linux/random.h>
+#include <linux/tty.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -897,10 +898,7 @@
 	}
 	acct_init_pacct(&sig->pacct);
 
-	spin_lock_irq(&current->sighand->siglock);
-	sig->audit_tty = current->signal->audit_tty;
-	spin_unlock_irq(&current->sighand->siglock);
-	sig->tty_audit_buf = NULL;
+	tty_audit_fork(sig);
 
 	return 0;
 }

Jan Engelhardt

2007-06-07 08:18 UTC #10 lore raw

In reply to Miloslav Trmac

Quoted 3 lines · Miloslav Trmac

From: Miloslav Trmac <[email protected]>
Add TTY input auditing, used to audit system administrator's actions.

_What_ exactly does it audit?
And why does it only audit sysadmin actions?
Is this supposed to be a keylogger?

Quoted 1 line

TTY input auditing works on a higher level than auditing all system

What about tty output?

Quoted 3 lines

calls within the session, which would produce an overwhelming amount of mostly useless audit events.

Jan

Steve Grubb

2007-06-07 10:50 UTC #11 lore raw

Quoted 3 lines · On Thursday 07 June 2007 04:13:42 Jan Engelhardt wrote:

Quoted 1 line
Add TTY input auditing, used to audit system administrator's actions.
_What_ exactly does it audit?

In theory, it should audit the actions performed by the sysadmin. This patch doesn't cover actions done via X windows interface.

Quoted 1 line

And why does it only audit sysadmin actions?

This is what's called out for in various security standards such as NISPOM, DCID 6/3, and PCI. Its all about non-repudiation.

Quoted 1 line

Is this supposed to be a keylogger?

Sort of. Its supposed to allow the Security Officer the ability to check on what an admin was doing if they were found to be attempting access to information outside their duties. Or if something is found to be misconfigured, they can backtrack and see how it became that way if that was important.

Some people try to be compliant with these security standards with user space tools like rootsh, but that is too easy to detect and defeat. And then it does not put its data into the audit system where its correlated with other system events.

Quoted 1 line

What about tty output?

That is not required.

-Steve

Casey Schaufler

2007-06-07 15:42 UTC #12 lore raw

Quoted 21 lines · --- Steve Grubb

Quoted 3 lines · On Thursday 07 June 2007 04:13:42 Jan Engelhardt wrote:
Quoted 1 line
Add TTY input auditing, used to audit system administrator's actions.
_What_ exactly does it audit?
In theory, it should audit the actions performed by the sysadmin. This patch doesn't cover actions done via X windows interface.
Quoted 1 line
And why does it only audit sysadmin actions?
This is what's called out for in various security standards such as NISPOM, DCID 6/3, and PCI. Its all about non-repudiation.
Quoted 1 line
Is this supposed to be a keylogger?
Sort of. Its supposed to allow the Security Officer the ability to check on what an admin was doing if they were found to be attempting access to information outside their duties. Or if something is found to be misconfigured, they can backtrack and see how it became that way if that was important.

Quoted 5 lines

Some people try to be compliant with these security standards with user space
tools like rootsh, but that is too easy to detect and defeat. And then it does not put its data into the audit system where its correlated with other system events.

The evaluation teams that I have worked with (OrangeBook and CC) as well as their technical advisory groups have always been clear that keyboard logging is not an appropriate mechanism for security audit logging. There is insufficient correlation between what is typed and security relevent actions for keyboard (or mouse event) logging to meet the audit requirements. You have to log what happened. Logging what was requested is insufficient and logging what was typed, which may or may not have resulted in an actual request is not helpful to meeting security audit requirements.

Quoted 6 lines

Quoted 1 line
What about tty output?
That is not required.
(quoted diff) +0 -2
-Steve
-

Quoted 7 lines

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Casey Schaufler
[email protected]

Alan Cox

2007-06-07 15:52 UTC #13 lore raw

Quoted 4 lines

logging to meet the audit requirements. You have to log what happened. Logging what was requested is insufficient and logging what was typed, which may or may not have resulted in an actual request is not helpful to meeting security audit requirements.

Key information can answer some questions as an additional adjunct, but all of this really depends whether your security audit requirement is to impress the monkeys or to protect your systems (or as usually is the case to protect your system while still impressing a bunch of half-trained 19 year old box tickers on their first outing doing auditing)

Steve Grubb

2007-06-07 16:35 UTC #14 lore raw

In reply to Casey Schaufler

Quoted 10 lines · Casey Schaufler

Quoted 3 lines
tools like rootsh, but that is too easy to detect and defeat. And then it does not put its data into the audit system where its correlated with other system events.
The evaluation teams that I have worked with (OrangeBook and CC) as well as their technical advisory groups have always been clear that keyboard logging is not an appropriate mechanism for security audit logging. There is insufficient correlation between what is typed and security relevent actions for keyboard (or mouse event) logging to meet the audit requirements.

Ok, this is a sample set of requirements we are trying to meet:

Implement automated audit trails for all system components to reconstruct the following events:
All actions taken by any individual with root or administrative privileges

If we do not get commands typed at a prompt, we have to audit by execve. Auditing execve for uid = 0 produces millions of events since that includes daemons. If we get clever and restrict auditing to events for root uid and auid >= 500, we still wind up with millions of events because of shell scripts.

People in control of some of these security targets said to me that auditing by execve cannot be the solution, because the audit trail becomes too big, unwieldy, full of irrelavent events, and not easily analysed. So, that approach does not work for people either.

Casey, so what approach would you take to meet the requirement?

Quoted 1 line

You have to log what happened.

Which can be done by auditing for execution of specific apps or watching access to certain files. So, I don't see tty auditing as something that replaces other auditing, it allows us to form a better picture of what happened to the system.

Quoted 3 lines

Logging what was requested is insufficient and logging what was typed, which may or may not have resulted in an actual request is not helpful to meeting security audit requirements.

I would disagree. Its helpful to complete the picture of what's happening on the system.

-Steve

Casey Schaufler

2007-06-07 17:33 UTC #15 lore raw

Quoted 6 lines · --- Steve Grubb

Ok, this is a sample set of requirements we are trying to meet:
Implement automated audit trails for all system components to reconstruct the
following events:
All actions taken by any individual with root or administrative privileges

My knee jerk response is "good luck with that". I hope you're not planning a GUI, because tracing the mouse movements, clicks, and window placement in order to identify what action the admin is taking is going to be a without-a-net trick.

Quoted 1 line

If we do not get commands typed at a prompt, we have to audit by execve.

I would suggest that you'll have to do that as well so that you can tell the difference between typed actions like these:

# cat > /dev/null
badprogram --badthing --everyone
^D
#

# badprogram --badthing --everyone

where the same typed line is a Bad Thing in one case and completely irrelevent in the other.

Quoted 4 lines

Auditing execve for uid = 0 produces millions of events since that includes daemons. If we get clever and restrict auditing to events for root uid and auid >= 500, we still wind up with millions of events because of shell scripts.

Yes. Your Linux system performs millions (yea, billions) of security relevent actions in the time it takes you (well, me) to spell check an lkml posting. Audit systems spew lots of data because there's lots of data that is required to track who did what to whom.

Quoted 4 lines

People in control of some of these security targets said to me that auditing by execve cannot be the solution, because the audit trail becomes too big, unwieldy, full of irrelavent events, and not easily analysed. So, that approach does not work for people either.

OK. They're not willing to pay the price for the result they are asking for.

Quoted 1 line

Casey, so what approach would you take to meet the requirement?

If the requirement is that all human initiated administrative actions be audited your only option is to seriously restrict the actions that the human can perform by hand. This is usually accomplished by providing an administration user interface facility as the admin shell. This shell only invokes a tightly controlled set of commands, and audits their parameters.

Quoted 6 lines

Quoted 1 line
You have to log what happened.
Which can be done by auditing for execution of specific apps or watching access to certain files. So, I don't see tty auditing as something that replaces other auditing, it allows us to form a better picture of what happened to the system.

Perhaps.

Quoted 6 lines

Quoted 3 lines
Logging what was requested is insufficient and logging what was typed, which may or may not have resulted in an actual request is not helpful to meeting security audit requirements.
I would disagree. Its helpful to complete the picture of what's happening on the system.

As an adjunct to system level audit it might be helpful, but it cannot stand on its own.

Casey Schaufler
[email protected]

Miloslav Trmac

author

2007-06-07 19:30 UTC #16 lore raw

Casey Schaufler napsal(a):

Quoted 9 lines

Quoted 1 line
If we do not get commands typed at a prompt, we have to audit by execve.
I would suggest that you'll have to do that as well so that you can tell the difference between typed actions like these:
# cat > /dev/null
badprogram --badthing --everyone
^D
#

Quoted 4 lines

# badprogram --badthing --everyone
where the same typed line is a Bad Thing in one case and completely irrelevent in the other.

The proposed patch audits each process separately, and includes a part of the command name in the audit event, so it is easy to distinguish between data entered into (cat > /dev/null) and the shell.

The command name can be faked, but the actions necessary to fake the command name would be audited.
Mirek

Jan Engelhardt

2007-06-07 21:09 UTC #17 lore raw

Quoted 12 lines · Miloslav Trmac

Casey Schaufler napsal(a):
Quoted 11 lines
Quoted 1 line
If we do not get commands typed at a prompt, we have to audit by execve.
I would suggest that you'll have to do that as well so that you can tell the difference between typed actions like these:
# cat > /dev/null
badprogram --badthing --everyone
^D
#
# badprogram --badthing --everyone

Quoted 8 lines

Quoted 2 lines
where the same typed line is a Bad Thing in one case and completely irrelevent in the other.
The proposed patch audits each process separately, and includes a part of the command name in the audit event, so it is easy to distinguish between data entered into (cat > /dev/null) and the shell.
The command name can be faked, but the actions necessary to fake the command name would be audited.

Someone please enlighten me why a regular keylogger² that captures both input and output could not do the same. (Auditing what one has done.)

² http://ttyrpld.sf.net (there are others too)

Jan

Casey Schaufler

2007-06-07 22:32 UTC #18 lore raw

Quoted 2 lines · --- Jan Engelhardt

Someone please enlighten me why a regular keylogger² that captures both input and output could not do the same. (Auditing what one has done.)

1. shell aliases

   # innocuous -p 0 
2. shell variables
   # $INNOCUOUS -p 0
3. symlinks
   # ./innocuous -p 0

Yes, I know there are ways to prevent each of these "attacks", but it's surprising how often simple textual changes are effective in hiding behavior.

Casey Schaufler
[email protected]