Re: [PATCH] Add SELinux context and TTY name to AUDIT_TTY records

Re: [PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Miloslav Trmac]

Paul,
Thanks for your review.
----- "Paul Moore" <paul.moore@hp.com> wrote:
> There are several audit experts which should review this code but two
> things 
> jumped out at me when glancing at your patch:
> 
>  1. SELinux SIDs should not be recorded
Almost all code that logs SELinux contexts in kernel/audit* does the same thing as this patch, falling back to a SID if it can't be converted to a string.

>  2. From a SELinux/security point of view ttys are considered objects
>     and their labels/contexts should be recorded with "obj=" not
> "subj="
The patch logs the context of the process, not of the TTY.
    Mirek

Re: [PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Miloslav Trmac]

Paul,
Thanks for your review.
----- "Paul Moore" <paul.moore@hp.com> wrote:
> There are several audit experts which should review this code but two
> things 
> jumped out at me when glancing at your patch:
> 
>  1. SELinux SIDs should not be recorded
Almost all code that logs SELinux contexts in kernel/audit* does the same thing as this patch, falling back to a SID if it can't be converted to a string.

>  2. From a SELinux/security point of view ttys are considered objects
>     and their labels/contexts should be recorded with "obj=" not
> "subj="
The patch logs the context of the process, not of the TTY.
    Mirek

Re: [PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Paul Moore]

On Friday 20 March 2009 04:53:27 am Miloslav Trmac wrote:
> ----- "Paul Moore" <paul.moore@hp.com> wrote:
> > There are several audit experts which should review this code but two
> > things jumped out at me when glancing at your patch:
> >
> >  1. SELinux SIDs should not be recorded
>
> Almost all code that logs SELinux contexts in kernel/audit* does the same
> thing as this patch, falling back to a SID if it can't be converted to a
> string.

Ungh, that's ugly and questionably useful (I suppose I know why this is done) 
but if that convention then who am I to argue.

> >  2. From a SELinux/security point of view ttys are considered objects
> >     and their labels/contexts should be recorded with "obj=" not
> > "subj="
>
> The patch logs the context of the process, not of the TTY.

Okay, that is what I get for just glancing at patches and not looking at them 
closer :)

-- 
paul moore
linux @ hp

Re: [PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Paul Moore]

On Friday 20 March 2009 04:53:27 am Miloslav Trmac wrote:
> ----- "Paul Moore" <paul.moore@hp.com> wrote:
> > There are several audit experts which should review this code but two
> > things jumped out at me when glancing at your patch:
> >
> >  1. SELinux SIDs should not be recorded
>
> Almost all code that logs SELinux contexts in kernel/audit* does the same
> thing as this patch, falling back to a SID if it can't be converted to a
> string.

Ungh, that's ugly and questionably useful (I suppose I know why this is done) 
but if that convention then who am I to argue.

> >  2. From a SELinux/security point of view ttys are considered objects
> >     and their labels/contexts should be recorded with "obj=" not
> > "subj="
>
> The patch logs the context of the process, not of the TTY.

Okay, that is what I get for just glancing at patches and not looking at them 
closer :)

-- 
paul moore
linux @ hp

[PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Miloslav Trmac]

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

From: Miloslav Trmač <mitr@redhat.com>

Add SELinux context information and TTY name (consistent with the
AUDIT_SYSCALL record) to AUDIT_TTY.  An example record after applying
this patch:

 type=TTY msg=audit(1237480806.220:22): tty pid=2601 uid=0 auid=500 ses=1 
 subj=unconfined_u:unconfined_r:unconfined_t:s0 major=136 minor=1 tty=pts1 
 comm="bash" data=6361740D

(line wrapped, new fields are "subj" and "tty".)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 drivers/char/tty_audit.c |   57 ++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 19 deletions(-)

