[patch] update: _working_ code to add device+inode check to ipt_owner.c

[patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

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

wow, gosh, it works.

okay, this is a patch to add support in iptables for per-program
firewall filtering.

also included is the patches to iptables-1.2.11.

i have confidence that this patch will provide support for
BOTH incoming AND outgoing per-program packet filtering.

reasons why patch was developed:

	fireflier is an on-demand popup firewall program that
	queues packets and asks the user there-and-then to create
	a firewall rule.

	fireflier can do a limited amount of per-program packet
	filtering but it is stateless.

	this patch hooks into netfilter, thereby allowing STATEFUL
	per-program per-connection firewall rules to be created.


potential uses:

	only allowing certain programs to do certain network
	activities (e.g. only allowing skype or other VoIP apps
	to do incoming connections on a certain port; only allowing
	mozilla to access port 80 but allowing lynx to do more)

	i cannot forsee a need for people to only do per-inode
	filtering that is going to cause them havoc.

	i _can_ however sort-of forsee a need for people to
	do per-mount-point filtering.

	for example, an nfs mount point or /usr/local may
	contain untrusted executables.

writing firewall rule - preconditions for use:

	note that of course you will need to ensure a userspace iptables
	policy of "DENY ALL" and "ALLOW specific".

	if you apply "DENY specific" rules on a per-program basis, all a
	non-root user has to do is to copy the binary, they will have
	changed the inode, the rule no longer applies.

importance for users:

	the strategic importance of this functionality (for the
	ordinary computer user) should not be underestimated.

	it allows "on-demand" firewalling to be done in
	userspace: generation of rules "on-the-fly" using
	programs like fireflier.

	no windows "personal firewall" program
	would be seen dead without being able to do
	per-program-plus-per-connection firewall filtering.

caveats:

	the task is not yet entirely complete: in userspace,
	it is necessary to do ls --inode <programname> and
	also determine the mount point, and then *keep that
	info up-to-date*.

	programs get installed by the administrator, the
	inode changes...

loads of rubbish questions about how to do a better job:

	i do not believe it to be sensible to have the kernel
	code doing that kind of checking (resolving the full
	pathname of an executable) but hey, if anyone feels
	otherwise, and knows of some pre-existing code to point
	me in the direction of, i'll add it, because it might
	be easier in the long run.

	for example... do i just hunt down the dentry
	cache entries back down the parent dentry->parent,
	prepending dentry->d_name and a "/" to construct the
	full path name?

	can i expect the dentry cache entries to all exist, i'm
	not going to run into flushed or out-of-date entries,
	am i? (what's this DCACHE_DISCONNECTED thing?)

	also where do i go from the root mount point?

	how do i turn the vfsmount mountpoint entry (/dev/hda5)
	into its mount point name?  hunt down its dentry again?

	what if i hit another mount point: how do i look up
	a mount point of another filesystem, given just an
	inode of the root of the device?


	has someone already done this before now, and if so,
	where?

	because it's the kind of code that would be extremely
	useful to have in selinux auditing.

	the selinux auditing log messages only presently include
	the name of the file (not the fully qualified path) and
	things like "error access denied to a local directory
	named "lib" isn't exactly very helpful!)

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 />


[-- Attachment #2: ipt_owner.patch --]
[-- Type: text/plain, Size: 7186 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	9 Sep 2004 15:32:32 -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	9 Sep 2004 15:32:32 -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	9 Sep 2004 15:32:40 -0000
@@ -7,6 +7,10 @@
 #define IPT_OWNER_PID	0x04
 #define IPT_OWNER_SID	0x08
 #define IPT_OWNER_COMM	0x10
+#define IPT_OWNER_INO	0x20
+#define IPT_OWNER_DEV	0x40
+
+#define IPT_DEVNAME_SZ 80
 
 struct ipt_owner_info {
     uid_t uid;
@@ -14,6 +18,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	9 Sep 2004 15:32:44 -0000
@@ -1,16 +1,34 @@
 /* Kernel module to match various things tied to sockets associated with
-   locally generated outgoing packets. */
+   locally generated outgoing packets.
+   
+   lkcl 2004sep9: match against filesystem on which program handling the
+                  packet can be found (IPT_OWNER_DEV) and also the inode
+				  on that filesystem of that same program.
+
+				  why anyone would want to only check just the mountpoint
+				  i don't know (well, i do - e.g. /usr/local is a
+				  separate untrusted or even an nfs-mounted partition)
+				  but i had to include and check the mountpoint because
+				  otherwise the inode is meaningless.
+   */
 
 /* (C) 2000 Marc Boucher <marc@mbsi.ca>
+ * (C) 2004 Luke Kenneth Casson Leighton <lkcl@lkcl.net>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
+ *
  */
 
 #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 +38,86 @@
 MODULE_AUTHOR("Marc Boucher <marc@mbsi.ca>");
 MODULE_DESCRIPTION("iptables owner match");
 
+/* lkcl: this function is in fs/proc/base.c.  it's a generic function
+ * derived from proc_exe_link().  it's inappropriate to leave that
+ * function in fs/proc/base.c.  but i don't care: i don't have the
+ * knowledge to say where it should go.  therefore i'm leaving
+ * it in fs/proc/base.c.
+ */
+extern int proc_task_dentry_lookup(struct task_struct *task,
+		                           struct dentry **dentry, 
+								   struct vfsmount **mnt);
+
+/*
+ * look up the dentry (for the inode) of the task's executable,
+ * plus lookup the mountpoint of the filesystem from where that
+ * executable came from.   then do exactly the same socket checking
+ * that all the other checks seem to be doing.
+ */
+static int proc_exe_check(struct task_struct *task, u_int8_t match,
+		                  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;
+
+	/* lkcl: i can't be bothered to make obtuse code out of some
+	 * boolean overkill logic cleverness.
+	 */
+	if (match & IPT_OWNER_INO && match & IPT_OWNER_DEV)
+		if (dentry->d_inode->i_ino == i_num &&
+			strncmp(mnt->mnt_devname, devname, IPT_DEVNAME_SZ) == 0)
+		return 0;
+	if (match & IPT_OWNER_INO)
+		if (dentry->d_inode->i_ino == i_num)
+		    return 0;
+	if (match & IPT_OWNER_DEV)
+		if (strncmp(mnt->mnt_devname, devname, IPT_DEVNAME_SZ) == 0)
+		    return 0;
+	return -ENOENT;
+}
+
+static int
+match_inode(const struct sk_buff *skb, u_int8_t match,
+		    const char *devname, unsigned long i_num)
+{
+	struct task_struct *g, *p;
+	struct files_struct *files;
+	int i;
+
+	read_lock(&tasklist_lock);
+	do_each_thread(g, p) {
+
+		if (proc_exe_check(p, match, 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 +261,12 @@
 			return 0;
 	}
 
+	if(info->match & IPT_OWNER_INO || info->match & IPT_OWNER_DEV) {
+		if (!match_inode(skb, info->match, info->device, info->ino) ^
+		    !!(info->invert & IPT_OWNER_INO))
+			return 0;
+	}
+
 	return 1;
 }
 

[-- Attachment #3: iptables.ipt_owner.patch --]
[-- Type: text/plain, Size: 3062 bytes --]

--- iptables-1.2.11/extensions/libipt_owner.c	2004-06-14 23:02:17.000000000 +0100
+++ /home/lkcl/src/iptables-1.2.11/debian/build/iptables-1.2.11/extensions/libipt_owner.c	2004-09-09 17:12:28.000000000 +0100
@@ -1,4 +1,8 @@
 /* Shared library add-on to iptables to add OWNER matching support. */
+
+/* lkcl2004sep09: code to do per-program packet filtering (by device name and inode number)
+ * Copyright (C) Luke Kenneth Casson Leighton <lkcl@lkcl.net>
+ */
 #include <stdio.h>
 #include <netdb.h>
 #include <string.h>
@@ -34,6 +38,14 @@
 "\n",
 IPTABLES_VERSION);
 #endif /* IPT_OWNER_COMM */
+#ifdef IPT_OWNER_INO
+	printf(
+"OWNER match v%s options:\n"
+"[!] --dev-owner name       Match local device\n"
+"[!] --ino-owner inode      Match local inode (always use with --dev-owner)\n"
+"\n",
+IPTABLES_VERSION);
+#endif /* IPT_OWNER_INO */
 }
 
 static struct option opts[] = {
@@ -44,6 +56,12 @@
 #ifdef IPT_OWNER_COMM
 	{ "cmd-owner", 1, 0, '5' },
 #endif
+#ifdef IPT_OWNER_INO
+	{ "ino-owner", 1, 0, '6' },
+#endif
+#ifdef IPT_OWNER_DEV
+	{ "dev-owner", 1, 0, '7' },
+#endif
 	{0}
 };
 
@@ -136,6 +154,33 @@
 		*flags = 1;
 		break;
 #endif
+#ifdef IPT_OWNER_INO
+	case '6':
+		check_inverse(optarg, &invert, &optind, 0);
+		ownerinfo->ino = strtoul(optarg, &end, 0);
+		if (*end != '\0' || end == optarg)
+			exit_error(PARAMETER_PROBLEM, "Bad OWNER INODE value `%s'", optarg);
+		if (invert)
+			ownerinfo->invert |= IPT_OWNER_INO;
+		ownerinfo->match |= IPT_OWNER_INO;
+		*flags = 1;
+		break;
+#endif
+#ifdef IPT_OWNER_DEV
+	case '7':
+		check_inverse(optarg, &invert, &optind, 0);
+		if(strlen(optarg) > sizeof(ownerinfo->device))
+			exit_error(PARAMETER_PROBLEM, "OWNER DEV `%s' too long, max %u characters", optarg, (unsigned int)sizeof(ownerinfo->device));
+
+		strncpy(ownerinfo->device, optarg, sizeof(ownerinfo->device));
+		ownerinfo->device[sizeof(ownerinfo->device)-1] = '\0';
+
+		if (invert)
+			ownerinfo->invert |= IPT_OWNER_DEV;
+		ownerinfo->match |= IPT_OWNER_DEV;
+		*flags = 1;
+		break;
+#endif
 
 	default:
 		return 0;
@@ -189,6 +234,16 @@
 			printf("%.*s ", (int)sizeof(info->comm), info->comm);
 			break;
 #endif
+#ifdef IPT_OWNER_DEV
+		case IPT_OWNER_DEV:
+			printf("%.*s ", (int)sizeof(info->device), info->device);
+			break;
+#endif
+#ifdef IPT_OWNER_INO
+		case IPT_OWNER_INO:
+			printf("%lu ", info->ino);
+			break;
+#endif
 		default:
 			break;
 		}
@@ -219,6 +274,12 @@
 #ifdef IPT_OWNER_COMM
 	print_item(info, IPT_OWNER_COMM, numeric, "OWNER CMD match ");
 #endif
+#ifdef IPT_OWNER_INO
+	print_item(info, IPT_OWNER_INO, numeric, "OWNER INO match ");
+#endif
+#ifdef IPT_OWNER_DEV
+	print_item(info, IPT_OWNER_DEV, numeric, "OWNER DEV match ");
+#endif
 }
 
 /* Saves the union ipt_matchinfo in parsable form to stdout. */
@@ -234,6 +295,12 @@
 #ifdef IPT_OWNER_COMM
 	print_item(info, IPT_OWNER_COMM, 0, "--cmd-owner ");
 #endif
+#ifdef IPT_OWNER_DEV
+	print_item(info, IPT_OWNER_DEV, 0, "--dev-owner ");
+#endif
+#ifdef IPT_OWNER_INO
+	print_item(info, IPT_OWNER_INO, 0, "--ino-owner ");
+#endif
 }
 
 static

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Chris Wright]

* Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> wow, gosh, it works.
> 
> okay, this is a patch to add support in iptables for per-program
> firewall filtering.
> 
> also included is the patches to iptables-1.2.11.
> 
> i have confidence that this patch will provide support for
> BOTH incoming AND outgoing per-program packet filtering.

Programs can share a socket.  Incoming is in interrupt context.  You
have no idea who will be woken up.  How do you handle this?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 09:19:31AM -0700, Chris Wright wrote:
> * Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> > wow, gosh, it works.
> > 
> > okay, this is a patch to add support in iptables for per-program
> > firewall filtering.
> > 
> > also included is the patches to iptables-1.2.11.
> > 
> > i have confidence that this patch will provide support for
> > BOTH incoming AND outgoing per-program packet filtering.
> 
> Programs can share a socket.  Incoming is in interrupt context.  You
> have no idea who will be woken up.  How do you handle this?
 
 chris, hi,

 thank you for mentioning this - i can quite cheerfully say
 that i have absolutely no idea what you are referring to, because
 i lack the knowledge on the design / inner workings to be able
 to say, but i am going to attempt to work out what you could
 mean.  please bear with me.

 perhaps you could help guide me through the ipt_owner.c code
 (or point me in the direction of some appropriate notes,
  documentation or prior discussions, or alternatively help
  me out with some keywords to search for).

 i didn't understand why the proscription that ipt_owner not
 be used for INCOMING packets (which someone mentioned to me).

 so if nothing else, like the other ipt_owner things, this patch
 would only be suitable for OUTGOING rules, yes?

 well, that's better than a kick in the teeth - esp. for userspace
 programs.

 doesn't help me with apollon (gift file sharing daemon which is
 kicked off in userspace by apollon client program) but hey.

 anyway.

 so we have, in the completely uncommented code ipt_owner.c, the
 match() function.  it passes in a sk_buff which has some packet
 header structs, a struct sock, which points to a file, which,
 from the usage inside match() shows me that the sk_buff has a
 uid and gid associated with its socket.

 okay, then we come to something like oh i dunno match_pid().

 this calls find_task_by_pid() on the iptables-owner-recorded
 PID.

 then, that task has files (including sockets) associated with it,
 so you have to search through every socket of that thing.  task.
 and you're looking for the file struct of the incoming packet.

 i'm ASSUMING that when you search the task that the task's
 files - (struct ttask_struct*)p)->files - contains AAALLLL
 sockets and files of that task, not just some it feels like.

 am i right about this?


 okay.  what happens for _shared_ sockets.

 under these circumstances... if you only had one rule for one
 PID, and the socket was shared with another PID then... yes, you'd
 end up in a situation where

 in fact, for servers which fork, this would be a problem.

 the task_struct would duplicate the files, you'd end up with a
 different PID...


 ... therefore, _that's_ why ipt_owner is not recommended for use
 with the --pid-owner argument because it's too fluid.

 is this what you are referring to when you say:

	> Programs can share a socket.  Incoming is in interrupt context.  You
	> have no idea who will be woken up.  How do you handle this?

 okay, assuming that this _is_ what you are referring to, i'll describe
 how i BELIEVE my patch to be different, and PLEASE, not least because i
 need to know the answer before i deploy this patch in a live system
 (*gibber*) but also for other people considering using this, please
 correct me on any points i may have got wrong.


 the difference between --pid-owner (IPT_OWNER_PID) and the new patch is
 that when a server forks(), i don't imagine that the mountpoint+inode
 (new IPT_OWNER_DEV+IPT_OWNER_INO) is going to change!

 ... or, and this might be the crux of the matter: when
 sockets are shared (e.g. via servers doing a fork(), is the
 files_struct REALLY duplicated such that the comparison if
 (fckeck_files(files, i) == file) check in match_sid(),
 match_pid(), etc. will FAIL, or are the files_struct entries
 shared between tasks - in particular are they shared between
 tasks that have been fork()ed?

 if they are NOT shared between tasks, then how is it possible
 to ensure that two processes do not attempt to bind to the same
 port number, for example?

 l.
 

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Chris Wright]

* Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> On Thu, Sep 09, 2004 at 09:19:31AM -0700, Chris Wright wrote:
> > * Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> > > 
> > > i have confidence that this patch will provide support for
> > > BOTH incoming AND outgoing per-program packet filtering.
> > 
> > Programs can share a socket.  Incoming is in interrupt context.  You
> > have no idea who will be woken up.  How do you handle this?
>  
>  chris, hi,
> 
>  thank you for mentioning this - i can quite cheerfully say
>  that i have absolutely no idea what you are referring to, because
>  i lack the knowledge on the design / inner workings to be able
>  to say, but i am going to attempt to work out what you could
>  mean.  please bear with me.

Heh, ok ;-)

>  perhaps you could help guide me through the ipt_owner.c code
>  (or point me in the direction of some appropriate notes,
>   documentation or prior discussions, or alternatively help
>   me out with some keywords to search for).
> 
>  i didn't understand why the proscription that ipt_owner not
>  be used for INCOMING packets (which someone mentioned to me).
> 
>  so if nothing else, like the other ipt_owner things, this patch
>  would only be suitable for OUTGOING rules, yes?

Yes, outgoing you know who sent it, the rule is checked in process
context.

>  well, that's better than a kick in the teeth - esp. for userspace
>  programs.
> 
>  doesn't help me with apollon (gift file sharing daemon which is
>  kicked off in userspace by apollon client program) but hey.
> 
>  anyway.
> 
>  so we have, in the completely uncommented code ipt_owner.c, the
>  match() function.  it passes in a sk_buff which has some packet
>  header structs, a struct sock, which points to a file, which,
>  from the usage inside match() shows me that the sk_buff has a
>  uid and gid associated with its socket.

