[patch] to add device+inode check to ipt_owner.c - HACKED UP

[patch] to add device+inode check to ipt_owner.c - HACKED UP

[Luke Kenneth Casson Leighton]

dear kernel people,

this is a first pass at attempting to add per-program firewall rule
checking to iptables.

fireflier makes a complete hash of this in user-space because there
is no way it track state information (therefore it cannot track RST
and FIN and FIN ACK packets)

so, i figured i'd try do this in kernel space instead, where it
should be done.

digitally signing the name of the exe is not okay.
_using_ the name of the exe isn't really okay in my book, either.

inodes are only meaningful on a per-device basis.

therefore i add a device (80 chars as a hack) and an inode number
argument to ipt_owner.h

i blatantly cut/paste the code from fs/proc/base.c into ipt_owner.c
only to find that a compile resulted in warnings about mmput
and mmget (present in kernel/sched.c) not existing.

therefore i blatantly hacked fs/proc/base.c to expose the functionality
i required, which is, "given a task struct, give me the dentry and
vfsmount structs associated with its executable".  i called it
proc_task_dentry_lookup().

from there, i can grab the inode number and the device name.

i had to add an EXPORT_SYMBOL(proc_task_dentry_lookup).
i realise this is not ideal (it shouldn't be in fs/proc/base.c,
ipt_owner.c shouldn't be dependent on CONFIG_PROC_FS, etc. at
this stage i don't care)

i feel certain that the functionality required in this function
is not only required elsewhere but also available elsewhere
(in which case, why is it being duplicated in fs/proc/base.c)
so clearly i must be missing something.


from previous messages:

- default rules should be "DENY ALL" and invididual "ALLOW"s this patch
  is NOT good to use with "DENY" rules because users can always copy the
  program (even on selinux systems)



anyone reading this far be warned of the following:

- if you don't like what i have done, i so don't care: i HAVE to
  get this to work and no amount of "i don't like" is going to
  take that away.  don't like it?  save yourself some effort:
  delete this message and your mindset.

- if you do like what i have done, and you are not an experienced
  kernel hacker, please be aware that i am still experimenting,
  i am very much in the dark on this, and your input and
  assistance would be greatly appreciated, but you NEED to take
  precautions to ensure it doesn't do your system any damage.

- you'll also need a hacked version of userspace iptables.
  i'm working on it.

with that in mind, comments and assistance much appreciated because
i feel certain that i cannot be the only person looking for this
sort of functionality.

l.

-- 
--
Truth, honesty and respect are rare commodities that all spring from
the same well: Love.  If you love yourself and everyone and everything
around you, funnily and coincidentally enough, life gets a lot better.
--
<a href="http://lkcl.net">      lkcl.net      </a> <br />
<a href="mailto:lkcl@lkcl.net"> lkcl@lkcl.net </a> <br />

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[Arjan van de Ven]

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

On Wed, 2004-09-08 at 12:09, Luke Kenneth Casson Leighton wrote:
> dear kernel people,
> 
> this is a first pass at attempting to add per-program firewall rule
> checking to iptables.

question: any reason you didn't use something like selinux-like contexts
instead of dentry/device pairs ? 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[Luke Kenneth Casson Leighton]

On Wed, Sep 08, 2004 at 12:14:50PM +0200, Arjan van de Ven wrote:
> On Wed, 2004-09-08 at 12:09, Luke Kenneth Casson Leighton wrote:
> > dear kernel people,
> > 
> > this is a first pass at attempting to add per-program firewall rule
> > checking to iptables.
> 
> question: any reason you didn't use something like selinux-like contexts
> instead of dentry/device pairs ? 

a very good question: stephen smalley described an approach in which
exactly what you suggest can be done.

please bear with me whilst i explain, then i will answer.

the issue is that FireFlier is an on-demand (user-driven) popup firewall
program [and there literally ISN'T any firewall program available for
linux that even remotely comes close to the same capabilities as
fireflier]

so rules are queued (ipt_queue) and the popup thrown at the user until
they select "yes, no, create-a-firewall-rule".

to parallel the same functionality i would need to place a hook in
selinux to catch an audit operation (hooks are already there), then
alert the user to it, then create a rule, recompile the policy, and
_then_ let the hook proceed.

i'm not sure if this would work!!!

so, i didn't want to use selinux contexts because  it involves
dynamically creating selinux policy rules.

fireflier is NOT a "create-it-once-then-apply-it-suck-it-and-see"
firewall program.

it's an on-demand "popup" firewall program where the default is
"block by virtue of the packet being in the ip_queue, awaiting
 user approval or disapproval".


unless... *shudder* ... you mean ... why didn't i consider getting
FireFlier to _create_ selinux contexts, blatting them into the policy
directly? (which i know is possible, there do exist binary policy
editing-and-writing tools).

well... if this approach turns out to be a total nightmare, then
your question is really appreciated because it makes me think of
other possibilities.

l.


-- 
--
Truth, honesty and respect are rare commodities that all spring from
the same well: Love.  If you love yourself and everyone and everything
around you, funnily and coincidentally enough, life gets a lot better.
--
<a href="http://lkcl.net">      lkcl.net      </a> <br />
<a href="mailto:lkcl@lkcl.net"> lkcl@lkcl.net </a> <br />

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[Luke Kenneth Casson Leighton]

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

... did i sent a patch?

did i send a patch??  i don't _think_ so *lol* :)

On Wed, Sep 08, 2004 at 11:09:47AM +0100, Luke Kenneth Casson Leighton wrote:
> dear kernel people,
> 
> this is a first pass at attempting to add per-program firewall rule
> checking to iptables.

[-- Attachment #2: ipt_owner.patch --]
[-- Type: text/plain, Size: 6491 bytes --]

Index: fs/proc/base.c
===================================================================
RCS file: /cvsroot/selinux/nsa/linux-2.6/fs/proc/base.c,v
retrieving revision 1.1.1.9
diff -u -u -r1.1.1.9 base.c
--- fs/proc/base.c	18 Jun 2004 19:30:20 -0000	1.1.1.9
+++ fs/proc/base.c	8 Sep 2004 01:09:10 -0000
@@ -206,11 +206,12 @@
 	return -ENOENT;
 }
 
-static int proc_exe_link(struct inode *inode, struct dentry **dentry, struct vfsmount **mnt)
+extern int proc_task_dentry_lookup(struct task_struct *task, struct dentry **dentry, struct vfsmount **mnt);
+
+int proc_task_dentry_lookup(struct task_struct *task, struct dentry **dentry, struct vfsmount **mnt)
 {
 	struct vm_area_struct * vma;
 	int result = -ENOENT;
-	struct task_struct *task = proc_task(inode);
 	struct mm_struct * mm = get_task_mm(task);
 
 	if (!mm)
@@ -233,6 +234,11 @@
 	return result;
 }
 
+static int proc_exe_link(struct inode *inode, struct dentry **dentry, struct vfsmount **mnt)
+{
+	return proc_task_dentry_lookup(proc_task(inode), dentry, mnt);
+}
+
 static int proc_cwd_link(struct inode *inode, struct dentry **dentry, struct vfsmount **mnt)
 {
 	struct fs_struct *fs;
Index: fs/proc/root.c
===================================================================
RCS file: /cvsroot/selinux/nsa/linux-2.6/fs/proc/root.c,v
retrieving revision 1.1.1.2
diff -u -u -r1.1.1.2 root.c
--- fs/proc/root.c	8 Apr 2004 14:13:50 -0000	1.1.1.2
+++ fs/proc/root.c	8 Sep 2004 01:09:10 -0000
@@ -147,6 +147,8 @@
 	.parent		= &proc_root,
 };
 
+extern int proc_task_dentry_lookup(struct task_struct *task, struct dentry **dentry, struct vfsmount **mnt);
+
 #ifdef CONFIG_SYSCTL
 EXPORT_SYMBOL(proc_sys_root);
 #endif
@@ -159,3 +161,4 @@
 EXPORT_SYMBOL(proc_net);
 EXPORT_SYMBOL(proc_bus);
 EXPORT_SYMBOL(proc_root_driver);
+EXPORT_SYMBOL(proc_task_dentry_lookup);
Index: include/linux/netfilter_ipv4/ipt_owner.h
===================================================================
RCS file: /cvsroot/selinux/nsa/linux-2.6/include/linux/netfilter_ipv4/ipt_owner.h,v
retrieving revision 1.1.1.1
diff -u -u -r1.1.1.1 ipt_owner.h
--- include/linux/netfilter_ipv4/ipt_owner.h	14 Aug 2003 12:09:16 -0000	1.1.1.1
+++ include/linux/netfilter_ipv4/ipt_owner.h	8 Sep 2004 01:09:14 -0000
@@ -7,6 +7,9 @@
 #define IPT_OWNER_PID	0x04
 #define IPT_OWNER_SID	0x08
 #define IPT_OWNER_COMM	0x10
+#define IPT_OWNER_INO	0x20
+
+#define IPT_DEVNAME_SZ 80
 
 struct ipt_owner_info {
     uid_t uid;
@@ -14,6 +17,12 @@
     pid_t pid;
     pid_t sid;
     char comm[16];
+
+	/* set these as a pair: specify the filesystem, specify the inode */
+	/* it's the only simple (and unambigous) way to reference a program */
+	char device[IPT_DEVNAME_SZ];
+    unsigned long ino;
+
     u_int8_t match, invert;	/* flags */
 };
 
Index: net/ipv4/netfilter/ipt_owner.c
===================================================================
RCS file: /cvsroot/selinux/nsa/linux-2.6/net/ipv4/netfilter/ipt_owner.c,v
retrieving revision 1.1.1.4
diff -u -u -r1.1.1.4 ipt_owner.c
--- net/ipv4/netfilter/ipt_owner.c	13 May 2004 18:03:23 -0000	1.1.1.4
+++ net/ipv4/netfilter/ipt_owner.c	8 Sep 2004 01:09:16 -0000
@@ -11,6 +11,11 @@
 #include <linux/module.h>
 #include <linux/skbuff.h>
 #include <linux/file.h>
+#include <linux/rwsem.h>
+#include <linux/mount.h>
+#include <linux/dcache.h>
+#include <linux/string.h>
+#include <linux/sched.h>
 #include <net/sock.h>
 
 #include <linux/netfilter_ipv4/ipt_owner.h>
@@ -20,6 +25,117 @@
 MODULE_AUTHOR("Marc Boucher <marc@mbsi.ca>");
 MODULE_DESCRIPTION("iptables owner match");
 
+extern int proc_task_dentry_lookup(struct task_struct *task, struct dentry **dentry, struct vfsmount **mnt);
+
+static int proc_exe_check(struct task_struct *task,
+		                  const char *devname, unsigned long i_num)
+{
+    int result = -ENOENT;
+	struct vfsmount *mnt;
+    struct dentry *dentry;
+	result = proc_task_dentry_lookup(task, &dentry, &mnt);
+	if (result != 0)
+		return result;
+
+	if (!dentry->d_inode)
+		return -ENOENT;
+	if (dentry->d_inode->i_ino == i_num &&
+			strncmp(mnt->mnt_devname, devname, IPT_DEVNAME_SZ) == 0)
+		return 0;
+	return -ENOENT;
+}
+
+#if 0
+static int proc_exe_check(struct task_struct *task,
+		                  const char *devname, unsigned long i_num)
+{
+    struct vm_area_struct * vma;
+    int result = -ENOENT;
+    struct mm_struct * mm = get_task_mm(task);
+
+    if (!mm)
+        goto out;
+    down_read(&mm->mmap_sem);
+    vma = mm->mmap;
+    while (vma) {
+        if ((vma->vm_flags & VM_EXECUTABLE) &&
+            vma->vm_file) {
+            struct vfsmount *mnt = mntget(vma->vm_file->f_vfsmnt);
+            struct dentry *dentry = dget(vma->vm_file->f_dentry);
+			if (!dentry->d_inode)
+				continue;
+			if (dentry->d_inode->i_ino == i_num &&
+					strncmp(mnt->mnt_devname, devname, IPT_DEVNAME_SZ) == 0)
+				{
+					result = 0;
+					break;
+				}
+        }
+        vma = vma->vm_next;
+    }
+    up_read(&mm->mmap_sem);
+    mmput(mm);
+out:
+    return result;
+}
+#endif
+
+static int
+match_inode(const struct sk_buff *skb, const char *devname, unsigned long i_num)
+{
+	struct task_struct *g, *p;
+	struct files_struct *files;
+	/*
+	struct inode *inode;
+	struct super_block *sb;
+	struct block_device *bd;
+	*/
+	int i;
+	read_lock(&tasklist_lock);
+
+	/* lkcl: these are fairly obvious (just obtuse): hunt for the
+	 * filesystem device, then its superblock, then the inode is
+	 * relevant to that superblock, _then_ we can find the inode.
+	bd = bdget(dev);
+	if (!bd)
+		goto out;
+
+	sb = get_super(bd);
+	if (!sb)
+		goto out;
+
+	inode = ilookup(sb, i_num);
+	if (!inode)
+		goto out;
+	 */
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+
+		if (proc_exe_check(p, devname, i_num))
+			continue;
+
+		task_lock(p);
+		files = p->files;
+		if(files) {
+			spin_lock(&files->file_lock);
+			for (i=0; i < files->max_fds; i++) {
+				if (fcheck_files(files, i) ==
+				    skb->sk->sk_socket->file) {
+					spin_unlock(&files->file_lock);
+					task_unlock(p);
+					read_unlock(&tasklist_lock);
+					return 1;
+				}
+			}
+			spin_unlock(&files->file_lock);
+		}
+		task_unlock(p);
+	} while_each_thread(g, p);
+	read_unlock(&tasklist_lock);
+	return 0;
+}
+
 static int
 match_comm(const struct sk_buff *skb, const char *comm)
 {
@@ -163,6 +279,12 @@
 			return 0;
 	}
 