[-- Attachment #2: audit-tty-more-fields.patch --]
[-- Type: application/octet-stream, Size: 4652 bytes --]

From: Miloslav Trmač <mitr@redhat.com>

Add SELinux context information and TTY name (consistent with the
AUDIT_SYSCALL record) to AUDIT_TTY.  An example record after applying
this patch:

 type=TTY msg=audit(1237480806.220:22): tty pid=2601 uid=0 auid=500 ses=1
 subj=unconfined_u:unconfined_r:unconfined_t:s0 major=136 minor=1 tty=pts1
 comm="bash" data=6361740D

(line wrapped, new fields are "subj" and "tty".)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 drivers/char/tty_audit.c |   57 ++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
index 34ab6d7..1b7add4 100644
--- a/drivers/char/tty_audit.c
+++ b/drivers/char/tty_audit.c
@@ -12,6 +12,7 @@
 #include <linux/audit.h>
 #include <linux/file.h>
 #include <linux/fdtable.h>
+#include <linux/security.h>
 #include <linux/tty.h>
 
 struct tty_audit_buf {
@@ -19,12 +20,21 @@ struct tty_audit_buf {
 	struct mutex mutex;	/* Protects all data below */
 	int major, minor;	/* The TTY which the data is from */
 	unsigned icanon:1;
+	char tty_name[sizeof(((struct tty_struct *)NULL)->name)];
 	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)
+static void tty_audit_buf_setup(struct tty_audit_buf *buf,
+				struct tty_struct *tty)
+{
+	buf->major = tty->driver->major;
+	buf->minor = tty->driver->minor_start + tty->index;
+	buf->icanon = tty->icanon;
+	strcpy(buf->tty_name, tty->name);
+}
+
+static struct tty_audit_buf *tty_audit_buf_alloc(struct tty_struct *tty)
 {
 	struct tty_audit_buf *buf;
 
@@ -39,9 +49,7 @@ static struct tty_audit_buf *tty_audit_buf_alloc(int major, int minor,
 		goto err_buf;
 	atomic_set(&buf->count, 1);
 	mutex_init(&buf->mutex);
-	buf->major = major;
-	buf->minor = minor;
-	buf->icanon = icanon;
+	tty_audit_buf_setup(buf, tty);
 	buf->valid = 0;
 	return buf;
 
@@ -69,7 +77,8 @@ static void tty_audit_buf_put(struct tty_audit_buf *buf)
 
 static void tty_audit_log(const char *description, struct task_struct *tsk,
 			  uid_t loginuid, unsigned sessionid, int major,
-			  int minor, unsigned char *data, size_t size)
+			  int minor, const char *tty_name,
+			  unsigned char *data, size_t size)
 {
 	struct audit_buffer *ab;
 
@@ -77,11 +86,25 @@ static void tty_audit_log(const char *description, struct task_struct *tsk,
 	if (ab) {
 		char name[sizeof(tsk->comm)];
 		uid_t uid = task_uid(tsk);
-
-		audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u "
-				 "major=%d minor=%d comm=", description,
-				 tsk->pid, uid, loginuid, sessionid,
-				 major, minor);
+		u32 sid;
+
+		audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u",
+				 description, tsk->pid, uid, loginuid,
+				 sessionid);
+		security_task_getsecid(tsk, &sid);
+		if (sid) {
+			char *ctx;
+			u32 len;
+
+			if (security_secid_to_secctx(sid, &ctx, &len))
+				audit_log_format(ab, " ssid=%u", sid);
+			else {
+				audit_log_format(ab, " subj=%s", ctx);
+				security_release_secctx(ctx, len);
+			}
+		}
+		audit_log_format(ab, " major=%d minor=%d tty=%s comm=", major,
+				 minor, tty_name);
 		get_task_comm(name, tsk);
 		audit_log_untrustedstring(ab, name);
 		audit_log_format(ab, " data=");
@@ -105,7 +128,7 @@ static void tty_audit_buf_push(struct task_struct *tsk, uid_t loginuid,
 	if (audit_enabled == 0)
 		return;
 	tty_audit_log("tty", tsk, loginuid, sessionid, buf->major, buf->minor,
-		      buf->data, buf->valid);
+		      buf->tty_name, buf->data, buf->valid);
 	buf->valid = 0;
 }
 
@@ -191,7 +214,7 @@ void tty_audit_tiocsti(struct tty_struct *tty, char ch)
 		auid = audit_get_loginuid(current);
 		sessionid = audit_get_sessionid(current);
 		tty_audit_log("ioctl=TIOCSTI", current, auid, sessionid, major,
-			      minor, &ch, 1);
+			      minor, tty->name, &ch, 1);
 	}
 }
 
@@ -240,9 +263,7 @@ static struct tty_audit_buf *tty_audit_buf_get(struct tty_struct *tty)
 	}
 	spin_unlock_irq(&current->sighand->siglock);
 
