[PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Rusty Russell]

Name: Multiport revision with port ranges (replaces "mport")
Status: Tested under nfsim
Signed-off-by: Pablo Neira Ayuso <pablo@eurodev.net>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> (modified)

The multiport match doesn't support ranges of ports, so a new match
called "mport" was written.  Now we have versioning of matches and
targets, we can simply put this extension in multiport revision 1.

Also, removes gratuitous checking in match: we basically trust
iptables userspace these days.

===== net/ipv4/netfilter/ipt_multiport.c 1.8 vs edited =====
Index: linux-2.6.10-bk7-Netfilter/include/linux/netfilter_ipv4/ipt_multiport.h
===================================================================
--- linux-2.6.10-bk7-Netfilter.orig/include/linux/netfilter_ipv4/ipt_multiport.h	2005-01-05 12:59:28.122700808 +1100
+++ linux-2.6.10-bk7-Netfilter/include/linux/netfilter_ipv4/ipt_multiport.h	2005-01-05 13:02:41.176352192 +1100
@@ -18,4 +18,12 @@
 	u_int8_t count;				/* Number of ports */
 	u_int16_t ports[IPT_MULTI_PORTS];	/* Ports */
 };
+
+struct ipt_multiport_v1
+{
+	u_int8_t flags;				/* Type of comparison */
+	u_int8_t count;				/* Number of ports */
+	u_int16_t ports[IPT_MULTI_PORTS];	/* Ports */
+	u_int8_t pflags[IPT_MULTI_PORTS];	/* Port flags */
+};
 #endif /*_IPT_MULTIPORT_H*/
Index: linux-2.6.10-bk7-Netfilter/net/ipv4/netfilter/ipt_multiport.c
===================================================================
--- linux-2.6.10-bk7-Netfilter.orig/net/ipv4/netfilter/ipt_multiport.c	2005-01-05 12:59:28.121700960 +1100
+++ linux-2.6.10-bk7-Netfilter/net/ipv4/netfilter/ipt_multiport.c	2005-01-05 13:02:41.176352192 +1100
@@ -46,6 +46,50 @@
 	return 0;
 }
 