No, it's indirect through the file (socket(2) returns a descriptor which
is a handle to a struct file *).  The file can be shared.

>  okay, then we come to something like oh i dunno match_pid().
> 
>  this calls find_task_by_pid() on the iptables-owner-recorded
>  PID.
> 
>  then, that task has files (including sockets) associated with it,
>  so you have to search through every socket of that thing.  task.
>  and you're looking for the file struct of the incoming packet.
> 
>  i'm ASSUMING that when you search the task that the task's
>  files - (struct ttask_struct*)p)->files - contains AAALLLL
>  sockets and files of that task, not just some it feels like.

that's correct.

>  am i right about this?
> 
>  okay.  what happens for _shared_ sockets.
> 
>  under these circumstances... if you only had one rule for one
>  PID, and the socket was shared with another PID then... yes, you'd
>  end up in a situation where
> 
>  in fact, for servers which fork, this would be a problem.
> 
>  the task_struct would duplicate the files, you'd end up with a
>  different PID...

yes.

>  ... therefore, _that's_ why ipt_owner is not recommended for use
>  with the --pid-owner argument because it's too fluid.
> 
>  is this what you are referring to when you say:

Yes.  Although consider fork/exec (so proc is different).

> 	> Programs can share a socket.  Incoming is in interrupt context.  You
> 	> have no idea who will be woken up.  How do you handle this?
> 
>  okay, assuming that this _is_ what you are referring to, i'll describe
>  how i BELIEVE my patch to be different, and PLEASE, not least because i
>  need to know the answer before i deploy this patch in a live system
>  (*gibber*) but also for other people considering using this, please
>  correct me on any points i may have got wrong.
> 
>  the difference between --pid-owner (IPT_OWNER_PID) and the new patch is
>  that when a server forks(), i don't imagine that the mountpoint+inode
>  (new IPT_OWNER_DEV+IPT_OWNER_INO) is going to change!