-	buf2 = tty_audit_buf_alloc(tty->driver->major,
-				   tty->driver->minor_start + tty->index,
-				   tty->icanon);
+	buf2 = tty_audit_buf_alloc(tty);
 	if (buf2 == NULL) {
 		audit_log_lost("out of memory in TTY auditing");
 		return NULL;
@@ -294,9 +315,7 @@ void tty_audit_add_data(struct tty_struct *tty, unsigned char *data,
 	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;
+		tty_audit_buf_setup(buf, tty);
 	}
 	do {
 		size_t run;

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

[PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Miloslav Trmac]

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

From: Miloslav Trmač <mitr@redhat.com>

Add SELinux context information and TTY name (consistent with the
AUDIT_SYSCALL record) to AUDIT_TTY.  An example record after applying
this patch:

 type=TTY msg=audit(1237480806.220:22): tty pid=2601 uid=0 auid=500 ses=1 
 subj=unconfined_u:unconfined_r:unconfined_t:s0 major=136 minor=1 tty=pts1 
 comm="bash" data=6361740D

(line wrapped, new fields are "subj" and "tty".)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 drivers/char/tty_audit.c |   57 ++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 19 deletions(-)

[-- Attachment #2: audit-tty-more-fields.patch --]
[-- Type: application/octet-stream, Size: 4652 bytes --]

From: Miloslav Trmač <mitr@redhat.com>

Add SELinux context information and TTY name (consistent with the
AUDIT_SYSCALL record) to AUDIT_TTY.  An example record after applying
this patch:

 type=TTY msg=audit(1237480806.220:22): tty pid=2601 uid=0 auid=500 ses=1
 subj=unconfined_u:unconfined_r:unconfined_t:s0 major=136 minor=1 tty=pts1
 comm="bash" data=6361740D

(line wrapped, new fields are "subj" and "tty".)

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 drivers/char/tty_audit.c |   57 ++++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tty_audit.c b/drivers/char/tty_audit.c
index 34ab6d7..1b7add4 100644
--- a/drivers/char/tty_audit.c
+++ b/drivers/char/tty_audit.c
@@ -12,6 +12,7 @@
 #include <linux/audit.h>
 #include <linux/file.h>
 #include <linux/fdtable.h>
+#include <linux/security.h>
 #include <linux/tty.h>
 
 struct tty_audit_buf {
@@ -19,12 +20,21 @@ struct tty_audit_buf {
 	struct mutex mutex;	/* Protects all data below */
 	int major, minor;	/* The TTY which the data is from */
 	unsigned icanon:1;
+	char tty_name[sizeof(((struct tty_struct *)NULL)->name)];
 	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)
+static void tty_audit_buf_setup(struct tty_audit_buf *buf,
+				struct tty_struct *tty)
+{
+	buf->major = tty->driver->major;
+	buf->minor = tty->driver->minor_start + tty->index;
+	buf->icanon = tty->icanon;
+	strcpy(buf->tty_name, tty->name);
+}
+
+static struct tty_audit_buf *tty_audit_buf_alloc(struct tty_struct *tty)
 {
 	struct tty_audit_buf *buf;
 
@@ -39,9 +49,7 @@ static struct tty_audit_buf *tty_audit_buf_alloc(int major, int minor,
 		goto err_buf;
 	atomic_set(&buf->count, 1);
 	mutex_init(&buf->mutex);
-	buf->major = major;
-	buf->minor = minor;
-	buf->icanon = icanon;
+	tty_audit_buf_setup(buf, tty);
 	buf->valid = 0;
 	return buf;
 
@@ -69,7 +77,8 @@ static void tty_audit_buf_put(struct tty_audit_buf *buf)
 
 static void tty_audit_log(const char *description, struct task_struct *tsk,
 			  uid_t loginuid, unsigned sessionid, int major,
-			  int minor, unsigned char *data, size_t size)
+			  int minor, const char *tty_name,
+			  unsigned char *data, size_t size)
 {
 	struct audit_buffer *ab;
 
@@ -77,11 +86,25 @@ static void tty_audit_log(const char *description, struct task_struct *tsk,
 	if (ab) {
 		char name[sizeof(tsk->comm)];
 		uid_t uid = task_uid(tsk);
-
-		audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u "
-				 "major=%d minor=%d comm=", description,
-				 tsk->pid, uid, loginuid, sessionid,
-				 major, minor);
+		u32 sid;
+
+		audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u",
+				 description, tsk->pid, uid, loginuid,
+				 sessionid);
+		security_task_getsecid(tsk, &sid);
+		if (sid) {
+			char *ctx;
+			u32 len;
+
+			if (security_secid_to_secctx(sid, &ctx, &len))
+				audit_log_format(ab, " ssid=%u", sid);
+			else {
+				audit_log_format(ab, " subj=%s", ctx);
+				security_release_secctx(ctx, len);
+			}
+		}
+		audit_log_format(ab, " major=%d minor=%d tty=%s comm=", major,
+				 minor, tty_name);
 		get_task_comm(name, tsk);
 		audit_log_untrustedstring(ab, name);
 		audit_log_format(ab, " data=");
@@ -105,7 +128,7 @@ static void tty_audit_buf_push(struct task_struct *tsk, uid_t loginuid,
 	if (audit_enabled == 0)
 		return;
 	tty_audit_log("tty", tsk, loginuid, sessionid, buf->major, buf->minor,
-		      buf->data, buf->valid);
+		      buf->tty_name, buf->data, buf->valid);
 	buf->valid = 0;
 }
 
@@ -191,7 +214,7 @@ void tty_audit_tiocsti(struct tty_struct *tty, char ch)
 		auid = audit_get_loginuid(current);
 		sessionid = audit_get_sessionid(current);
 		tty_audit_log("ioctl=TIOCSTI", current, auid, sessionid, major,
-			      minor, &ch, 1);
+			      minor, tty->name, &ch, 1);
 	}
 }
 
@@ -240,9 +263,7 @@ static struct tty_audit_buf *tty_audit_buf_get(struct tty_struct *tty)
 	}
 	spin_unlock_irq(&current->sighand->siglock);
 
-	buf2 = tty_audit_buf_alloc(tty->driver->major,
-				   tty->driver->minor_start + tty->index,
-				   tty->icanon);
+	buf2 = tty_audit_buf_alloc(tty);
 	if (buf2 == NULL) {
 		audit_log_lost("out of memory in TTY auditing");
 		return NULL;
@@ -294,9 +315,7 @@ void tty_audit_add_data(struct tty_struct *tty, unsigned char *data,
 	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;
+		tty_audit_buf_setup(buf, tty);
 	}
 	do {
 		size_t run;

Re: [PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Paul Moore]

On Thursday 19 March 2009 01:18:00 pm Miloslav Trmac wrote:
> From: Miloslav Trmač <mitr@redhat.com>
>
> Add SELinux context information and TTY name (consistent with the
> AUDIT_SYSCALL record) to AUDIT_TTY.  An example record after applying
> this patch:
>
>  type=TTY msg=audit(1237480806.220:22): tty pid=2601 uid=0 auid=500 ses=1
>  subj=unconfined_u:unconfined_r:unconfined_t:s0 major=136 minor=1 tty=pts1
>  comm="bash" data=6361740D
>
> (line wrapped, new fields are "subj" and "tty".)
>
> Signed-off-by: Miloslav Trmač <mitr@redhat.com>
> ---
>  drivers/char/tty_audit.c |   57 ++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 19 deletions(-)

Just a quick procedural comment, in the future you should include patches in 
the body of the email; people will likely ignore your submission otherwise.

There are several audit experts which should review this code but two things 
jumped out at me when glancing at your patch:

 1. SELinux SIDs should not be recorded
 2. From a SELinux/security point of view ttys are considered objects
    and their labels/contexts should be recorded with "obj=" not "subj="

-- 
paul moore
linux @ hp

Re: [PATCH] Add SELinux context and TTY name to AUDIT_TTY records

[Paul Moore]

On Thursday 19 March 2009 01:18:00 pm Miloslav Trmac wrote:
> From: Miloslav Trmač <mitr@redhat.com>
>
> Add SELinux context information and TTY name (consistent with the
> AUDIT_SYSCALL record) to AUDIT_TTY.  An example record after applying
> this patch:
>
>  type=TTY msg=audit(1237480806.220:22): tty pid=2601 uid=0 auid=500 ses=1
>  subj=unconfined_u:unconfined_r:unconfined_t:s0 major=136 minor=1 tty=pts1
>  comm="bash" data=6361740D
>
> (line wrapped, new fields are "subj" and "tty".)
>
> Signed-off-by: Miloslav Trmač <mitr@redhat.com>
> ---
>  drivers/char/tty_audit.c |   57 ++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 19 deletions(-)

Just a quick procedural comment, in the future you should include patches in 
the body of the email; people will likely ignore your submission otherwise.

There are several audit experts which should review this code but two things 
jumped out at me when glancing at your patch:

 1. SELinux SIDs should not be recorded
 2. From a SELinux/security point of view ttys are considered objects
    and their labels/contexts should be recorded with "obj=" not "subj="

-- 
paul moore
linux @ hp