+/* Returns 1 if the port is matched by the test, 0 otherwise. */
+static inline int
+ports_match_v1(const struct ipt_multiport_v1 *minfo,
+	       u_int16_t src, u_int16_t dst)
+{
+	unsigned int i;
+	u_int16_t s, e;
+
+	for (i=0; i < minfo->count; i++) {
+		s = minfo->ports[i];
+
+		if (minfo->pflags[i]) {
+			/* range port matching */
+			e = minfo->ports[++i];
+			duprintf("src or dst matches with %d-%d?\n", s, e);
+
+			if (minfo->flags == IPT_MULTIPORT_SOURCE
+			    && src >= s && src <= e)
+				return 1;
+			if (minfo->flags == IPT_MULTIPORT_DESTINATION
+			    && dst >= s && dst <= e)
+				return 1;
+			if (minfo->flags == IPT_MULTIPORT_EITHER
+			    && ((dst >= s && dst <= e)
+				|| (src >= s && src <= e)))
+				return 1;
+		} else {
+			/* exact port matching */
+			duprintf("src or dst matches with %d?\n", s);
+			if (minfo->flags == IPT_MULTIPORT_SOURCE
+			    && src == s)
+				return 1;
+			if (minfo->flags == IPT_MULTIPORT_DESTINATION
+			    && dst == s)
+				return 1;
+			if (minfo->flags == IPT_MULTIPORT_EITHER
+			    && (src == s || dst == s))
+				return 1;
+		}
+	}
+ 
+ 	return 0;
+}
+
 static int
 match(const struct sk_buff *skb,
       const struct net_device *in,
@@ -57,14 +101,11 @@
 	u16 _ports[2], *pptr;
 	const struct ipt_multiport *multiinfo = matchinfo;
 
-	/* Must not be a fragment. */
 	if (offset)
 		return 0;
 
-	/* Must be big enough to read ports (both UDP and TCP have
-           them at the start). */
 	pptr = skb_header_pointer(skb, skb->nh.iph->ihl * 4,
-				  sizeof(_ports), &_ports[0]);
+				  sizeof(_ports), _ports);
 	if (pptr == NULL) {
 		/* We've been asked to examine this packet, and we
 		 * can't.  Hence, no choice but to drop.
@@ -80,6 +121,35 @@
 			   ntohs(pptr[0]), ntohs(pptr[1]));
 }
 
+static int
+match_v1(const struct sk_buff *skb,
+	 const struct net_device *in,
+	 const struct net_device *out,
+	 const void *matchinfo,
+	 int offset,
+	 int *hotdrop)
+{
+	u16 _ports[2], *pptr;
+	const struct ipt_multiport_v1 *multiinfo = matchinfo;
+
+	if (offset)
+		return 0;
+
+	pptr = skb_header_pointer(skb, skb->nh.iph->ihl * 4,
+				  sizeof(_ports), _ports);
+	if (pptr == NULL) {
+		/* We've been asked to examine this packet, and we
+		 * can't.  Hence, no choice but to drop.
+		 */
+		duprintf("ipt_multiport:"
+			 " Dropping evil offset=0 tinygram.\n");
+		*hotdrop = 1;
+		return 0;
+	}
+
+	return ports_match_v1(multiinfo, ntohs(pptr[0]), ntohs(pptr[1]));
+}
+
 /* Called when user tries to insert an entry of this type. */
 static int
 checkentry(const char *tablename,
@@ -88,36 +158,53 @@
 	   unsigned int matchsize,
 	   unsigned int hook_mask)
 {
-	const struct ipt_multiport *multiinfo = matchinfo;
-
-	if (matchsize != IPT_ALIGN(sizeof(struct ipt_multiport)))
-		return 0;
+	return (matchsize == IPT_ALIGN(sizeof(struct ipt_multiport)));
+}
 
-	/* Must specify proto == TCP/UDP, no unknown flags or bad count */
-	return (ip->proto == IPPROTO_TCP || ip->proto == IPPROTO_UDP)
-		&& !(ip->invflags & IPT_INV_PROTO)
-		&& matchsize == IPT_ALIGN(sizeof(struct ipt_multiport))
-		&& (multiinfo->flags == IPT_MULTIPORT_SOURCE
-		    || multiinfo->flags == IPT_MULTIPORT_DESTINATION
-		    || multiinfo->flags == IPT_MULTIPORT_EITHER)
-		&& multiinfo->count <= IPT_MULTI_PORTS;
+static int
+checkentry_v1(const char *tablename,
+	      const struct ipt_ip *ip,
+	      void *matchinfo,
+	      unsigned int matchsize,
+	      unsigned int hook_mask)
+{
+	return (matchsize == IPT_ALIGN(sizeof(struct ipt_multiport_v1)));
 }
 
 static struct ipt_match multiport_match = {
 	.name		= "multiport",
+	.revision	= 0,
 	.match		= &match,
 	.checkentry	= &checkentry,
 	.me		= THIS_MODULE,
 };
 
+static struct ipt_match multiport_match_v1 = {
+	.name		= "multiport",
+	.revision	= 1,
+	.match		= &match_v1,
+	.checkentry	= &checkentry_v1,
+	.me		= THIS_MODULE,
+};
+
 static int __init init(void)
 {
-	return ipt_register_match(&multiport_match);
+	int err;
+
+	err = ipt_register_match(&multiport_match);
+	if (!err) {
+		err = ipt_register_match(&multiport_match_v1);
+		if (err)
+			ipt_unregister_match(&multiport_match);
+	}
+
+	return err;
 }
 
 static void __exit fini(void)
 {
 	ipt_unregister_match(&multiport_match);
+	ipt_unregister_match(&multiport_match_v1);
 }
 
 module_init(init);

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Phil Oester]