not really much different from comm match, right?  other than it's a
better match on the process?  i assume you look for a match then stop.
if the match is good, and the rule is allow...you have no idea who that
packet will be delievered to.  it's queued to the socket, and if more
than one process is waiting on that socket, you don't know which one
will be woken up.  so with this level of inaccuaracy, it's hard to say
you are buying security...

>  ... or, and this might be the crux of the matter: when
>  sockets are shared (e.g. via servers doing a fork(), is the
>  files_struct REALLY duplicated such that the comparison if
>  (fckeck_files(files, i) == file) check in match_sid(),
>  match_pid(), etc. will FAIL, or are the files_struct entries
>  shared between tasks - in particular are they shared between
>  tasks that have been fork()ed?

they are shared (i.e. another refcount on same structure).

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 11:48:46AM -0700, Chris Wright wrote:

> >  to say, but i am going to attempt to work out what you could
> >  mean.  please bear with me.
> 
> Heh, ok ;-)
 
 :) *mmm*

> >  so we have, in the completely uncommented code ipt_owner.c, the
> >  match() function.  it passes in a sk_buff which has some packet
> >  header structs, a struct sock, which points to a file, which,
> >  from the usage inside match() shows me that the sk_buff has a
> >  uid and gid associated with its socket.
> 
> No, it's indirect through the file (socket(2) returns a descriptor which
> is a handle to a struct file *).  The file can be shared.
 
 struct file* - that'd explain why you have to go through
 some hoops, using fcheck_files().

