[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [RFC] linux-2.6.10-auditfs-tc1.patch



* Timothy R. Chavez (chavezt gmail com) wrote:
> rudimentary file system auditing capability using the in-kernel audit
> subsystem.  From userspace, an administrator passes into the kernel a
> pathname and filterkey pair.  The terminating file or directory does
> not have to exist at the time a watch point is created for it, but
> it's parent does.

What is a filterkey?

> TODOs:
> 
> * Add RCU locking in the appropriate places to make SMP-safe

Is RCU a requirement, or just some real locking?

> * Switch over to slab caches for struct audit_data, struct audit_file,
> and struct audit_watch

Yes, please.

> * Think about how I can make it so an administrator can say, "Watch
> this file only on the syscall open where uid=0" -- As of right now, if
> filesystem auditing is on, every syscall (being audited) that accesses
> a watched file or directory collects information about it and
> generates a record.  Is this sufficient?

Seems intuitively useful to at least be able to watch a file regardless
of who touched it, or what syscall was used.  I think you'd especially
want to know if you had some non uid=0 process that wrote to a sensitive
file abusing it's CAP_DAC_OVERRIDE privilege for example.

> * Make it so filesystem auditing can be dynamically enabled/disabled
> from userspace, rather then being "enabled" if configured or
> "disabled" if not configured at boot time.
> * Add/remove unnecessary information about the file or directory being
> collected in the audit context.  Input on this?

I missed which parts are unnecessary?

diff -Npru linux-2.6.10/Makefile linux-2.6.10-audit/Makefile
--- linux-2.6.10/Makefile	2004-12-24 15:35:01.000000000 -0600
+++ linux-2.6.10-audit/Makefile	2005-01-19 11:17:19.000000000 -0600
@@ -1,8 +1,8 @@
 VERSION = 2
 PATCHLEVEL = 6
 SUBLEVEL = 10
-EXTRAVERSION =
-NAME=Woozy Numbat
+EXTRAVERSION =-auditfs-tc1
+NAME=

Probably not really necessary.

--- linux-2.6.10/arch/i386/defconfig    2004-12-24 15:35:01.000000000
-0600
+++ linux-2.6.10-audit/arch/i386/defconfig      2005-01-07
15:44:26.000000000 -0600
@@ -23,6 +23,7 @@ CONFIG_POSIX_MQUEUE=y
 CONFIG_SYSCTL=y
 CONFIG_AUDIT=y
 CONFIG_AUDITSYSCALL=y
+CONFIG_AUDITFILESYSTEM=y

Yes here, but default Kconfig is N?

diff -Npru linux-2.6.10/fs/dcache.c linux-2.6.10-audit/fs/dcache.c
--- linux-2.6.10/fs/dcache.c	2004-12-24 15:34:00.000000000 -0600
+++ linux-2.6.10-audit/fs/dcache.c	2005-01-18 14:20:18.000000000 -0600
@@ -32,6 +32,7 @@
 #include <linux/seqlock.h>
 #include <linux/swap.h>
 #include <linux/bootmem.h>
+#include <linux/audit.h>
 
 /* #define DCACHE_DEBUG 1 */
 
@@ -781,6 +782,7 @@ void d_instantiate(struct dentry *entry,
 	entry->d_inode = inode;
 	spin_unlock(&dcache_lock);
 	security_d_instantiate(entry, inode);
+	audit_watch(entry);
 }
 
 /**
@@ -920,6 +922,7 @@ struct dentry *d_splice_alias(struct ino
 			security_d_instantiate(dentry, inode);
 			d_rehash(dentry);
 		}
+		audit_watch(dentry);
 	} else
 		d_add(dentry, inode);
 	return new;
@@ -1266,6 +1269,7 @@ already_unhashed:
 	spin_unlock(&dentry->d_lock);
 	write_sequnlock(&rename_lock);
 	spin_unlock(&dcache_lock);
+	audit_watch(dentry);

Hmm, I don't think this does what you mentioned.  Here, a rename gets the
new pathname watchpoint, so you could lose the inode watch with a rename.

 }
 
 /**
diff -Npru linux-2.6.10/fs/inode.c linux-2.6.10-audit/fs/inode.c
--- linux-2.6.10/fs/inode.c	2004-12-24 15:35:40.000000000 -0600
+++ linux-2.6.10-audit/fs/inode.c	2005-01-17 17:44:59.000000000 -0600
@@ -135,6 +135,7 @@ static struct inode *alloc_inode(struct 
 		inode->i_cdev = NULL;
 		inode->i_rdev = 0;
 		inode->i_security = NULL;
+		inode->i_audit = NULL;
 		inode->dirtied_when = 0;
 		if (security_inode_alloc(inode)) {
 			if (inode->i_sb->s_op->destroy_inode)
diff -Npru linux-2.6.10/fs/namei.c linux-2.6.10-audit/fs/namei.c
--- linux-2.6.10/fs/namei.c	2004-12-24 15:34:30.000000000 -0600
+++ linux-2.6.10-audit/fs/namei.c	2005-01-18 13:03:18.000000000 -0600
@@ -232,6 +232,10 @@ int permission(struct inode * inode,int 
 	/* Ordinary permission routines do not understand MAY_APPEND. */
 	submask = mask & ~MAY_APPEND;
 
+	retval = audit_notify_watch(current, inode, mask);
+	if (retval < 0)
+		return retval;

I wouldn't expect this to fail.

 	if (inode->i_op && inode->i_op->permission)
 		retval = inode->i_op->permission(inode, submask, nd);
 	else
diff -Npru linux-2.6.10/include/linux/audit.h linux-2.6.10-audit/include/linux/audit.h
--- linux-2.6.10/include/linux/audit.h	2005-01-19 15:18:52.000000000 -0600
+++ linux-2.6.10-audit/include/linux/audit.h	2005-01-19 15:37:56.000000000 -0600
@@ -25,14 +25,16 @@
 #define _LINUX_AUDIT_H_
 
 /* Request and reply types */
-#define AUDIT_GET      1000	/* Get status */
-#define AUDIT_SET      1001	/* Set status (enable/disable/auditd) */
-#define AUDIT_LIST     1002	/* List filtering rules */
-#define AUDIT_ADD      1003	/* Add filtering rule */
-#define AUDIT_DEL      1004	/* Delete filtering rule */
-#define AUDIT_USER     1005	/* Send a message from user-space */
-#define AUDIT_LOGIN    1006     /* Define the login id and informaiton */
-#define AUDIT_KERNEL   2000	/* Asynchronous audit record. NOT A REQUEST. */
+#define AUDIT_GET      	1000	/* Get status */
+#define AUDIT_SET      	1001	/* Set status (enable/disable/auditd) */
+#define AUDIT_LIST     	1002	/* List filtering rules */
+#define AUDIT_ADD      	1003	/* Add filtering rule */
+#define AUDIT_DEL      	1004	/* Delete filtering rule */
+#define AUDIT_USER     	1005	/* Send a message from user-space */
+#define AUDIT_LOGIN    	1006    /* Define the login id and information */
+#define AUDIT_WATCH_INS	1007    /* Insert file/dir watch entry */
+#define AUDIT_WATCH_REM	1008	/* Remove file/dir watch entry */
+#define AUDIT_KERNEL   	2000	/* Asynchronous audit record. NOT A REQUEST. */
 
 /* Rule flags */
 #define AUDIT_PER_TASK 0x01	/* Apply rule at task creation (not syscall) */
@@ -74,7 +76,7 @@
 #define AUDIT_DEVMINOR	101
 #define AUDIT_INODE	102
 #define AUDIT_EXIT	103
-#define AUDIT_SUCCESS   104	/* exit >= 0; value ignored */
+#define AUDIT_SUCCESS	104	/* exit >= 0; value ignored */
 
 #define AUDIT_ARG0      200
 #define AUDIT_ARG1      (AUDIT_ARG0+1)
@@ -129,6 +131,31 @@ struct audit_rule {		/* for AUDIT_LIST, 
 	__u32		values[AUDIT_MAX_FIELDS];
 };
 
+/* Input structure for fs object auditing */
+
+struct audit_watch_request {
+	char	*path;
+	char	*fk;

s/fk/filterkey/?

+	int	pathlen;
+	int	fklen;
+};
+
+/* Watch entry */
+
+/* Following two structs must be commented out for userspace copy */
+
+struct audit_watch {
+	char			*name;
+	char			*fk;
+	struct list_head	list;
+};

Anyway to come up with name distintion between this data and the
function?  And with many of these at 16bytes each on 32bit, it's a large
waste (smallest kmalloc slab is 32bytes iirc)

+struct audit_data {
+	struct audit_watch	*watch;
+	struct list_head	watchlist;
+};

Does audit_watch ever exist without audit_data?  Looking through, it
seems like they can be decoupled, but that's only on rename, where I
think you wouldn't want to change, or drop the whole thing and start
over.

@@ -155,6 +182,8 @@ extern int  audit_receive_filter(int typ
 extern void audit_get_stamp(struct audit_context *ctx,
 			    struct timespec *t, int *serial);
 extern int  audit_set_loginuid(struct audit_context *ctx, uid_t loginuid);
+extern int audit_notify_watch(struct task_struct *task, struct inode *inode,
+                              int mask);
 #else
 #define audit_alloc(t) ({ 0; })
 #define audit_free(t) do { ; } while (0)
@@ -163,8 +192,22 @@ extern int  audit_set_loginuid(struct au
 #define audit_getname(n) do { ; } while (0)
 #define audit_putname(n) do { ; } while (0)
 #define audit_inode(n,i,d) do { ; } while (0)
+#define audit_notify_watch(t,i,m) do { ; } while(0)
 #endif
 
+#ifdef CONFIG_AUDITFILESYSTEM
+extern int audit_receive_watch(int type, int pid, int uid, int seq, 
+			       struct audit_watch_request *req);
+extern int audit_watch(struct dentry *dentry);
+extern void audit_inode_free(struct inode *inode);
+extern struct audit_data *audit_inode_alloc(void);
+#else
+#define audit_receive_watch(t,p,u,s,r) do { ; } while(0)
+#define audit_watch(d) do { ; } while(0)
+#define audit_inode_free(i) do { ; } while(0)
+#define audit_inode_alloc() do { ; } while(0)
+#endif 
+
 #ifdef CONFIG_AUDIT
 /* These are defined in audit.c */
 				/* Public API */
diff -Npru linux-2.6.10/include/linux/fs.h linux-2.6.10-audit/include/linux/fs.h
--- linux-2.6.10/include/linux/fs.h	2004-12-24 15:34:27.000000000 -0600
+++ linux-2.6.10-audit/include/linux/fs.h	2005-01-07 16:17:20.000000000 -0600
@@ -27,6 +27,7 @@ struct poll_table_struct;
 struct kstatfs;
 struct vm_area_struct;
 struct vfsmount;
+struct audit_data;
 
 /*
  * It's silly to have NR_OPEN bigger than NR_FILE, but you can change
@@ -459,6 +460,7 @@ struct inode {
 #ifdef CONFIG_QUOTA
 	struct dquot		*i_dquot[MAXQUOTAS];
 #endif
+	struct audit_data	*i_audit;
 	/* These three should probably be a union */
 	struct list_head	i_devices;
 	struct pipe_inode_info	*i_pipe;
diff -Npru linux-2.6.10/init/Kconfig linux-2.6.10-audit/init/Kconfig
--- linux-2.6.10/init/Kconfig	2004-12-24 15:35:24.000000000 -0600
+++ linux-2.6.10-audit/init/Kconfig	2005-01-16 22:35:08.000000000 -0600
@@ -174,6 +174,17 @@ config AUDITSYSCALL
 	  can be used independently or with another kernel subsystem,
 	  such as SELinux.
 
+config AUDITFILESYSTEM
+	bool "Enable filesystem auditing support"
+	depends on AUDITSYSCALL
+	default n
+	help
+	  Generate audit records for regular files or directories that
+	  are being watched for access by audited syscalls.  To insert
+	  and remove watch points into the filesystem you may use the
+	  auditctl program provided with auditd.  For more information, 
+	  'man auditctl'
+	  
 config LOG_BUF_SHIFT
 	int "Kernel log buffer size (16 => 64KB, 17 => 128KB)" if DEBUG_KERNEL
 	range 12 21
diff -Npru linux-2.6.10/kernel/Makefile linux-2.6.10-audit/kernel/Makefile
--- linux-2.6.10/kernel/Makefile	2004-12-24 15:34:26.000000000 -0600
+++ linux-2.6.10-audit/kernel/Makefile	2005-01-05 15:23:14.000000000 -0600
@@ -23,6 +23,7 @@ obj-$(CONFIG_IKCONFIG_PROC) += configs.o
 obj-$(CONFIG_STOP_MACHINE) += stop_machine.o
 obj-$(CONFIG_AUDIT) += audit.o
 obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
+obj-$(CONFIG_AUDITFILESYSTEM) += auditfs.o
 obj-$(CONFIG_KPROBES) += kprobes.o
 obj-$(CONFIG_SYSFS) += ksysfs.o
 obj-$(CONFIG_GENERIC_HARDIRQS) += irq/
diff -Npru linux-2.6.10/kernel/audit.c linux-2.6.10-audit/kernel/audit.c
--- linux-2.6.10/kernel/audit.c	2004-12-24 15:35:50.000000000 -0600
+++ linux-2.6.10-audit/kernel/audit.c	2005-01-19 11:30:02.000000000 -0600
@@ -20,6 +20,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  *
  * Written by Rickard E. (Rik) Faith <faith redhat com>
+ * Modified by Timothy R. Chavez <chavezt us ibm com>
  *
  * Goals: 1) Integrate fully with SELinux.
  *	  2) Minimal run-time overhead:
@@ -46,7 +47,6 @@
 #include <asm/types.h>
 #include <linux/mm.h>
 #include <linux/module.h>
-
 #include <linux/audit.h>
 
 #include <net/sock.h>
@@ -60,6 +60,9 @@ static int	audit_initialized;
 /* No syscall auditing will take place unless audit_enabled != 0. */
 int		audit_enabled;
 
+/* No filesystem auditing will take place unless audit_filesystem != 0 */
+int		audit_filesystem;

Err, this is only set based on config.  So it doesn't really do
anything.

+
 /* Default state when kernel boots without any parameters. */
 static int	audit_default;
 
@@ -394,6 +397,15 @@ static int audit_receive_msg(struct sk_b
 		err = -EOPNOTSUPP;
 #endif
 		break;
+#ifdef CONFIG_AUDITFILESYSTEM
+	case AUDIT_WATCH_INS:
+	case AUDIT_WATCH_REM:
+		err = audit_receive_watch(nlh->nlmsg_type, pid, uid, seq,
+					   data);
+#else
+		err = -EOPNOTSUPP;
+#endif
+		break;
 	default:
 		err = -EINVAL;
 		break;
@@ -533,6 +545,16 @@ int __init audit_init(void)
 
 	audit_initialized = 1;
 	audit_enabled = audit_default;
+/* FIXME:
+ * We should be able to enable/disable filesystem auditing dynamically,
+ * after we boot up.  Configuring support for it should != automatic
+ * enablement.
+ */
+#ifdef CONFIG_AUDITFILESYSTEM
+	audit_filesystem = 1;
+#else
+	audit_filesystem = 0;
+#endif

Oh, nm, I see the fixme comment ;-)

 	audit_log(NULL, "initialized");
 	return 0;
 }
diff -Npru linux-2.6.10/kernel/auditfs.c linux-2.6.10-audit/kernel/auditfs.c
--- linux-2.6.10/kernel/auditfs.c	1969-12-31 17:00:00.000000000 -0700
+++ linux-2.6.10-audit/kernel/auditfs.c	2005-01-19 14:48:08.000000000 -0600
@@ -0,0 +1,396 @@
+/* auditfs.c -- Filesystem auditing support -*- linux-c -*-
+ * Implements filesystem auditing support, depends on kernel/auditsc.c
+ *
+ * Copyright 2005 International Business Machines Corp. (IBM)
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ * Written by Timothy R. Chavez <chavezt us ibm com>
+ *
+ */
+
+#include <linux/init.h>
+
+#include <linux/fs.h>
+#include <linux/sched.h>
+#include <linux/kernel.h>
+#include <linux/namei.h>
+#include <linux/mount.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/audit.h>
+
+#include <asm/types.h>
+#include <asm/uaccess.h>
+
+/* Helper functions */
+
+struct audit_lookup {
+	char *name;
+	struct dentry *dentry;
+	struct dentry *parent;
+};
+
+/* audit_path_lookup 	- Look up a string path and store the parent dentry,
+ *		          string name of the file/dir we're interested in,
+ *		          and optionally, the dentry of the file/dir we're 
+ *		          interested in if it exists
+ *
+ * @path		- String path (ie: "/etc/passwd")
+ * @lup			- Lookup structure

These aren't proper docbook style comments.

+ *
+ * NOTE: caller must dput dentries if necessary
+ */
+static int audit_path_lookup(const char *path, struct audit_lookup *lup)
+{
+	int ret;
+	struct nameidata nd;
+
+	ret = path_lookup(path, LOOKUP_PARENT | LOOKUP_FOLLOW, &nd);
+	if (ret < 0)
+		goto audit_path_lookup_exit;
+
+	lup->parent = dget(nd.dentry);
+	lup->dentry = d_lookup(nd.dentry, &nd.last);
+	lup->name = kmalloc(strlen(nd.last.name) + 1, GFP_KERNEL);
+	if (!lup->name)
+		return -ENOMEM;

Never called path_release()

+	strcpy(lup->name, nd.last.name);
+
+	path_release(&nd);
+
+      audit_path_lookup_exit:
+	return ret;
+}
+
+/* audit_fetch_watch		- If the string name matches that of
+ *				  of a watch entry in the parent dentry
+ *				  then return that watch structure.
+ *
+ * @name			- String name (ie: "passwd") 
+ * @parent			- The dentry of the inode whose watchlist
+ 				  we want to traverse
+ */

DocBook comments here too, I think they all need to be redone.

+static struct audit_watch *audit_fetch_watch(const char *name,
+					     struct dentry *parent)
+{
+	struct audit_data *data;
+	struct audit_watch *watch, *ret = NULL;
+
+	data = parent->d_inode->i_audit;
+	if (!data)
+		goto audit_fetch_watch_exit;
+
+	if (list_empty(&data->watchlist))
+		goto audit_fetch_watch_exit;

list_for_each will tell you if it's empty.

+	list_for_each_entry(watch, &data->watchlist, list)
+	    if (!strcmp(watch->name, name)) {
+		ret = watch;
+		goto audit_fetch_watch_exit;

usually just break;

+	}

Have a feel for how many are usually in a list?  for /etc, for example,
maybe a hash is better?

+      audit_fetch_watch_exit:
+	return ret;
+}
+
+static struct audit_watch *audit_watch_alloc(void)
+{
+	struct audit_watch *watch;
+
+	watch = kmalloc(sizeof(struct audit_watch), GFP_KERNEL);
+	if (!watch)
+		return NULL;
+
+	watch->name = NULL;
+	watch->fk = NULL;
+

I'd do:
 if (watch) {
	watch->name = NULL;
	watch->fk = NULL;
 }
+	return watch;

To get a single point of return.  Most places that are leaking would
benefit from this type of coding style.

+}
+
+static void audit_watch_free(struct audit_watch *watch)
+{
+	if (watch) {
+		if (watch->name)
+			kfree(watch->name);
+		if (watch->fk)
+			kfree(watch->fk);

kfree(NULL) is fine.

+		kfree(watch);
+	}


+}
+
+/* audit_create_watch		- Add a watch entry to the dentry's parent's
+ *				  watchlist with the given name and
+ *				  fk.
+ *
+ * @name			- String name (ie: "passwd")
+ * @fk				- String fk (ie: "fk_file_passwd")
+ * @parent			- The dentry of the inode with relevant
+ *				  watchlist
+ */
+
+static int
+audit_create_watch(const char *name, const char *fk, struct dentry *parent)
+{
+	struct audit_watch *watch;
+
+	/* FIXME: More appropriate errno for something that already exists? */
+	watch = audit_fetch_watch(name, parent);
+	if (watch)
+		return -EINVAL;

EEXIST?  EBUSY?

+
+	if (!parent->d_inode->i_audit) {
+		parent->d_inode->i_audit = audit_inode_alloc();
+		if (!parent->d_inode->i_audit)
+			return -ENOMEM;
+	}
+
+	watch = audit_watch_alloc();
+	if (!watch)
+		return -ENOMEM;

Possible memory leak.

+	watch->name = kmalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!watch->name)
+		return -ENOMEM;
+	watch->fk = kmalloc(strlen(fk) + 1, GFP_KERNEL);
+	if (!watch->fk)
+		return -ENOMEM;

More possible memory leaks.

+	strcpy(watch->name, name);
+	strcpy(watch->fk, fk);
+
+	list_add(&watch->list, &parent->d_inode->i_audit->watchlist);

We won't go into the locking ;-)

+	return 0;
+}
+
+/* audit_destroy_watch		- Find the watch entry and then physically
+ *				  destroy it.
+ *
+ * @name			- String name (ie: "passwd")
+ * @parent			- The dentry of the inode with relevant
+ *				  watchlist
+ */
+
+static int audit_destroy_watch(const char *name, struct dentry *parent)
+{
+	struct audit_watch *watch;
+
+	/* FIXME: More appropriate errno for something that does not exist? */
+	watch = audit_fetch_watch(name, parent);
+	if (!watch)
+		return -EINVAL;

-EEXIST?

+
+	list_del_init(&watch->list);
+	audit_watch_free(watch);
+
+	if (!parent->d_inode->i_audit->watch &&
+	    list_empty(&parent->d_inode->i_audit->watchlist))
+		audit_inode_free(parent->d_inode);
+
+	return 0;
+}
+
+/* audit_insert_watch		- Insert a watch entry for req->name
+ * 				  with req->fk into the filesystem.
+ *
+ * @req			- The request originating from userspace.
+ *
+ * (Called by audit_receive_watch())
+ */
+
+static int audit_insert_watch(struct audit_watch_request *req)
+{
+	int ret;
+	char *fk, *path;
+	struct audit_lookup lup;

Why can't you just use nameidata?

+	path = kmalloc(req->pathlen, GFP_KERNEL);
+	if (!path)
+		return -ENOMEM;
+	fk = kmalloc(req->fklen, GFP_KERNEL);
+	if (!fk)
+		return -ENOMEM;

Possible memleak.

+	ret = strncpy_from_user(path, req->path, req->pathlen);
+	if (ret < 0)
+		return ret;
+	ret = strncpy_from_user(fk, req->fk, req->fklen);
+	if (ret < 0)
+		return ret;

req->pathlen is user specified.  it can't overflow path, but should
these be checked against valid values, say PATH_MAX?

+	ret = audit_path_lookup(path, &lup);
+	if (ret < 0)
+		goto audit_insert_watch_exit;
+
+	ret = audit_create_watch(lup.name, fk, lup.parent);
+	if (ret < 0)
+		goto audit_insert_watch_exit;
+
+	ret = audit_watch(lup.dentry);
+
+      audit_insert_watch_exit:

CodingStyle, no tab before label.

+	if (lup.parent)
+		dput(lup.parent);
+	if (lup.dentry)
+		dput(lup.dentry);
+	return ret;

Never kfree path, fk, or lup.name.

+}
+
+/* audit_remove_watch 		- Remove a watch entry for req->name
+ * 				  from the filesystem.
+ *
+ * @req 			- The request originating from userspace.
+ *
+ * (Called by audit_watch_receive())
+ */
+
+static int audit_remove_watch(struct audit_watch_request *req)
+{
+	int ret;
+	char *path;
+	struct audit_lookup lup;

Why not use nameidata?

+	path = kmalloc(req->pathlen, GFP_KERNEL);
+	if (!path)
+		return -ENOMEM;

getname()?

+	ret = strncpy_from_user(path, req->path, req->pathlen);
+	if (ret < 0)
+		goto audit_remove_watch_exit;
+
+	ret = audit_path_lookup(req->path, &lup);

That's the userspace pointer, should pass path.

+	if (ret < 0)
+		goto audit_remove_watch_exit;
+
+	ret = audit_destroy_watch(lup.name, lup.parent);
+	if (ret < 0)
+		goto audit_remove_watch_exit;
+
+	if (!lup.dentry)
+		goto audit_remove_watch_exit;
+
+	/* Free up inode audit data, if possible */
+	lup.dentry->d_inode->i_audit->watch = NULL;

audit_inode_free() does this too.  is this safe?  in audit_inode_free
you check against i_audit == NULL.

+	if (list_empty(&lup.dentry->d_inode->i_audit->watchlist))
+		audit_inode_free(lup.dentry->d_inode);
+
+      audit_remove_watch_exit:

CodingStyle, no tab before label.

+	if (lup.parent)
+		dput(lup.parent);
+	if (lup.dentry)
+		dput(lup.dentry);
+	return ret;

You never kfree path or lup.name.

+}
+
+/* Public interface */
+
+struct audit_data *audit_inode_alloc(void)
+{
+	struct audit_data *data;
+
+	data = kmalloc(sizeof(struct audit_data), GFP_KERNEL);
+	if (!data)
+		return NULL;
+
+	data->watch = NULL;
+	INIT_LIST_HEAD(&data->watchlist);
+
+	return data;
+}
+
+void audit_inode_free(struct inode *inode)
+{
+	struct audit_watch *watch, *tmp;
+
+	if (!inode->i_audit)
+		return;
+
+	inode->i_audit->watch = NULL;
+	list_for_each_entry_safe(watch, tmp, &inode->i_audit->watchlist,
+				 list) {
+		list_del(&watch->list);
+		audit_watch_free(watch);
+	}
+
+	kfree(inode->i_audit);
+	inode->i_audit = NULL;
+}
+
+/* If child exists in parent's watchlist, the child will point
+ * into the parent's watchlist at its own entry.  Otherwise,
+ * it will point to NULL
+ *
+ * Hook appears in: fs/dcache.c:d_instantiate(), d_move(), and d_splice_alias()
+ */
+int audit_watch(struct dentry *dentry)
+{
+	struct audit_watch *watch;
+
+	/* NOTE: Not being watched != error */
+	if (!dentry || !dentry->d_inode)
+		return 0;
+
+	watch = audit_fetch_watch(dentry->d_name.name, dentry->d_parent);
+	if (!watch)
+		return 0;
+
+	/* In the event that we're being watched, but not yet allocated. */
+	if (!dentry->d_inode->i_audit) {
+		dentry->d_inode->i_audit = audit_inode_alloc();
+		if (!dentry->d_inode->i_audit)
+			return -ENOMEM;
+	}
+	dentry->d_inode->i_audit->watch = watch;
+
+	return 0;
+}
+
+/* Called from kernel/audit.c by audit_receive_msg()
+ * 
+ * Code path for watch insertions:
+ *  audit_receive_watch()->audit_insert_watch()->audit_create_watch()
+ * Code path for watch removals:
+ *  audit_receive_watch()->audit_remove_watch()->audit_destroy_watch()
+ *
+ */
+int
+audit_receive_watch(int type, int pid, int uid, int seq,
+		    struct audit_watch_request *req)
+{
+	int ret;
+
+	switch (type) {
+	case AUDIT_WATCH_INS:
+		ret = audit_insert_watch(req);
+		break;
+	case AUDIT_WATCH_REM:
+		ret = audit_remove_watch(req);
+		break;
+	default:
+		return -EINVAL;

do you want to reply to userspace here or below on error?

+	}
+
+	/* Need to return something more meaningful to userspace and
+	 * update comment that states audit_send_reply is only called
+	 * from auditsc.c
+	 */
+	if (!ret)
+		audit_send_reply(pid, seq, type, 0, 1, NULL, 0);
+
+	return ret;
+}
diff -Npru linux-2.6.10/kernel/auditsc.c linux-2.6.10-audit/kernel/auditsc.c
--- linux-2.6.10/kernel/auditsc.c	2004-12-24 15:35:24.000000000 -0600
+++ linux-2.6.10-audit/kernel/auditsc.c	2005-01-19 15:39:22.000000000 -0600
@@ -19,6 +19,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  *
  * Written by Rickard E. (Rik) Faith <faith redhat com>