+	if(info->match & IPT_OWNER_INO) {
+		if (!match_inode(skb, info->device, info->ino) ^
+		    !!(info->invert & IPT_OWNER_INO))
+			return 0;
+	}
+
 	return 1;
 }
 

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[viro]

On Wed, Sep 08, 2004 at 11:39:22AM +0100, Luke Kenneth Casson Leighton wrote:
> +static int
> +match_inode(const struct sk_buff *skb, const char *devname, unsigned long i_num)
> +{
> +	struct task_struct *g, *p;
> +	struct files_struct *files;
> +	/*
> +	struct inode *inode;
> +	struct super_block *sb;
> +	struct block_device *bd;
> +	*/
> +	int i;
> +	read_lock(&tasklist_lock);
> +
> +	/* lkcl: these are fairly obvious (just obtuse): hunt for the
> +	 * filesystem device, then its superblock, then the inode is
> +	 * relevant to that superblock, _then_ we can find the inode.
> +	bd = bdget(dev);


... the hell?  Where does that "dev" come from?

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[Luke Kenneth Casson Leighton]

hello, hello, thank you v. much for responding.

On Wed, Sep 08, 2004 at 11:47:39AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:

> On Wed, Sep 08, 2004 at 11:39:22AM +0100, Luke Kenneth Casson Leighton wrote:
> > +static int
> > +match_inode(const struct sk_buff *skb, const char *devname, unsigned long i_num)
> > +{
> > +	struct task_struct *g, *p;
> > +	struct files_struct *files;
> > +	/*
> > +	struct inode *inode;
> > +	struct super_block *sb;
> > +	struct block_device *bd;
> > +	*/
> > +	int i;
> > +	read_lock(&tasklist_lock);
> > +
> > +	/* lkcl: these are fairly obvious (just obtuse): hunt for the
> > +	 * filesystem device, then its superblock, then the inode is
> > +	 * relevant to that superblock, _then_ we can find the inode.
> > +	bd = bdget(dev);
> 
> 
> ... the hell?  Where does that "dev" come from?

it's code commented out that i put in there when i _really_ wasn't
sure what i was doing.

it might come in handy so i haven't deleted it yet.

basically in an earlier experiment, i put the device (dev_t)
major/minor number into ipt_owner.h.

then i discovered that fs/proc/base.c could look up a vfsmount
struct, which according to struct vfsmount contains the _name_
of the device.

i'm counting on that actually working.

if it doesn't work, then it's back to the drawing board and uncommenting
the stuff that you have noticed is all commented out, above.

l.


-- 
--
Truth, honesty and respect are rare commodities that all spring from
the same well: Love.  If you love yourself and everyone and everything
around you, funnily and coincidentally enough, life gets a lot better.
--
<a href="http://lkcl.net">      lkcl.net      </a> <br />
<a href="mailto:lkcl@lkcl.net"> lkcl@lkcl.net </a> <br />

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[Gianni Tedesco]

On Wed, 2004-09-08 at 11:39 +0100, Luke Kenneth Casson Leighton wrote:
> ... did i sent a patch?
> 
> did i send a patch??  i don't _think_ so *lol* :)

heh :)

IMO the number of constraints involed here make using this patch fairly
involved (for something security related at least) in that, as you said,
you have to:

 - be careful to use ACCEPT rules only
 - be careful that you do:
    1. remove fw rules
    2. upgrade software
    3. replace rules

plus the fastpath code looks very hairy with at least 3 locks taken and
O(num_tasks * max_fds) unpreemptable in softirq...

There has to be a simpler approach, perhaps passing in a path, looking
up the dentry in current namespace and setting an unused flag in
d_vfs_flags? That way you could just match on skb->sk->sk_socket->file-
>f_dentry.

I don't know enough about VFS to know if that's really possible. I mean
would you need vfsmnt too to make it accurate across namespaces? and if
so, could dcookie infrastructure be used?

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[Luke Kenneth Casson Leighton]

On Fri, Sep 10, 2004 at 08:49:28AM +0100, Gianni Tedesco wrote:
> On Wed, 2004-09-08 at 11:39 +0100, Luke Kenneth Casson Leighton wrote:
> > ... did i sent a patch?
> > 
> > did i send a patch??  i don't _think_ so *lol* :)
> 
> heh :)
> 
> IMO the number of constraints involed here make using this patch fairly
> involved (for something security related at least) in that, as you said,
> you have to:
> 
>  - be careful to use ACCEPT rules only
>  - be careful that you do:
>     1. remove fw rules
>     2. upgrade software
>     3. replace rules
 
 i've since replaced the code with use of d_path() which stephen
 smalley kindly pointed out, negating the need for doing a
 commit-replace-thing.

> plus the fastpath code looks very hairy with at least 3 locks taken and
> O(num_tasks * max_fds) unpreemptable in softirq...

 *shrug*.  that's the way ipt_owner works...

> There has to be a simpler approach, perhaps passing in a path, looking
> up the dentry in current namespace and setting an unused flag in
> d_vfs_flags? That way you could just match on skb->sk->sk_socket->file-
> >f_dentry.
 
  ooo...

> I don't know enough about VFS to know if that's really possible. I mean
> would you need vfsmnt too to make it accurate across namespaces? 


 well you need vfsmnt in order to trackdown the full path name.

 l.

-- 
--
Truth, honesty and respect are rare commodities that all spring from
the same well: Love.  If you love yourself and everyone and everything
around you, funnily and coincidentally enough, life gets a lot better.
--
<a href="http://lkcl.net">      lkcl.net      </a> <br />
<a href="mailto:lkcl@lkcl.net"> lkcl@lkcl.net </a> <br />

Re: [patch] to add device+inode check to ipt_owner.c - HACKED UP

[Luke Kenneth Casson Leighton]

On Fri, Sep 10, 2004 at 08:49:28AM +0100, Gianni Tedesco wrote:
> On Wed, 2004-09-08 at 11:39 +0100, Luke Kenneth Casson Leighton wrote:
> > ... did i sent a patch?
> > 
> > did i send a patch??  i don't _think_ so *lol* :)
> 
> heh :)
> 
> IMO the number of constraints involed here make using this patch fairly
> involved (for something security related at least) in that, as you said,
> you have to:
> 
>  - be careful to use ACCEPT rules only
>  - be careful that you do:
>     1. remove fw rules
>     2. upgrade software
>     3. replace rules
> 
> plus the fastpath code looks very hairy with at least 3 locks taken and
> O(num_tasks * max_fds) unpreemptable in softirq...
 
 it's no worse than the present fireflier solution, which on a
 per-packet basis in userspace will go hunting through /proc
 looking for the socket _that_ way *gibber*.

 fireflier reads /proc/NNNN/exe, then also hunts through the fds for
 that process on /proc looking for things beginning with "socket:".

 [actually it used not to bother with the qualification for "socket:"
  resulting in a complete nightmare time for creating appropriate
  selinux policy]

 l.