Mailing List Archive

From: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>

> From: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>
>
> > From: Patrick McHardy <kaber@trash.net>
> > Date: Sat, 7 Jul 2007 17:34:49 +0200 (CEST)
> > >
> > > The local ICMP tracking is basically useless nowadays since we always
> > > manually attach the conntrack reference from the original packet
> > > (exactly because of the half-done double NAT FIXME quoted above).
> > > But this is an interesting case, the connection tracking code itself
> > > thought the RST was invalid, but ICMP tracking will still associate
> > > the ICMP containing the RST with the original connection. I'm wondering
> > > whether it should really do that. In case it shouldn't, just removing
> > > all locally generated ICMP special-casing should also fix the bug,
> > > right?
> >
> > At first I thought so. But I didn't come up with any bad situation caused
> > by returning ICMP error to such invalid packets.
>
> Can kernel correctly return ICMP error without conntrack in this case ?
> If so, I agree. (maybe yes, I'll check it after waking up).

I've tested following patch in some situations. Unless I used state or
conntrack match in OUTPUT (e.g. --state INVALID -j DROP), there is
no change of behavior. But can we accept this change ? ICMP error itself
is not invalid.


[NETFILTER]: nf_conntrack: Don't track locally generated special ICMP error

The conntrack assigned to locally generated ICMP error is usually the one
assigned to the original packet which has caused the error. But if
the original packet is handled as invalid by nf_conntrack, no conntrack
is assigned to the original packet. Then nf_ct_attach() cannot assign
any conntrack to the ICMP error packet. In that case the current
nf_conntrack_icmp assigns appropriate conntrack to it. But the current
code mistakes the direction of the packet. As a result, NAT code mistakes
the address to be mangled.

To fix the bug, this changes nf_conntrack_icmp not to assign conntrack
to such ICMP error. Actually no address is necessary to be mangled
in this case.

Spotted by Jordan Russell.

Signed-off-by: Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp>
---
net/ipv4/netfilter/nf_conntrack_proto_icmp.c | 22 +++++-----------------
1 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_proto_icmp.c b/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
index f4fc657..474b4ce 100644
--- a/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
+++ b/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
@@ -189,25 +189,13 @@ icmp_error_message(struct sk_buff *skb,

h = nf_conntrack_find_get(&innertuple, NULL);
if (!h) {
- /* Locally generated ICMPs will match inverted if they
- haven't been SNAT'ed yet */
- /* FIXME: NAT code has to handle half-done double NAT --RR */
- if (hooknum == NF_IP_LOCAL_OUT)
- h = nf_conntrack_find_get(&origtuple, NULL);
-
- if (!h) {
- DEBUGP("icmp_error_message: no match\n");
- return -NF_ACCEPT;
- }
-
- /* Reverse direction from that found */
- if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
- *ctinfo += IP_CT_IS_REPLY;
- } else {
- if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
- *ctinfo += IP_CT_IS_REPLY;
+ DEBUGP("icmp_error_message: no match\n");
+ return -NF_ACCEPT;
}

+ if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
+ *ctinfo += IP_CT_IS_REPLY;
+
/* Update skb to refer to this connection */
skb->nfct = &nf_ct_tuplehash_to_ctrack(h)->ct_general;
skb->nfctinfo = *ctinfo;
--
1.5.2.2

From: Jordan Russell <jr-list-2007@quo.to>

> Yasuyuki KOZAKAI wrote:
> > OK, I try to explain what the current netfilter does in Jordan's situation.
> > I also attach the patch to fix this problem. Jordan, can you try it ?
>
> Excellent. The patch does appear to fix the problem. I repeated the
> BitTorrent test, and no packets matched my LOG rule.
>
> I'll keep watching the logs and report back if it happens again.
>
> Thanks for looking into this!

Thanks for testing.

-- Yasuyuki Kozakai

Yasuyuki KOZAKAI wrote:
> From: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>
> Date: Sun, 08 Jul 2007 02:28:54 +0900 (JST)
>
>
>>From: Patrick McHardy <kaber@trash.net>
>>Date: Sat, 7 Jul 2007 17:34:49 +0200 (CEST)
>>
>>>The local ICMP tracking is basically useless nowadays since we always
>>>manually attach the conntrack reference from the original packet
>>>(exactly because of the half-done double NAT FIXME quoted above).
>>>But this is an interesting case, the connection tracking code itself
>>>thought the RST was invalid, but ICMP tracking will still associate
>>>the ICMP containing the RST with the original connection. I'm wondering
>>>whether it should really do that. In case it shouldn't, just removing
>>>all locally generated ICMP special-casing should also fix the bug,
>>>right?
>>
>>At first I thought so. But I didn't come up with any bad situation caused
>>by returning ICMP error to such invalid packets.
>
>
> Can kernel correctly return ICMP error without conntrack in this case ?
> If so, I agree. (maybe yes, I'll check it after waking up).


All locally generated ICMP errors are already associated with the
original conntrack and packets skip connection tracking. The
only locally generated ICMP packets that are handled by conntrack
are errors for INVALID packets.

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 09 Jul 2007 15:34:46 +0200

> Yasuyuki KOZAKAI wrote:
> > From: Yasuyuki KOZAKAI <yasuyuki.kozakai@toshiba.co.jp>
> > Date: Sun, 08 Jul 2007 02:28:54 +0900 (JST)
> >
> >
> >>From: Patrick McHardy <kaber@trash.net>
> >>Date: Sat, 7 Jul 2007 17:34:49 +0200 (CEST)
> >>
> >>>The local ICMP tracking is basically useless nowadays since we always
> >>>manually attach the conntrack reference from the original packet
> >>>(exactly because of the half-done double NAT FIXME quoted above).
> >>>But this is an interesting case, the connection tracking code itself
> >>>thought the RST was invalid, but ICMP tracking will still associate
> >>>the ICMP containing the RST with the original connection. I'm wondering
> >>>whether it should really do that. In case it shouldn't, just removing
> >>>all locally generated ICMP special-casing should also fix the bug,
> >>>right?
> >>
> >>At first I thought so. But I didn't come up with any bad situation caused
> >>by returning ICMP error to such invalid packets.
> >
> >
> > Can kernel correctly return ICMP error without conntrack in this case ?
> > If so, I agree. (maybe yes, I'll check it after waking up).
>
>
> All locally generated ICMP errors are already associated with the
> original conntrack and packets skip connection tracking. The
> only locally generated ICMP packets that are handled by conntrack
> are errors for INVALID packets.

Well, what I wanted to say was wether kernel can use correct address to
locally generated ICMP where NAT is used.
As I said in previous mail, I tested following patch in some
situations. Unless I used state or conntrack match in OUTPUT
(e.g. --state INVALID -j DROP), there is no change of behavior. But can we
accept this change ? ICMP error itself is not invalid.


[NETFILTER]: nf_conntrack: Don't track locally generated special ICMP error

The conntrack assigned to locally generated ICMP error is usually the one
assigned to the original packet which has caused the error. But if
the original packet is handled as invalid by nf_conntrack, no conntrack
is assigned to the original packet. Then nf_ct_attach() cannot assign
any conntrack to the ICMP error packet. In that case the current
nf_conntrack_icmp assigns appropriate conntrack to it. But the current
code mistakes the direction of the packet. As a result, NAT code mistakes
the address to be mangled.

To fix the bug, this changes nf_conntrack_icmp not to assign conntrack
to such ICMP error. Actually no address is necessary to be mangled
in this case.

Spotted by Jordan Russell.

Signed-off-by: Yasuyuki Kozakai <yasuyuki.kozakai@toshiba.co.jp>
---
net/ipv4/netfilter/nf_conntrack_proto_icmp.c | 22 +++++-----------------
1 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_proto_icmp.c b/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
index f4fc657..474b4ce 100644
--- a/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
+++ b/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
@@ -189,25 +189,13 @@ icmp_error_message(struct sk_buff *skb,

h = nf_conntrack_find_get(&innertuple, NULL);
if (!h) {
- /* Locally generated ICMPs will match inverted if they
- haven't been SNAT'ed yet */
- /* FIXME: NAT code has to handle half-done double NAT --RR */
- if (hooknum == NF_IP_LOCAL_OUT)
- h = nf_conntrack_find_get(&origtuple, NULL);
-
- if (!h) {
- DEBUGP("icmp_error_message: no match\n");
- return -NF_ACCEPT;
- }
-
- /* Reverse direction from that found */
- if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
- *ctinfo += IP_CT_IS_REPLY;
- } else {
- if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
- *ctinfo += IP_CT_IS_REPLY;
+ DEBUGP("icmp_error_message: no match\n");
+ return -NF_ACCEPT;
}

+ if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
+ *ctinfo += IP_CT_IS_REPLY;
+
/* Update skb to refer to this connection */
skb->nfct = &nf_ct_tuplehash_to_ctrack(h)->ct_general;
skb->nfctinfo = *ctinfo;
--
1.5.2.2

Yasuyuki KOZAKAI wrote:
> [NETFILTER]: nf_conntrack: Don't track locally generated special ICMP error


Thanks. I had to manually apply it because it clashed with the
pr_debug changes, please verify that I made no mistake. I'll
also push your original patch to -stable.

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 13 Jul 2007 16:50:02 +0200

> Yasuyuki KOZAKAI wrote:
> > [NETFILTER]: nf_conntrack: Don't track locally generated special ICMP error
>
>
> Thanks. I had to manually apply it because it clashed with the
> pr_debug changes, please verify that I made no mistake. I'll
> also push your original patch to -stable.

Sorry for inconvenience. I've checked it. Looks fine except of a Hunk.
Thanks.

-- Yasuyuki Kozakai