+ * Modified by Timothy R. Chavez <chavezt us ibm com>
  *
  * Many of the ideas implemented here are from Stephen C. Tweedie,
  * especially the idea of avoiding a copy by using getname.
@@ -49,6 +50,11 @@
 /* No syscall auditing will take place unless audit_enabled != 0. */
 extern int audit_enabled;
 
+/* No syscall will collect information about auditable file/dirs
+ * unless audit_filesystem != 0
+ */
+extern int audit_filesystem;
+
 /* AUDIT_NAMES is the number of slots we reserve in the audit_context
  * for saving names from getname(). */
 #define AUDIT_NAMES    20
@@ -113,6 +119,7 @@ struct audit_context {
 	uid_t		    uid, euid, suid, fsuid;
 	gid_t		    gid, egid, sgid, fsgid;
 	unsigned long	    personality;
+	struct list_head    watched; /* The auditable files/dirs accessed */
 
 #if AUDIT_DEBUG
 	int		    put_count;
@@ -134,6 +141,22 @@ struct audit_entry {
 	struct audit_rule rule;
 };
 
+/* The structure that stores information about files/directories being
+ * watched in the filesystem, that the syscall accessed.
+ */
+
+struct audit_file {
+        char *name;
+        char *fk;
+        unsigned long ino;
+        umode_t mode;
+        uid_t uid;
+        gid_t gid;
+        dev_t rdev;
+        int mask;
+        struct list_head list;
+};
+
 /* Check to see if two rules are identical.  It is called from
  * audit_del_rule during AUDIT_DEL. */
 static int audit_compare_rule(struct audit_rule *a, struct audit_rule *b)
@@ -506,6 +529,44 @@ static inline void audit_free_names(stru
 	context->name_count = 0;
 }
 
+static struct audit_file *audit_file_alloc(void)
+{
+	struct audit_file *file;
+	
+	file = kmalloc(sizeof(struct audit_file), GFP_KERNEL);
+	if (!file)
+		return NULL;
+
+	file->name = NULL;
+	file->fk = NULL;
+         
+	return file;

single point of return.

+}
+
+static void audit_file_free(struct audit_file *file)
+{
+	if (file) {
+		if (file->name)
+			kfree(file->name);
+		if (file->fk)
+			kfree(file->fk);

kfree(NULL) ok.

+		kfree(file);
+	}
+}
+
+static inline void audit_free_files(struct audit_context *context)
+{
+	struct audit_file *file, *tmp;
+	
+	if (!audit_filesystem)
+		return;

I don't think that's right.  If it was dynamically turned off (after
fixme is fixed) then this could leak some already created audit_files,
right?

+		
+	list_for_each_entry_safe(file, tmp, &context->watched, list) {
+	        list_del(&file->list);
+		audit_file_free(file);
+	}
+}
+
 static inline void audit_zero_context(struct audit_context *context,
 				      enum audit_state state)
 {
@@ -514,6 +575,7 @@ static inline void audit_zero_context(st
 	memset(context, 0, sizeof(*context));
 	context->state      = state;
 	context->loginuid   = loginuid;
+	INIT_LIST_HEAD(&context->watched);
 }
 
 static inline struct audit_context *audit_alloc_context(enum audit_state state)
@@ -572,6 +634,7 @@ static inline void audit_free_context(st
 			       context->name_count, count);
 		}
 		audit_free_names(context);
+		audit_free_files(context);
 		kfree(context);
 		context  = previous;
 	} while (context);