On Wed, Jan 05, 2005 at 02:35:59PM +1100, Rusty Russell wrote:
> The multiport match doesn't support ranges of ports, so a new match
> called "mport" was written.  Now we have versioning of matches and
> targets, we can simply put this extension in multiport revision 1.

While I agree the above is a useful change, why not also add inversion
to multiport in the process -- as long as you're making a new revision?
Or should that be done in revision 2?

Phil

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Nicolas Bouliane]

Phil Oester wrote:
> On Wed, Jan 05, 2005 at 02:35:59PM +1100, Rusty Russell wrote:
> 
>>The multiport match doesn't support ranges of ports, so a new match
>>called "mport" was written.  Now we have versioning of matches and
>>targets, we can simply put this extension in multiport revision 1.
> 
> 
> While I agree the above is a useful change, why not also add inversion
> to multiport in the process -- as long as you're making a new revision?
> Or should that be done in revision 2?
> 
> Phil
> 

AFAIK iptables already support ranges of ports.

iptables -A INPUT -p tcp --sport 10:40


Cheers :)

-acidfu

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Rusty Russell]

On Tue, 2005-01-04 at 20:30 -0800, Phil Oester wrote:
> On Wed, Jan 05, 2005 at 02:35:59PM +1100, Rusty Russell wrote:
> > The multiport match doesn't support ranges of ports, so a new match
> > called "mport" was written.  Now we have versioning of matches and
> > targets, we can simply put this extension in multiport revision 1.
> 
> While I agree the above is a useful change, why not also add inversion
> to multiport in the process -- as long as you're making a new revision?
> Or should that be done in revision 2?

If you put it in before 2.6.11 is released, it can go in the current
revision, otherwise we want a new one (where do we put the invert
flags?).

Cheers,
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Phil Oester]

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

On Wed, Jan 05, 2005 at 04:46:54PM +1100, Rusty Russell wrote:
> On Tue, 2005-01-04 at 20:30 -0800, Phil Oester wrote:
> > On Wed, Jan 05, 2005 at 02:35:59PM +1100, Rusty Russell wrote:
> > > The multiport match doesn't support ranges of ports, so a new match
> > > called "mport" was written.  Now we have versioning of matches and
> > > targets, we can simply put this extension in multiport revision 1.
> > 
> > While I agree the above is a useful change, why not also add inversion
> > to multiport in the process -- as long as you're making a new revision?
> > Or should that be done in revision 2?
> 
> If you put it in before 2.6.11 is released, it can go in the current
> revision, otherwise we want a new one (where do we put the invert
> flags?).

OK, how about the below which adds inversion?

(note: didn't update manpage, but then again, wasn't updated with previous
changes)

Phil



[-- Attachment #2: patch-mport-ipt --]
[-- Type: text/plain, Size: 1738 bytes --]

diff -ru iptables-orig/extensions/libipt_multiport.c iptables-new/extensions/libipt_multiport.c
--- iptables-orig/extensions/libipt_multiport.c	2005-01-03 04:51:58.000000000 -0500
+++ iptables-new/extensions/libipt_multiport.c	2005-01-07 20:08:07.000000000 -0500
@@ -31,13 +31,13 @@
 {
 	printf(
 "multiport v%s options:\n"
-" --source-ports port[,port:port,port...]\n"
+" --source-ports [!] port[,port:port,port...]\n"
 " --sports ...\n"
 "				match source port(s)\n"
-" --destination-ports port[,port:port,port...]\n"
+" --destination-ports [!] port[,port:port,port...]\n"
 " --dports ...\n"
 "				match destination port(s)\n"
-" --ports port[,port:port,port]\n"
+" --ports [!] port[,port:port,port]\n"
 "				match both source and destination port(s)\n",
 IPTABLES_VERSION);
 }
@@ -255,8 +255,7 @@
 	}
 
 	if (invert)
-		exit_error(PARAMETER_PROBLEM,
-			   "multiport does not support invert");
+		multiinfo->invert = 1;
 
 	if (*flags)
 		exit_error(PARAMETER_PROBLEM,
@@ -362,6 +361,9 @@
 		break;
 	}
 