> >  the difference between --pid-owner (IPT_OWNER_PID) and the new patch is
> >  that when a server forks(), i don't imagine that the mountpoint+inode
> >  (new IPT_OWNER_DEV+IPT_OWNER_INO) is going to change!
> 
> not really much different from comm match, right?  

 based code on match_comm.

> other than it's a
> better match on the process?  

 yes?

> i assume you look for a match then stop.

 added match_inode() after match_comm().  cut/paste job (my favourite
 way of programming).

> if the match is good, and the rule is allow...you have no idea who that
> packet will be delievered to.  

 allow, allow...

> it's queued to the socket, and if more
> than one process is waiting on that socket, you don't know which one
> will be woken up.

 well... okay, i must be missing something.

 what's the disconnect between the task_list and the sockets (sk_buff)
 that makes that [not knowing which one will be woken up] relevant?

 so it's a socket: let's take an example - and i'm assuming for now
 that things like passing file descriptors over unix-domain-sockets
 between processes just ... doesn't happen, okay? :)

 apache.

 an executable named apache creates a TCP socket and binds to port 80,
 and let's assume it's the prefork model, so it binds to port 80 and
 then starts sharing that [as you say, with refs] between all the
 apache processes.

 in this instance, all the processes are going to be named "apache",
 yes?

 they'll all have the same inode number.

 under these circumstances, i don't honestly think it makes
 any difference as to whether one process named apache is
 waiting on the socket and another process also named apache
 (fired up from the same inode) gets woken up.

 i mean, _yes_ it would be a problem if there really were two
 apaches run from different configuration files [that bound to
 different port numbers - i do this all the time and then run
 *another* apache web server in a DMZ to do proxy redirects]

 under these circumstances the best you could hope to do to get
 some sort of separation would be to copy the apache binary
 e.g. have an apache2-8000 and an apache2-80, and apply a
 different set of rules to match each separate binary.

 
 _or_, is there something "odd" going on, that i've missed?

> so with this level of inaccuaracy, it's hard to say
> you are buying security...

 well with a default rule of "deny"...

 what have i missed?  i no unnerstand!

 in match_inode, it's an "and" rule, i.e. _only_ tasks with
 the right dev+inode, and then _only_ those tasks with sockets
 matching the right one get through...

 and you'd only use this in combination with other "match" rules which
 get "and"ed in - e.g. match dev+inode _and_ match port 80 _and_ match
 TCP before jumping to "ACCEPT"...

 so if anything, you could end up being _too_ specific and end up
 falling through to the default "DENY" rule.

 which is the whole idea.
 
 or in the case of fireflier, falling through to the default "QUEUE"
 rule to trigger user intervention.


> >  ... or, and this might be the crux of the matter: when
> >  sockets are shared (e.g. via servers doing a fork(), is the
> >  files_struct REALLY duplicated such that the comparison if
> >  (fckeck_files(files, i) == file) check in match_sid(),
> >  match_pid(), etc. will FAIL, or are the files_struct entries
> >  shared between tasks - in particular are they shared between
> >  tasks that have been fork()ed?
> 
> they are shared (i.e. another refcount on same structure).
 
 *whew* that makes it a lot easier.

 l.

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Chris Wright]

* Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> On Thu, Sep 09, 2004 at 11:48:46AM -0700, Chris Wright wrote:
> 
> > not really much different from comm match, right?  
> 
>  based code on match_comm.
> 
> > other than it's a
> > better match on the process?  
> 
>  yes?
> 
> > i assume you look for a match then stop.
> 
>  added match_inode() after match_comm().  cut/paste job (my favourite
>  way of programming).
> 
> > if the match is good, and the rule is allow...you have no idea who that
> > packet will be delievered to.  
> 
>  allow, allow...
> 
> > it's queued to the socket, and if more
> > than one process is waiting on that socket, you don't know which one
> > will be woken up.
> 
>  well... okay, i must be missing something.
> 
>  what's the disconnect between the task_list and the sockets (sk_buff)
>  that makes that [not knowing which one will be woken up] relevant?

There's nothing stopping the following:  

exec good_proc
	socket()
	fork
	exec bad_proc

Now good_proc and bad_proc are sharing a socket.  Packet comes in
destined for that socket.  Rule says it's ok to deliver to socket
(because of good_proc).  Packet delivered to socket, wakes up waiters
(good and bad).  Now, what's stopping the bad_proc from reading from the
socket?

>  so it's a socket: let's take an example - and i'm assuming for now
>  that things like passing file descriptors over unix-domain-sockets
>  between processes just ... doesn't happen, okay? :)

These do happen, which is part of the problem ;-)

