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

Re: [PATCH 2/2] filesystem auditing: augment audit_inode



Not too many comments on my first cursory glance.  I'll go ahead and incorporate this patch into my patch set.

-tim

On Wednesday 19 October 2005 16:11, Amy Griffis wrote:
> Collect more inode information during syscall processing.
> 
> NOTE: This patch makes some changes to the output of AUDIT_PATH
> records.  In the case of the name field, the record will show
> "name=(null)" if there is no name field (e.g. in an fchown call).  I
> did this because it seemed it would make more sense to someone looking
> at the records.
> 
> I also added a "parent" field to distinguish between the inode number
> and the parent inode number.  This allowed me to remove the "flags"
> field.  In some cases, such as syscall failures, inode information may
> not be present in the audit context.  I took the liberty to not emit
> fields with undefined values.  I don't know if this is the right
> approach.  I think the real solution is to move to a binary record
> format and leave this decision for a userspace tool.

I definately agree with you with regards to the binary record.

> 
> 
> diff --git a/fs/namei.c b/fs/namei.c
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1046,8 +1046,7 @@ int fastcall path_lookup(const char *nam
>  	current->total_link_count = 0;
>  	retval = link_path_walk(name, nd);
>  out:
> -	if (unlikely(current->audit_context
> -		     && nd && nd->dentry && nd->dentry->d_inode))
> +	if (nd && nd->dentry && nd->dentry->d_inode)
>  		audit_inode(name, nd->dentry->d_inode, flags);
>  	return retval;
>  }
> @@ -1192,6 +1191,7 @@ static inline int may_delete(struct inod
>  		return -ENOENT;
>  
>  	BUG_ON(victim->d_parent->d_inode != dir);
> +	audit_inode_child(victim->d_name.name, victim->d_inode, dir->i_ino);
>  
>  	error = permission(dir,MAY_WRITE | MAY_EXEC, NULL);
>  	if (error)
> diff --git a/fs/open.c b/fs/open.c
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -25,6 +25,7 @@
>  #include <linux/pagemap.h>
>  #include <linux/syscalls.h>
>  #include <linux/rcupdate.h>
> +#include <linux/audit.h>
>  
>  #include <asm/unistd.h>
>  
> @@ -609,6 +610,8 @@ asmlinkage long sys_fchmod(unsigned int 
>  	dentry = file->f_dentry;
>  	inode = dentry->d_inode;
>  
> +	audit_inode(NULL, inode, 0);
> +
>  	err = -EROFS;
>  	if (IS_RDONLY(inode))
>  		goto out_putf;
> @@ -732,7 +735,10 @@ asmlinkage long sys_fchown(unsigned int 
>  
>  	file = fget(fd);
>  	if (file) {
> -		error = chown_common(file->f_dentry, user, group);
> +		struct dentry * dentry;

I guess for consistency's sake this should be declared at the top?

> +		dentry = file->f_dentry;
> +		audit_inode(NULL, dentry->d_inode, 0);
> +		error = chown_common(dentry, user, group);
>  		fput(file);
>  	}
>  	return error;
> diff --git a/fs/xattr.c b/fs/xattr.c
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -17,6 +17,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/module.h>
>  #include <linux/fsnotify.h>
> +#include <linux/audit.h>
>  #include <asm/uaccess.h>
>  
>  /*
> @@ -114,12 +115,15 @@ sys_fsetxattr(int fd, char __user *name,
>  	      size_t size, int flags)
>  {
>  	struct file *f;
> +	struct dentry *dentry;
>  	int error = -EBADF;
>  
>  	f = fget(fd);
>  	if (!f)
>  		return error;
> -	error = setxattr(f->f_dentry, name, value, size, flags);
> +	dentry = f->f_dentry;
> +	audit_inode(NULL, dentry->d_inode, 0);
> +	error = setxattr(dentry, name, value, size, flags);
>  	fput(f);
>  	return error;
>  }
> @@ -364,12 +368,15 @@ asmlinkage long
>  sys_fremovexattr(int fd, char __user *name)
>  {
>  	struct file *f;
> +	struct dentry *dentry;
>  	int error = -EBADF;
>  
>  	f = fget(fd);
>  	if (!f)
>  		return error;
> -	error = removexattr(f->f_dentry, name);
> +	dentry = f->f_dentry;
> +	audit_inode(NULL, dentry->d_inode, 0);
> +	error = removexattr(dentry, name);
>  	fput(f);
>  	return error;
>  }
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -222,7 +222,20 @@ extern void audit_syscall_entry(struct t
>  extern void audit_syscall_exit(struct task_struct *task, int failed, long return_code);
>  extern void audit_getname(const char *name);
>  extern void audit_putname(const char *name);
> -extern void audit_inode(const char *name, const struct inode *inode, unsigned flags);
> +extern void __audit_inode(const char *name, const struct inode *inode, unsigned flags);
> +extern void __audit_inode_child(const char *dname, const struct inode *inode,
> +				unsigned long pino);
> +static inline void audit_inode(const char *name, const struct inode *inode,
> +			       unsigned flags) {
> +	if (unlikely(current->audit_context))
> +		__audit_inode(name, inode, flags);
> +}
> +static inline void audit_inode_child(const char *dname, 
> +				     const struct inode *inode, 
> +				     unsigned long pino) {
> +	if (unlikely(current->audit_context))
> +		__audit_inode_child(dname, inode, pino);
> +}
>  
>  				/* Private API (for audit.c only) */
>  extern int  audit_receive_filter(int type, int pid, int uid, int seq,
> @@ -245,7 +258,10 @@ extern int audit_filter_user(struct netl
>  #define audit_syscall_exit(t,f,r) do { ; } while (0)
>  #define audit_getname(n) do { ; } while (0)
>  #define audit_putname(n) do { ; } while (0)
> +#define __audit_inode(n,i,f) do { ; } while (0)
> +#define __audit_inode_child(d,i,p) do { ; } while (0)
>  #define audit_inode(n,i,f) do { ; } while (0)
> +#define audit_inode_child(d,i,p) do { ; } while (0)
>  #define audit_receive_filter(t,p,u,s,d,l) ({ -EOPNOTSUPP; })
>  #define auditsc_get_stamp(c,t,s) do { BUG(); } while (0)
>  #define audit_get_loginuid(c) ({ -1; })
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -15,6 +15,7 @@
>  
>  #include <linux/dnotify.h>
>  #include <linux/inotify.h>
> +#include <linux/audit.h>
>  
>  /*
>   * fsnotify_move - file old_name at old_dir was moved to new_name at new_dir
> @@ -45,6 +46,8 @@ static inline void fsnotify_move(struct 
>  	if (source) {
>  		inotify_inode_queue_event(source, IN_MOVE_SELF, 0, NULL);
>  	}
> +	audit_inode_child(old_name, source, old_dir->i_ino);
> +	audit_inode_child(new_name, target, new_dir->i_ino);
>  }
>  
>  /*
> @@ -74,6 +77,7 @@ static inline void fsnotify_create(struc
>  {
>  	inode_dir_notify(inode, DN_CREATE);
>  	inotify_inode_queue_event(inode, IN_CREATE, 0, dentry->d_name.name);
> +	audit_inode_child(dentry->d_name.name, dentry->d_inode, inode->i_ino);
>  }
>  
>  /*
> @@ -84,6 +88,7 @@ static inline void fsnotify_mkdir(struct
>  	inode_dir_notify(inode, DN_CREATE);
>  	inotify_inode_queue_event(inode, IN_CREATE | IN_ISDIR, 0, 
>  				  dentry->d_name.name);
> +	audit_inode_child(dentry->d_name.name, dentry->d_inode, inode->i_ino);
>  }
>  
>  /*
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2,6 +2,7 @@
>   * Handles all system-call specific auditing features.
>   *
>   * Copyright 2003-2004 Red Hat Inc., Durham, North Carolina.
> + * Copyright 2005 Hewlett-Packard Development Company, L.P.
>   * All Rights Reserved.
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -27,11 +28,15 @@
>   * this file -- see entry.S) is based on a GPL'd patch written by
>   * okir suse de and Copyright 2003 SuSE Linux AG.
>   *
> + * Modified by Amy Griffis <amy griffis hp com> to collect additional
> + * filesystem information.
>   */
>  
>  #include <linux/init.h>
>  #include <asm/atomic.h>
>  #include <asm/types.h>
> +#include <linux/fs.h>
> +#include <linux/namei.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/mount.h>
> @@ -93,12 +98,12 @@ enum audit_state {
>  struct audit_names {
>  	const char	*name;
>  	unsigned long	ino;
> +	unsigned long	pino;
>  	dev_t		dev;
>  	umode_t		mode;
>  	uid_t		uid;
>  	gid_t		gid;
>  	dev_t		rdev;
> -	unsigned	flags;
>  };
>  
>  struct audit_aux_data {
> @@ -479,7 +484,9 @@ static int audit_filter_rules(struct tas
>  		case AUDIT_INODE:
>  			if (ctx) {
>  				for (j = 0; j < ctx->name_count; j++) {
> -					if (ctx->names[j].ino == value) {
> +					if ((ctx->names[j].ino == value) ||
> +					    (ctx->names[j].pino == value))
> +					{
>  						++result;
>  						break;
>  					}
> @@ -663,17 +670,17 @@ static inline void audit_free_names(stru
>  #if AUDIT_DEBUG == 2
>  	if (context->auditable
>  	    ||context->put_count + context->ino_count != context->name_count) {
> -		printk(KERN_ERR "audit.c:%d(:%d): major=%d in_syscall=%d"
> +		printk(KERN_ERR "%s:%d(:%d): major=%d in_syscall=%d"
>  		       " name_count=%d put_count=%d"
>  		       " ino_count=%d [NOT freeing]\n",
> -		       __LINE__,
> +		       __FILE__, __LINE__,
>  		       context->serial, context->major, context->in_syscall,
>  		       context->name_count, context->put_count,
>  		       context->ino_count);
>  		for (i = 0; i < context->name_count; i++)
>  			printk(KERN_ERR "names[%d] = %p = %s\n", i,
>  			       context->names[i].name,
> -			       context->names[i].name);
> +			       context->names[i].name ?: "(null)");
>  		dump_stack();
>  		return;
>  	}
> @@ -899,27 +906,34 @@ static void audit_log_exit(struct audit_
>  		}
>  	}
>  	for (i = 0; i < context->name_count; i++) {
> +		unsigned long ino  = context->names[i].ino;
> +		unsigned long pino = context->names[i].pino;
> +
>  		ab = audit_log_start(context, GFP_KERNEL, AUDIT_PATH);
>  		if (!ab)
>  			continue; /* audit_panic has been called */
>  
>  		audit_log_format(ab, "item=%d", i);
> -		if (context->names[i].name) {
> -			audit_log_format(ab, " name=");
> +
> +		audit_log_format(ab, " name=");
> +		if (context->names[i].name)
>  			audit_log_untrustedstring(ab, context->names[i].name);
> -		}
> -		audit_log_format(ab, " flags=%x\n", context->names[i].flags);
> -			 
> -		if (context->names[i].ino != (unsigned long)-1)
> -			audit_log_format(ab, " inode=%lu dev=%02x:%02x mode=%#o"
> -					     " ouid=%u ogid=%u rdev=%02x:%02x",
> -					 context->names[i].ino,
> -					 MAJOR(context->names[i].dev),
> -					 MINOR(context->names[i].dev),
> -					 context->names[i].mode,
> -					 context->names[i].uid,
> -					 context->names[i].gid,
> -					 MAJOR(context->names[i].rdev),
> +		else
> +			audit_log_format(ab, "(null)");
> +
> +		if (pino != (unsigned long)-1)
> +			audit_log_format(ab, " parent=%lu",  pino);
> +		if (ino != (unsigned long)-1)
> +			audit_log_format(ab, " inode=%lu",  ino);
> +		if ((pino != (unsigned long)-1) || (ino != (unsigned long)-1))
> +			audit_log_format(ab, " dev=%02x:%02x mode=%#o" 
> +					 " ouid=%u ogid=%u rdev=%02x:%02x", 
> +					 MAJOR(context->names[i].dev), 
> +					 MINOR(context->names[i].dev), 
> +					 context->names[i].mode, 
> +					 context->names[i].uid, 
> +					 context->names[i].gid, 
> +					 MAJOR(context->names[i].rdev), 
>  					 MINOR(context->names[i].rdev));
>  		audit_log_end(ab);
>  	}
> @@ -1146,7 +1160,7 @@ void audit_putname(const char *name)
>  			for (i = 0; i < context->name_count; i++)
>  				printk(KERN_ERR "name[%d] = %p = %s\n", i,
>  				       context->names[i].name,
> -				       context->names[i].name);
> +				       context->names[i].name ?: "(null)");
>  		}
>  #endif
>  		__putname(name);
> @@ -1174,9 +1188,10 @@ void audit_putname(const char *name)
>   * @inode: inode being audited
>   * @flags: lookup flags (as used in path_lookup())
>   *
> - * Called from fs/namei.c:path_lookup().
> + * Hooking path_lookup() catches most cases.  Syscalls operating on
> + * file descriptors must be separately hooked.
>   */
> -void audit_inode(const char *name, const struct inode *inode, unsigned flags)
> +void __audit_inode(const char *name, const struct inode *inode, unsigned flags)
>  {
>  	int idx;
>  	struct audit_context *context = current->audit_context;
> @@ -1202,13 +1217,93 @@ void audit_inode(const char *name, const
>  		++context->ino_count;
>  #endif
>  	}
> -	context->names[idx].flags = flags;
> -	context->names[idx].ino   = inode->i_ino;
>  	context->names[idx].dev	  = inode->i_sb->s_dev;
>  	context->names[idx].mode  = inode->i_mode;
>  	context->names[idx].uid   = inode->i_uid;
>  	context->names[idx].gid   = inode->i_gid;
>  	context->names[idx].rdev  = inode->i_rdev;
> +	if ((flags & LOOKUP_PARENT) && (strcmp(name, "/") != 0) && 
> +	    (strcmp(name, ".") != 0)) {
> +		context->names[idx].ino   = (unsigned long)-1;
> +		context->names[idx].pino  = inode->i_ino;
> +	} else {
> +		context->names[idx].ino   = inode->i_ino;
> +		context->names[idx].pino  = (unsigned long)-1;
> +	}
> +}
> +
> +/**
> + * audit_inode_child - collect inode info for created/removed objects
> + * @dname: inode's dentry name
> + * @inode: inode being audited
> + * @pino: inode number of dentry parent
> + *
> + * For syscalls that create or remove filesystem objects, audit_inode
> + * can only collect information for the filesystem object's parent.
> + * This call updates the audit context with the child's information.
> + * Syscalls that create a new filesystem object must be hooked after
> + * the object is created.  Syscalls that remove a filesystem object
> + * must be hooked prior, in order to capture the target inode during
> + * unsuccessful attempts.
> + */
> +void __audit_inode_child(const char *dname, const struct inode *inode,
> +			 unsigned long pino)
> +{
> +	int idx;
> +	struct audit_context *context = current->audit_context;

Does this process even if !audit_enabled?

> +
> +	if (!context->in_syscall)
> +		return;
> +
> +	/* determine matching parent */
> +	if (dname)
> +		for (idx = 0; idx < context->name_count; idx++)
> +			if (context->names[idx].pino == pino) {
> +				const char *n;
> +				const char *name = context->names[idx].name;
> +				int dlen = strlen(dname);
> +				int nlen = name ? strlen(name) : 0;
> +
> +				if (nlen < dlen)
> +					continue;
> +				
> +				/* disregard trailing slashes */
> +				n = name + nlen - 1;
> +				while ((*n == '/') && (n > name))
> +					n--;
> +
> +				/* find last path component */
> +				n = n - dlen + 1;
> +				if (n < name)
> +					continue;
> +				else if (n > name) {
> +					if (*--n != '/')
> +						continue;
> +					else
> +						n++;
> +				}
> +
> +				if (strncmp(n, dname, dlen) == 0)
> +					goto update_context;
> +			}
> +
> +	/* catch-all in case match not found */
> +	idx = context->name_count++;
> +	context->names[idx].name  = NULL;
> +	context->names[idx].pino  = pino;
> +#if AUDIT_DEBUG
> +	context->ino_count++;
> +#endif
> +
> +update_context:
> +	if (inode) {
> +		context->names[idx].ino   = inode->i_ino;
> +		context->names[idx].dev	  = inode->i_sb->s_dev;
> +		context->names[idx].mode  = inode->i_mode;
> +		context->names[idx].uid   = inode->i_uid;
> +		context->names[idx].gid   = inode->i_gid;
> +		context->names[idx].rdev  = inode->i_rdev;
> +	}
>  }
>  
>  /**
> 
> --
> Linux-audit mailing list
> Linux-audit redhat com
> https://www.redhat.com/mailman/listinfo/linux-audit
> 
> 


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