+	if (multiinfo->invert)
+		printf("! ");
+
 	for (i=0; i < multiinfo->count; i++) {
 		printf("%s", i ? "," : "");
 		print_port(multiinfo->ports[i], ip->proto, numeric);
diff -ru iptables-orig/include/linux/netfilter_ipv4/ipt_multiport.h iptables-new/include/linux/netfilter_ipv4/ipt_multiport.h
--- iptables-orig/include/linux/netfilter_ipv4/ipt_multiport.h	2005-01-03 04:37:07.000000000 -0500
+++ iptables-new/include/linux/netfilter_ipv4/ipt_multiport.h	2005-01-06 20:37:38.000000000 -0500
@@ -24,5 +24,6 @@
 	u_int8_t count;				/* Number of ports */
 	u_int16_t ports[IPT_MULTI_PORTS];	/* Ports */
 	u_int8_t pflags[IPT_MULTI_PORTS];	/* Port flags */
+	u_int8_t invert;			/* Invert flag */
 };
 #endif /*_IPT_MULTIPORT_H*/

[-- Attachment #3: patch-mport-kern --]
[-- Type: text/plain, Size: 1774 bytes --]

diff -ru linux-orig/include/linux/netfilter_ipv4/ipt_multiport.h linux-mport/include/linux/netfilter_ipv4/ipt_multiport.h
--- linux-orig/include/linux/netfilter_ipv4/ipt_multiport.h	2005-01-07 20:51:06.293435976 -0500
+++ linux-mport/include/linux/netfilter_ipv4/ipt_multiport.h	2005-01-06 19:55:28.000000000 -0500
@@ -25,5 +25,6 @@
 	u_int8_t count;				/* Number of ports */
 	u_int16_t ports[IPT_MULTI_PORTS];	/* Ports */
 	u_int8_t pflags[IPT_MULTI_PORTS];	/* Port flags */
+	u_int8_t invert;			/* Invert flag */
 };
 #endif /*_IPT_MULTIPORT_H*/
diff -ru linux-orig/net/ipv4/netfilter/ipt_multiport.c linux-mport/net/ipv4/netfilter/ipt_multiport.c
--- linux-orig/net/ipv4/netfilter/ipt_multiport.c	2005-01-07 20:51:06.404419104 -0500
+++ linux-mport/net/ipv4/netfilter/ipt_multiport.c	2005-01-07 20:53:23.468582184 -0500
@@ -64,30 +64,31 @@
 
 			if (minfo->flags == IPT_MULTIPORT_SOURCE
 			    && src >= s && src <= e)
-				return 1;
+				return 1 ^ minfo->invert;
 			if (minfo->flags == IPT_MULTIPORT_DESTINATION
 			    && dst >= s && dst <= e)
-				return 1;
+				return 1 ^ minfo->invert;
 			if (minfo->flags == IPT_MULTIPORT_EITHER
 			    && ((dst >= s && dst <= e)
 				|| (src >= s && src <= e)))
-				return 1;
+				return 1 ^ minfo->invert;
 		} else {
 			/* exact port matching */
 			duprintf("src or dst matches with %d?\n", s);
+
 			if (minfo->flags == IPT_MULTIPORT_SOURCE
 			    && src == s)
-				return 1;
+				return 1 ^ minfo->invert;
 			if (minfo->flags == IPT_MULTIPORT_DESTINATION
 			    && dst == s)
-				return 1;
+				return 1 ^ minfo->invert;
 			if (minfo->flags == IPT_MULTIPORT_EITHER
 			    && (src == s || dst == s))
-				return 1;
+				return 1 ^ minfo->invert;
 		}
 	}
  
- 	return 0;
+ 	return minfo->invert;
 }
 
 static int

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Herve Eychenne]

On Fri, Jan 07, 2005 at 06:03:22PM -0800, Phil Oester wrote:

