Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

Ingo Molnar
2006-05-02T14:11:37Z

* Patrick McHardy <kaber@trash.net> wrote:

> I did a couple of minutes ago. Here it is again in case my last mail
> won't show up.

> - (sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)); \
> - offset += (htons(sch->length) + 3) & ~3, count++)
> + (sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)) && \
> + sch->length; offset += (htons(sch->length) + 3) & ~3, count++)

but this makes do_basic_checks() not fail, and the clearly bogus packet
is passed further down. The reason i have put it inside the loop is to
be able to return 1 for the early checks. How about the fix below? It
should be cleaner and it will also return 1 if the initial offset is
oversized.

Ingo

----
From: Ingo Molnar <mingo@elte.hu>

fix infinite loop in the SCTP-netfilter code: check SCTP chunk size to
guarantee progress of for_each_sctp_chunk(). (all other uses of
for_each_sctp_chunk() are preceded by do_basic_checks(), so this fix
should be complete.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>

---
net/ipv4/netfilter/ip_conntrack_proto_sctp.c | 16 +++++++++++++---
1 files changed, 13 insertions(+), 3 deletions(-)

Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
===================================================================
--- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -224,6 +224,13 @@ static int do_basic_checks(struct ip_con
DEBUGP(__FUNCTION__);
DEBUGP("\n");

+ /*
+ * Dont trust the initial offset:
+ */
+ offset = skb->nh.iph->ihl * 4 + sizeof(sctp_sctphdr_t);
+ if (offset >= skb->len)
+ return 1;
+
flag = 0;

for_each_sctp_chunk (skb, sch, _sch, offset, count) {
@@ -235,12 +242,15 @@ static int do_basic_checks(struct ip_con
flag = 1;
}

- /* Cookie Ack/Echo chunks not the first OR
- Init / Init Ack / Shutdown compl chunks not the only chunks */
+ /*
+ * Cookie Ack/Echo chunks not the first OR
+ * Init / Init Ack / Shutdown compl chunks not the only chunks
+ * OR zero-length.
+ */
if ((sch->type == SCTP_CID_COOKIE_ACK
|| sch->type == SCTP_CID_COOKIE_ECHO
|| flag)
- && count !=0 ) {
+ && count !=0 || !sched->length) {
DEBUGP("Basic checks failed\n");
return 1;
}