[PATCH] Orphaned expectations

[PATCH] Orphaned expectations

[Phil Oester]

In trying to track down the cause of the oops on read of /proc/net/ip_conntrack
I stumbled upon a larger issue which may be responsible for the livelocks I 
have seen on a few heavily used firewalls.

For reference, the relevant bug report is:

https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=131

The oops is actually caused by a mangled expect which has lost its helper. So
when print_expect hits this expect and tries to dereference like so:

	if (expect->expectant->helper->timeout)

The oops occurs.  Simply making this change prevents the oops:

	if (expect->expectant->helper && expect->expectant->helper->timeout)

But doesn't fix the underlying problem of how the expect lost its helper.

The problem is the TFTP helper can create orphaned expectations.  Consider the
following testcase:

on a:
  iptables -I INPUT -p udp --dport 69 -j DROP

on b:
  tftp -c get a:foo

The expect is setup by the tftp helper, but the master conntrack is not
confirmed.  Instead, the master is immediately sent to destroy_conntrack.
Whoops!  We've now got an orphaned expectation which will not go away (ever!).
This makes for a nice DOS btw, if it was known the TFTP helper was loaded one
could trivially fill up the conntrack table with expects.

Further, now that we have this orphaned expectation, we can Oops the kernel
without even reading /proc/net/ip_conntrack by:

on a:
  iptables -D INPUT 1

on b:
  tftp -c get a:foo   (assuming we get the same udp port as before)

This time, the kernel will oops in init_conntrack.

So we need to make sure that expectations don't get orphaned in this way, which
I believe the below patch will ensure.  Of course it will also stop the
oops on read of /proc/net/ip_conntrack.

Let me know if anyone sees problems with it.


Phil Oester




diff -ru linux-2.4.26/net/ipv4/netfilter/ip_conntrack_core.c linux-2.4.26-po/net/ipv4/netfilter/ip_conntrack_core.c
--- linux-2.4.26/net/ipv4/netfilter/ip_conntrack_core.c 2004-02-18 08:36:32.000000000 -0500
+++ linux-2.4.26-po/net/ipv4/netfilter/ip_conntrack_core.c      2004-04-17 15:27:50.884612424 -0400
@@ -326,6 +322,11 @@
                ip_conntrack_destroyed(ct);
 
        WRITE_LOCK(&ip_conntrack_lock);
+
+       /* Make sure we don't leave any orphaned expectations lying around */
+       if (ct->expecting)
+               remove_expectations(ct,1);
+
        /* Delete us from our own list to prevent corruption later */
        list_del(&ct->sibling_list);
 

Re: [PATCH] Orphaned expectations

[Chris Wilson]

Hi Phil,

> https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=131

Thanks for fixing that one, it's been bugging me for a while! 

Do you think that any other helpers might be vulnerable to the same 
problem?

Cheers, Chris.
-- 
_  __ __     _
 / __/ / ,__(_)_  | Chris Wilson -- UNIX Firewall Lead Developer |
/ (_  ,\/ _/ /_ \ | NetServers.co.uk http://www.netservers.co.uk |
\__/_/_/_//_/___/ | 21 Signet Court, Cambridge, UK. 01223 576516 |

Re: [PATCH] Orphaned expectations

[Phil Oester]

I don't think any other helper is vulnerable, since it would likely have to 
be a UDP helper for it to generate the expect in advance of conntrack
confirmation.

Phil

On Tue, Apr 20, 2004 at 12:51:38PM +0100, Chris Wilson wrote:
> Hi Phil,
> 
> > https://bugzilla.netfilter.org/cgi-bin/bugzilla/show_bug.cgi?id=131
> 
> Thanks for fixing that one, it's been bugging me for a while! 
> 
> Do you think that any other helpers might be vulnerable to the same 
> problem?

Re: [PATCH] Orphaned expectations

[Pablo Neira]

Hi,

Chris Wilson wrote:

>Do you think that any other helpers might be vulnerable to the same 
>problem?
>  
>

I think that only helpers for udp based protocols. So, I think that it 
could be also reproduced with the amanda helper in a weird scenario 
since this helper only looks for patterns in packets which comes from 
server to client.

Actually, when an udp packet hits the connection tracking system for 
first time, in the ip_conntrack_in function:

a) a new conntrack is created.
b) a helper is asigned to this conntrack.
c) and if this helper exists (case of tftp helper), it is also called.

So now, with only an udp packet, we have a conntrack which is not 
confirmed with an expectation associated, and as Phil pointed out, if 
this conntrack is destroyed the expectation keeps there forever.

regards,
Pablo

Re: [PATCH] Orphaned expectations

[Chris Wilson]

Hi Pablo,

> a) a new conntrack is created.
> b) a helper is asigned to this conntrack.
> c) and if this helper exists (case of tftp helper), it is also called.
> 
> So now, with only an udp packet, we have a conntrack which is not 
> confirmed with an expectation associated, and as Phil pointed out, if 
> this conntrack is destroyed the expectation keeps there forever.

Thanks very much for your explanation. So, to prevent this from happening, 
what is the best thing to do?

1. Audit all helpers to check that they don't create expectations to 
unconfirmed conntracks

2. Modify destroy_conntrack to destroy any associated expectations, 
whether or not the conntrack is confirmed (presumably right now it only 
does so if the conntrack is confirmed? hence the problem?)

Cheers, Chris.
-- 
_  __ __     _
 / __/ / ,__(_)_  | Chris Wilson -- UNIX Firewall Lead Developer |
/ (_  ,\/ _/ /_ \ | NetServers.co.uk http://www.netservers.co.uk |
\__/_/_/_//_/___/ | 21 Signet Court, Cambridge, UK. 01223 576516 |

Re: [PATCH] Orphaned expectations

[Pablo Neira]

Hi Chris,

Chris Wilson wrote:

>>So now, with only an udp packet, we have a conntrack which is not 
>>confirmed with an expectation associated, and as Phil pointed out, if 
>>this conntrack is destroyed the expectation keeps there forever.
>>    
>>
>
>Thanks very much for your explanation. So, to prevent this from happening, 
>what is the best thing to do?
>
>1. Audit all helpers to check that they don't create expectations to 
>unconfirmed conntracks
>  
>

I think that this assumption is wrong for udp based helpers. When 
connection tracking sees udp packet which has a helper associated for 
first time, a conntrack (not confirmed yet) and its respective 
expectation are created, and surely this conntrack will be confirmed later.

>2. Modify destroy_conntrack to destroy any associated expectations, 
>whether or not the conntrack is confirmed (presumably right now it only 
>does so if the conntrack is confirmed? hence the problem?)
>  
>

yes, I think that Phil's solution is ok, this could also prevent any 
other weird combinations.

regards,
Pablo

Re: [PATCH] Orphaned expectations

[Patrick McHardy]

Phil Oester wrote:
> So we need to make sure that expectations don't get orphaned in this way, which
> I believe the below patch will ensure.  Of course it will also stop the
> oops on read of /proc/net/ip_conntrack.

Thanks Phil, I've added this patch to CVS:

@@ -324,8 +324,9 @@
                 ip_conntrack_destroyed(ct);

         WRITE_LOCK(&ip_conntrack_lock);
-       /* Delete us from our own list to prevent corruption later */
-       list_del(&ct->sibling_list);
+       /* Make sure don't leave any orphaned expectations lying around */
+       if (ct->expecting)
+               remove_expectations(ct, 1);

         /* Delete our master expectation */
         if (ct->master) {

Regards,
Patrick