[PATCH] xt_request_find_match

[PATCH] xt_request_find_match

[Jan Engelhardt]

Hi,



Reusing code is a good idea, and I would like to do so from my 
match modules. netfilter already provides a xt_request_find_target() but 
an xt_request_find_match() does not yet exist. This patch adds it.

Objections welcome :)

---

Signed-off-by: Jan Engelhardt <jengelh@gmx.de>

Index: linux-2.6.20-rc1/net/netfilter/x_tables.c
===================================================================
--- linux-2.6.20-rc1.orig/net/netfilter/x_tables.c
+++ linux-2.6.20-rc1/net/netfilter/x_tables.c
@@ -206,6 +206,19 @@ struct xt_match *xt_find_match(int af, c
 }
 EXPORT_SYMBOL(xt_find_match);
 
+struct xt_match *xt_request_find_match(int af, const char *name, u8 revision)
+{
+	struct xt_match *match;
+
+	match = try_then_request_module(xt_find_match(af, name, revision),
+	                                "%st_%s", xt_prefix[af], name);
+	if(IS_ERR(match) || match == NULL)
+		return NULL;
+
+	return match;
+}
+EXPORT_SYMBOL_GPL(xt_request_find_match);
+
 /* Find target, grabs ref.  Returns ERR_PTR() on error. */
 struct xt_target *xt_find_target(int af, const char *name, u8 revision)
 {
#<EOF>

	-`J'
-- 

Re: [PATCH] xt_request_find_match

[Patrick McHardy]

Jan Engelhardt wrote:
> Reusing code is a good idea, and I would like to do so from my 
> match modules. netfilter already provides a xt_request_find_target() but 
> an xt_request_find_match() does not yet exist. This patch adds it.

Why does your match module needs to lookup other matches?

Re: [PATCH] xt_request_find_match

[Jan Engelhardt]

On Dec 19 2006 12:51, Patrick McHardy wrote:
>> Reusing code is a good idea, and I would like to do so from my 
>> match modules. netfilter already provides a xt_request_find_target() but 
>> an xt_request_find_match() does not yet exist. This patch adds it.
>
>Why does your match module needs to lookup other matches?

To use them?

I did not want to write


some_xt_target() {
    if(skb->nh.iph->protocol == IPPROTO_TCP)
        do_this();
    else
        do_that();
}

since the xt_tcpudp module provides far more checks than just the protocol
(TCP/UDP), like

    /* To quote Alan:

       Don't allow a fragment of TCP 8 bytes in. Nobody normal
       causes this. Its a cracker trying to break in by doing a
       flag overwrite to pass the direction checks.
    */

(see xt_tcpudp.c)



	-`J'
-- 

Re: [PATCH] xt_request_find_match

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 19 2006 12:51, Patrick McHardy wrote:
> 
>>>Reusing code is a good idea, and I would like to do so from my 
>>>match modules. netfilter already provides a xt_request_find_target() but 
>>>an xt_request_find_match() does not yet exist. This patch adds it.
>>
>>Why does your match module needs to lookup other matches?
> 
> 
> To use them?
> 
> I did not want to write
> 
> 
> some_xt_target() {
>     if(skb->nh.iph->protocol == IPPROTO_TCP)
>         do_this();
>     else
>         do_that();
> }

I don't think

xt_request_find_match(match->family, "tcp", 0)->match(lots of arguments)

is better than a simple comparison. Besides that the tcp match itself
expects that the protocol match already checked for IPPROTO_TCP, so
you'd still have to do it.

> since the xt_tcpudp module provides far more checks than just the protocol
> (TCP/UDP), like
> 
>     /* To quote Alan:
> 
>        Don't allow a fragment of TCP 8 bytes in. Nobody normal
>        causes this. Its a cracker trying to break in by doing a
>        flag overwrite to pass the direction checks.
>     */

This check makes sure the flags are not overwritten _after you
matched on them_. It doesn't matter at all if you're only
interested in the protocol since the user didn't tell you to care.

Re: [PATCH] xt_request_find_match

[Jan Engelhardt]

>>>>Reusing code is a good idea, and I would like to do so from my 
>>>>match modules. netfilter already provides a xt_request_find_target() but 
>>>>an xt_request_find_match() does not yet exist. This patch adds it.
>>>
>>>Why does your match module needs to lookup other matches?
>> 
>> To use them?
>> 
>> I did not want to write
>> 
>> some_xt_target() {
>>     if(skb->nh.iph->protocol == IPPROTO_TCP)
>>         do_this();
>>     else
>>         do_that();
>> }
>
>I don't think
>
>xt_request_find_match(match->family, "tcp", 0)->match(lots of arguments)
>
>is better than a simple comparison. Besides that the tcp match itself
>expects that the protocol match already checked for IPPROTO_TCP, so
>you'd still have to do it.
>>     /* To quote Alan:
>> 
>>        Don't allow a fragment of TCP 8 bytes in. Nobody normal
>>        causes this. Its a cracker trying to break in by doing a
>>        flag overwrite to pass the direction checks.
>>     */
>
>This check makes sure the flags are not overwritten _after you
>matched on them_. It doesn't matter at all if you're only
>interested in the protocol since the user didn't tell you to care.

Ok, but let's say I wanted to use a bigger match module (layer7, anyone?)
Then it's just not if(protocol == IPPROTO_TCP). What's the preferred solution
then?


	-`J'
-- 

Re: [PATCH] xt_request_find_match

[Patrick McHardy]

Jan Engelhardt wrote:
> [...]
>
> Ok, but let's say I wanted to use a bigger match module (layer7, anyone?)
> Then it's just not if(protocol == IPPROTO_TCP). What's the preferred solution
> then?

Make sure the user specifies the match on the command line before
your match. Look at the TCPMSS or REJECT targets for examples for
this.

Re: [PATCH] xt_request_find_match

[Jan Engelhardt]

>Jan Engelhardt wrote:
>> [...]
>>
>> Ok, but let's say I wanted to use a bigger match module (layer7, anyone?)
>> Then it's just not if(protocol == IPPROTO_TCP). What's the preferred solution
>> then?
>
>Make sure the user specifies the match on the command line before
>your match. Look at the TCPMSS or REJECT targets for examples for
>this.

That would mean I'd have to

  -p tcp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
lotsofothers -j TARGET
  -p udp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
lotsofothers -j TARGET

which can become quite computationally expensive - which I wanted to 
avoid.


	-`J'
-- 

Re: [PATCH] xt_request_find_match

[Patrick McHardy]

Jan Engelhardt wrote:
>>Make sure the user specifies the match on the command line before
>>your match. Look at the TCPMSS or REJECT targets for examples for
>>this.
> 
> 
> That would mean I'd have to
> 
>   -p tcp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
> lotsofothers -j TARGET
>   -p udp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
> lotsofothers -j TARGET

I don't see any match that would depend on an other match in
your example. How about your start explaining what you would
like to do, ideally with some code.

Re: xt_request_find_match

[Jan Engelhardt]

On Dec 20 2006 10:17, Patrick McHardy wrote:
>Jan Engelhardt wrote:
>>>Make sure the user specifies the match on the command line before
>>>your match. Look at the TCPMSS or REJECT targets for examples for
>>>this.
>> 
>> That would mean I'd have to
>> 
>>   -p tcp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
>> lotsofothers -j TARGET
>>   -p udp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
>> lotsofothers -j TARGET
>
>I don't see any match that would depend on an other match in
>your example. How about your start explaining what you would
>like to do, ideally with some code.

Yup, on the spot!
http://jengelh.hopto.org/f/chaostables/chaostables-0.1.tar.bz2
(Contains a target, but still something that could use 
xt_request_find_module.)


	-`J'
-- 

Re: xt_request_find_match

[Patrick McHardy]

Jan Engelhardt wrote:
> On Dec 20 2006 10:17, Patrick McHardy wrote:
> 
>>Jan Engelhardt wrote:
>>
>>>>Make sure the user specifies the match on the command line before
>>>>your match. Look at the TCPMSS or REJECT targets for examples for
>>>>this.
>>>
>>>That would mean I'd have to
>>>
>>>  -p tcp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
>>>lotsofothers -j TARGET
>>>  -p udp -m multiport --dport 1,2,3,4 -m time --time sundays -m 
>>>lotsofothers -j TARGET
>>
>>I don't see any match that would depend on an other match in
>>your example. How about your start explaining what you would
>>like to do, ideally with some code.
> 
> 
> Yup, on the spot!
> http://jengelh.hopto.org/f/chaostables/chaostables-0.1.tar.bz2
> (Contains a target, but still something that could use 
> xt_request_find_module.)

That looks a bit silly, you combine matches and targets through
code instead of through the ruleset.

        /* Equivalent to
            -A chaos -m random --average 1 -j REJECT --reject-with
host-unreach
            -A chaos -p tcp -j TARPIT
            -A chaos -j DROP
        */

Just do that ..

Re: xt_request_find_match

[Jan Engelhardt]

On Jan 9 2007 11:44, Patrick McHardy wrote:
>> 
>> Yup, on the spot!
>> http://jengelh.hopto.org/f/chaostables/chaostables-0.1.tar.bz2
[-0.4.tar.bz2]
>> (Contains a target, but still something that could use 
>> xt_request_find_module.)
>
>That looks a bit silly, you combine matches and targets through
>code instead of through the ruleset.

xt_portscan consolidates multiple rules (at least 23 when done with 
rules only) into one. Instead of calling tcp_match() 23 times as part of 
-p tcp, it's done exactly once. By using ".proto = IPPROTO_TCP" in 
xt_portscan.c, the user is forced to specify -p tcp when doing `iptables 
-m portscan`. This means that xt_portscan_match() will always get a TCP 
packet, so we can save additional checks. Plus, we do not lose, for 
example, the offset check from tcp_match(), since -p tcp was specified.

Because of how 'portscan' works, the packet and connection mark 
inherently need to be changed - as part of a match, yes. (At least it's 
documented behavior!) I did not feel like adding another mark into 
struct sk_buff or struct ip_conntrack.

As far as xt_CHAOS goes, it is designed to work with all types of IP 
traffic, TCP, UDP, ICMP, and so on. (Read: I wanted it that way.) Doing 
just "if(ipproto == IPPROTO_TCP)" in xt_chaos_target() would leave out 
the offset check[*] from tcp_match(). To not duplicate code, xt_tcpudp 
is looked up and used.

[*] Is the offset check even meaningful?


I hope this makes the reason a bit clearer.
Thank you for your input.

Jan
--