@@ -582,6 +645,7 @@ static inline void audit_free_context(st
 static void audit_log_exit(struct audit_context *context)
 {
 	int i;
+	struct audit_file *file;
 	struct audit_buffer *ab;
 
 	ab = audit_log_start(context);
@@ -628,6 +692,24 @@ static void audit_log_exit(struct audit_
 					 MINOR(context->names[i].rdev));
 		audit_log_end(ab);
 	}
+
+	if (!audit_filesystem)
+		return;
+	list_for_each_entry(file, &context->watched, list) {
+		ab = audit_log_start(context);
+		if (!ab)
+			continue;
+		
+		/* Do we need more information? */
+		audit_log_format(ab,
+			"name=%s filter_key=%s inode=%lu inode_mode=%d "
+			"inode_uid=%d inode_gid=%d inode_dev=%02x:%02x "
+			"mask=%d",
+			file->name, file->fk, file->ino, file->mode, 
+			file->uid, file->gid, MAJOR(file->rdev), 
+			MINOR(file->rdev), file->mask);
+		audit_log_end(ab);
+	}
 }
 
 /* Free a per-task audit context.  Called from copy_process and
@@ -791,6 +873,7 @@ void audit_syscall_exit(struct task_stru
 		tsk->audit_context = new_context;
 	} else {
 		audit_free_names(context);
+		audit_free_files(context);
 		audit_zero_context(context, context->state);
 		tsk->audit_context = context;
 	}
@@ -914,3 +997,58 @@ int audit_set_loginuid(struct audit_cont
 	}
 	return 0;
 }
+
+/* If file/dir has an audit_context and has filesystem auditing
+ * turned on, then if this inode is being watched, collect info
+ * about it.
+ *
+ * Hook located in: fs/namie.c:permission()
+ */
+
+int audit_notify_watch(struct task_struct *task, struct inode *inode,
+		       int mask)
+{
+	struct audit_context *context;
+	struct audit_file *file;
+	
+	context = current->audit_context;

Technically, this is a bug.  You pass the task, but use current.
Luckily, this only appears to be called in current context.

+	/* Do we have a context and are we in a syscall? */
+	if (!context || !context->in_syscall)
+		return 0;
+  
+	/* Do we care about file system auditing? */
+	if (!audit_filesystem)
+		return 0;
+	
+	/* Are we being watched? */
+	if (!inode->i_audit || !inode->i_audit->watch)
+		return 0;
+
+	file = audit_file_alloc();
+	if (!file)
+		return -ENOMEM;
+	
+	file->name = kmalloc(strlen(inode->i_audit->watch->name) + 1,
+			     GFP_KERNEL);
+	if (!file->name)
+		return -ENOMEM;
+
+	file->fk = kmalloc(strlen(inode->i_audit->watch->fk) + 1, 
+			   GFP_KERNEL);
+	if (!file->fk)
+		return -ENOMEM;

Anyway to avoid these extra allocations (which leak on failure paths ;-)?
Like refcount the watch struct and pass that along?  Ditto for the inode
data.

+	strcpy(file->name, inode->i_audit->watch->name);
+	strcpy(file->fk, inode->i_audit->watch->fk);
+	file->ino = inode->i_ino;
+	file->uid = inode->i_uid;
+	file->gid = inode->i_gid;
+	file->rdev = inode->i_rdev;
+	file->mask = mask;
+        
+	list_add(&file->list, &task->audit_context->watched);
+
+	return 0;
+}
+


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]