>  apache.
> 
>  an executable named apache creates a TCP socket and binds to port 80,
>  and let's assume it's the prefork model, so it binds to port 80 and
>  then starts sharing that [as you say, with refs] between all the
>  apache processes.
> 
>  in this instance, all the processes are going to be named "apache",
>  yes?
> 
>  they'll all have the same inode number.
> 
>  under these circumstances, i don't honestly think it makes
>  any difference as to whether one process named apache is
>  waiting on the socket and another process also named apache
>  (fired up from the same inode) gets woken up.

Sure.  Now, take xinted...it happens to close all of the fds except for
the one it's sending on down to the child from within the child just
prior to exec, but you get the idea.

>  i mean, _yes_ it would be a problem if there really were two
>  apaches run from different configuration files [that bound to
>  different port numbers - i do this all the time and then run
>  *another* apache web server in a DMZ to do proxy redirects]
> 
>  under these circumstances the best you could hope to do to get
>  some sort of separation would be to copy the apache binary
>  e.g. have an apache2-8000 and an apache2-80, and apply a
>  different set of rules to match each separate binary.

*nod*, or have something that's labelling each context, and match on
that.  even this isn't quite enough, w/out watching how fds get passed
around.

>  _or_, is there something "odd" going on, that i've missed?
> 
> > so with this level of inaccuaracy, it's hard to say
> > you are buying security...
> 
>  well with a default rule of "deny"...
> 
>  what have i missed?  i no unnerstand!
> 
>  in match_inode, it's an "and" rule, i.e. _only_ tasks with
>  the right dev+inode, and then _only_ those tasks with sockets
>  matching the right one get through...

it got through, but does the process you want get the packet?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 04:04:49PM -0700, Chris Wright wrote:

> >  what's the disconnect between the task_list and the sockets (sk_buff)
> >  that makes that [not knowing which one will be woken up] relevant?
> 
> There's nothing stopping the following:  
> 
> exec good_proc
> 	socket()
> 	fork
> 	exec bad_proc
 
> Now good_proc and bad_proc are sharing a socket.  Packet comes in
> destined for that socket.  Rule says it's ok to deliver to socket
> (because of good_proc).  

> Packet delivered to socket, wakes up waiters
> (good and bad).  Now, what's stopping the bad_proc from reading from the
> socket?

 okay.

 i am not so worried about this scenario _because_:

 under an selinux system, you would set up a policy which only
 allowed the good_proc to exec other_good_procs (with the
 macro can_exec(good_proc, { other_good_proc1, other_good_proc2 })


 consequently, you'd design your firewall rules (in conjunction with
 your selinux policy) to add _two_ dev+inode-program-enabled firewall rules
 to cover the same socket, e.g. apache2 (good_proc) and some cgi script
 helper (other_good_proc) - one for each program:

	 iptables -A INPUT -m tcp --dport=80 -m program --exe=/usr/bin/apache2 -j ACCEPT

 and:

	 iptables -A INPUT -m tcp --dport=80 -m program --exe=/usr/cgi-bin/blahblah -j ACCEPT

 
> >  so it's a socket: let's take an example - and i'm assuming for now
> >  that things like passing file descriptors over unix-domain-sockets
> >  between processes just ... doesn't happen, okay? :)
> 
> These do happen, which is part of the problem ;-)
 
 i would not consider this to be a problem [in an environment where
 you specify DENY as the default and ALLOW specific instances]

 under such circumstances [file descs passed between programs]...
 you would end up having to create _two_ program-specific rules, like
 above.

 one for each of the two programs.

 [this has got to be better than the present situation, where you have
  to "iptables -A OUTPUT -m tcp --sport=25" and that allows ANY process
  under the sun to have data escaping on port 25.]

 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] update: _working_ code to add device+inode check to ipt_owner.c

[Chris Wright]

* Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
>  i am not so worried about this scenario _because_:
> 
>  under an selinux system, you would set up a policy which only
>  allowed the good_proc to exec other_good_procs (with the
>  macro can_exec(good_proc, { other_good_proc1, other_good_proc2 })

OK, good.  Although, this does not help xinetd, so we're still trusting
that code.

>  consequently, you'd design your firewall rules (in conjunction with
>  your selinux policy) to add _two_ dev+inode-program-enabled firewall rules
>  to cover the same socket, e.g. apache2 (good_proc) and some cgi script
>  helper (other_good_proc) - one for each program:
> 
> 	 iptables -A INPUT -m tcp --dport=80 -m program --exe=/usr/bin/apache2 -j ACCEPT
> 
>  and:
> 
> 	 iptables -A INPUT -m tcp --dport=80 -m program --exe=/usr/cgi-bin/blahblah -j ACCEPT

Isn't this likely to be in modcgi/modperl/etc instead of fork/exec'd?
This is one specific example (which SELinux doesn't support, so it's
moot there) where changing security domains during execution would
confuse these rules.  E.g. reducing to a more restrictive domain while
executing cgi-scripts.