> On Wed, Jan 05, 2005 at 04:46:54PM +1100, Rusty Russell wrote:

> > > While I agree the above is a useful change, why not also add inversion
> > > to multiport in the process -- as long as you're making a new revision?
> > > Or should that be done in revision 2?

> > If you put it in before 2.6.11 is released, it can go in the current
> > revision, otherwise we want a new one (where do we put the invert
> > flags?).

> OK, how about the below which adds inversion?

> (note: didn't update manpage, but then again, wasn't updated with previous
> changes)

Maybe because no one stated what the manpage policy regarding the
new revision system would be?...
When adding every successive revision documentation of every
match/target to the manpage, wouldn't it become severly bloated in the
end?

 Herve

-- 
 _
(°=  Hervé Eychenne
//)
v_/_ WallFire project:  http://www.wallfire.org/

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Pablo Neira]

bonjour,

Herve Eychenne wrote:

>>(note: didn't update manpage, but then again, wasn't updated with previous
>>changes)
>>    
>>
>
>Maybe because no one stated what the manpage policy regarding the
>new revision system would be?...
>When adding every successive revision documentation of every
>match/target to the manpage, wouldn't it become severly bloated in the
>end?
>  
>

Hm, I think that the revision infrastructure gives the possibility of 
extending a target/match with some kind of interesting features, 
something that we couldn't do so far. However, I don't encourage the 
release of a new revision every month or something like that.

So, I think that people must have a good reason to extend a 
target/match, and in case that this new revision is considered worthy. I 
would hold it for quite some time in the SVN repository and after that 
push it forward.

If we follow a policy similar to this, the manpages shouldn't get 
bloated so much I guess.

--
Pablo

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Rusty Russell]

On Sun, 2005-01-09 at 23:34 +0100, Pablo Neira wrote:
> bonjour,
> 
> Herve Eychenne wrote:
> 
> >>(note: didn't update manpage, but then again, wasn't updated with previous
> >>changes)
> >>    
> >>
> >
> >Maybe because no one stated what the manpage policy regarding the
> >new revision system would be?...
> >When adding every successive revision documentation of every
> >match/target to the manpage, wouldn't it become severly bloated in the
> >end?

You should add the new feature to the manpage.  The extension should
then fail with an appropriate message if the version of the kernel does
not support the option used.

eg. libipt_MARK.c's "parse_v0" accepts --and-mark (introduced in rev1)
like so:

	case '2':
		exit_error(PARAMETER_PROBLEM,
			   "MARK target: kernel too old for --and-mark");

Cheers,
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [PATCH 13/18] Netfilter: Multiport revision with port ranges (replaces "mport")

[Herve Eychenne]

On Mon, Jan 10, 2005 at 01:24:11PM +1100, Rusty Russell wrote:

> On Sun, 2005-01-09 at 23:34 +0100, Pablo Neira wrote:
> > bonjour,
> > 
> > Herve Eychenne wrote:
> > 
> > >>(note: didn't update manpage, but then again, wasn't updated with previous
> > >>changes)
> > >>    
> > >>
> > >
> > >Maybe because no one stated what the manpage policy regarding the
> > >new revision system would be?...
> > >When adding every successive revision documentation of every
> > >match/target to the manpage, wouldn't it become severly bloated in the
> > >end?

> You should add the new feature to the manpage.

Yes, let's just hope there won't be too many revisions over time,
because that would certainly confuse the users.  So, we shouln't add
revisions too often as Pablo said, or at least think before doing so.

> The extension should
> then fail with an appropriate message if the version of the kernel does
> not support the option used.

Yes, and add a mecanism in iptables command enabling to know in advance
(before actually trying) if a given revision will work (for third party
software), as well as the list of usable revisions (for third
party again) _and_ available (whether they would actually
work with current kernel or not) revisions (for the user, this time),
ideally.

 Herve

-- 
 _
(°=  Hervé Eychenne
//)
v_/_ WallFire project:  http://www.wallfire.org/