> > >  so it's a socket: let's take an example - and i'm assuming for now
> > >  that things like passing file descriptors over unix-domain-sockets
> > >  between processes just ... doesn't happen, okay? :)
> > 
> > These do happen, which is part of the problem ;-)
>  
>  i would not consider this to be a problem [in an environment where
>  you specify DENY as the default and ALLOW specific instances]
> 
>  under such circumstances [file descs passed between programs]...
>  you would end up having to create _two_ program-specific rules, like
>  above.
> 
>  one for each of the two programs.

Actually you wouldn't, just one.  It will match, then one of those
processes will get woken up and receive the data, regardless of whether
you meant to allow it.  So, having some security layer involved mediated
file desc passing clearly helps.  Point remains, any match will deliver
data to socket.  However, if socket is shared, it could be a different
process picking up data off of socket.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 05:21:35PM -0700, Chris Wright wrote:
> >  under such circumstances [file descs passed between programs]...
> >  you would end up having to create _two_ program-specific rules, like
> >  above.
> > 
> >  one for each of the two programs.
> 
> Actually you wouldn't, just one.  It will match, then one of those
> processes will get woken up and receive the data, regardless of whether
> you meant to allow it.  

 blehhrrr....

 oh i get it.  
 
 is that like someone writing really poor quality code where
 you have two processes reading from the same socket, wot like
 you're not supposed to do?

 or are there real instances / times where you really _do_ want
 that sort of thing to happen (xinetd?)

 [btw the sk_socket->file thing isn't filled in on input packets,
  but you still get the packet.  arg.  how the heck does ip_queue
  get enough info???]

 ta,

 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] update: _working_ code to add device+inode check to ipt_owner.c

[Chris Wright]

* Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> On Thu, Sep 09, 2004 at 05:21:35PM -0700, Chris Wright wrote:
> > >  under such circumstances [file descs passed between programs]...
> > >  you would end up having to create _two_ program-specific rules, like
> > >  above.
> > > 
> > >  one for each of the two programs.
> > 
> > Actually you wouldn't, just one.  It will match, then one of those
> > processes will get woken up and receive the data, regardless of whether
> > you meant to allow it.  
> 
>  blehhrrr....
> 
>  oh i get it.  
>  
>  is that like someone writing really poor quality code where
>  you have two processes reading from the same socket, wot like
>  you're not supposed to do?

I don't think it's behaviour many apps rely on.  But this is exactly the
kind of behaviour which can break security models.

>  or are there real instances / times where you really _do_ want
>  that sort of thing to happen (xinetd?)

Well, xinted won't really read from multiple processes simultaneously
(if all is working properly).  The xinetd server will see the initial
packet, then fork/exec and close off all extra fds.  Now, try and write
a firewall ruleset that mandatorily enforces that.  See the trouble?

>  [btw the sk_socket->file thing isn't filled in on input packets,
>   but you still get the packet.  arg.  how the heck does ip_queue
>   get enough info???]

Heh, right.  The sock is protocol specific.  The input happening on ip
level is before sock lookup.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 06:08:38PM -0700, Chris Wright wrote:
> * Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> > On Thu, Sep 09, 2004 at 05:21:35PM -0700, Chris Wright wrote:
> > > >  under such circumstances [file descs passed between programs]...
> > > >  you would end up having to create _two_ program-specific rules, like
> > > >  above.
> > > > 
> > > >  one for each of the two programs.
> > > 
> > > Actually you wouldn't, just one.  It will match, then one of those
> > > processes will get woken up and receive the data, regardless of whether
> > > you meant to allow it.  
> > 
> >  blehhrrr....
> > 
> >  oh i get it.  
> >  
> >  is that like someone writing really poor quality code where
> >  you have two processes reading from the same socket, wot like
> >  you're not supposed to do?
> 
> I don't think it's behaviour many apps rely on.  But this is exactly the
> kind of behaviour which can break security models.
> 
> >  or are there real instances / times where you really _do_ want
> >  that sort of thing to happen (xinetd?)
> 
> Well, xinted won't really read from multiple processes simultaneously
> (if all is working properly).  The xinetd server will see the initial
> packet, then fork/exec and close off all extra fds.  Now, try and write
> a firewall ruleset that mandatorily enforces that.  See the trouble?
 
 hmmm... *thinks*...

 thought-experiment:

 it'd involve doing a userspace rule that caught the packet.


> >  [btw the sk_socket->file thing isn't filled in on input packets,
> >   but you still get the packet.  arg.  how the heck does ip_queue
> >   get enough info???]
> 
> Heh, right.  The sock is protocol specific.  The input happening on ip
> level is before sock lookup.
 
 *neck muscles tensing causing head to vibrate at about 8Hz*
 rrrrrr arg!  

 okay - chris, anyone - got any tips on triggering socket lookup?

 what sort of things should i be looking for in netfilter that does
 the socket lookup?

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 07:10:34PM +0100, Luke Kenneth Casson Leighton wrote:

> On Thu, Sep 09, 2004 at 09:19:31AM -0700, Chris Wright wrote:
> > * Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> > > wow, gosh, it works.
> > > 
> > > okay, this is a patch to add support in iptables for per-program
> > > firewall filtering.
> > > 
> > > also included is the patches to iptables-1.2.11.
> > > 
> > > i have confidence that this patch will provide support for
> > > BOTH incoming AND outgoing per-program packet filtering.
> > 
> > Programs can share a socket.  Incoming is in interrupt context.  You
> > have no idea who will be woken up.  How do you handle this?
>  
>  chris, hi,

chris - for example, i notice that at the top of ipt_owner.c it says
"deals with local outgoing sockets".

so... does sk_buff _only_ contain a list of outgoing sockets?

iiiisss... there a different socket for incoming traffic that
someone is different from the list of sockets associated with
a task?

is the clue in what you say about "Incoming is in interrupt context"?

are the sockets in the interrupt context somehow different / special
such that they would never get to this code?

gloop, gloop, i'm drowning in lack of knowledge, here.

l.

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Chris Wright]

* Luke Kenneth Casson Leighton (lkcl@lkcl.net) wrote:
> chris - for example, i notice that at the top of ipt_owner.c it says
> "deals with local outgoing sockets".
> 
> so... does sk_buff _only_ contain a list of outgoing sockets?

no, and in fact outgoing there's no question who's sending the packet,
as it's still in process context.

> iiiisss... there a different socket for incoming traffic that
> someone is different from the list of sockets associated with
> a task?
> 
> is the clue in what you say about "Incoming is in interrupt context"?
> 
> are the sockets in the interrupt context somehow different / special
> such that they would never get to this code?

Depends on where the hooks are registered into netfilter whether you'll
get the inbound stuff.  I'm assuming this part is ok.  Point is, the act
of receiving a packet and queueing data to a socket does not happen in
process context.  So you don't know who will be woken up to actually
read data from that socket.  Your stuff should work for most cases.  But
it's not fundamentally deterministic enough to call it really secure.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 04:41:44PM -0700, Chris Wright wrote:

> > are the sockets in the interrupt context somehow different / special
> > such that they would never get to this code?
> 
> Depends on where the hooks are registered into netfilter whether you'll
> get the inbound stuff.  

 eek!

 e.g. ip_queue definitely gets it because fireflier's userspace code
 is able to determine the program name [from the pid, and it then
 goes hunting through /proc *gibber*] on both incoming and outgoing
 packets.

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Stephen Smalley]

On Thu, 2004-09-09 at 12:22, Luke Kenneth Casson Leighton wrote:
> 	i do not believe it to be sensible to have the kernel
> 	code doing that kind of checking (resolving the full
> 	pathname of an executable) but hey, if anyone feels
> 	otherwise, and knows of some pre-existing code to point
> 	me in the direction of, i'll add it, because it might
> 	be easier in the long run.
<snip>
> 	has someone already done this before now, and if so,
> 	where?

d_path() will give you a pathname given a (dentry, vfsmount) pair.

> 	because it's the kind of code that would be extremely
> 	useful to have in selinux auditing.
> 
> 	the selinux auditing log messages only presently include
> 	the name of the file (not the fully qualified path) and
> 	things like "error access denied to a local directory
> 	named "lib" isn't exactly very helpful!)

SELinux already uses d_path when it can when generating audit messages
(but always includes device and inode information); try reading the
code.  But a vfsmount is often not available to it at the point of a
permission check.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Stephen Smalley]

On Thu, 2004-09-09 at 12:29, Stephen Smalley wrote:
> SELinux already uses d_path when it can when generating audit messages
> (but always includes device and inode information); try reading the
> code.  But a vfsmount is often not available to it at the point of a
> permission check.

But note that you can get more pathname information by enabling the
kernel syscall auditing support, i.e. boot with audit=1.  Then, whenever
SELinux generates an audit message during syscall processing, the audit
framework will also generate an audit message on syscall exit that
includes parameter information, including the supplied pathname (for
what it is worth).  Those audit messages can be tied together based on
serial number.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [patch] update: _working_ code to add device+inode check to ipt_owner.c

[Luke Kenneth Casson Leighton]

On Thu, Sep 09, 2004 at 12:29:07PM -0400, Stephen Smalley wrote:
> On Thu, 2004-09-09 at 12:22, Luke Kenneth Casson Leighton wrote:
> > 	i do not believe it to be sensible to have the kernel
> > 	code doing that kind of checking (resolving the full
> > 	pathname of an executable) but hey, if anyone feels
> > 	otherwise, and knows of some pre-existing code to point
> > 	me in the direction of, i'll add it, because it might
> > 	be easier in the long run.
> <snip>
> > 	has someone already done this before now, and if so,
> > 	where?
> 
> d_path() will give you a pathname given a (dentry, vfsmount) pair.
 
 GREAT.
 
 thank you, that means i _can_ put full path names into an
 iptables rule, which will make life a lot easier from a userspace
 perspective.  i'm a bit worried about keeping the rules list
 up-to-date if an inode changes.

 fireflier already constructs the full path name of the executable
 in its userspace code, for comparison against its rules.

 _i_ accept the performance penalty (per per-packet) but some
 people won't.... and such people will just have to live with
 per-packet firewall rules (not per-packet per-program).

 [or, and i mention this for the benefit of lkml people,
  to create per-program SE/Linux network policy rules, as
  already described by stephen on sel-ml last week]

 l.