Skip to contentSkip to conversation

Add sanity checkings for ICMP

PATCH Applied Pablo Neira Ayuso 3 participants 3 messages Dec 5, 2005 – Dec 15, 2005 1 message not in the archive

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Pablo Neira Ayuso

author maintainer
#0 lore raw

PATCH 2/8 Add sanity checkings for ICMP

Signature · 3 lines
-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Add proper checkings to avoid possible malformed ICMP conntracks. And return to userspace -EINVAL in case of error.

Signed-off-by Pablo Neira Ayuso <[email protected]>
net/ipv4/netfilter/ip_conntrack_proto_icmp.c +5 -1
Index: netfilter-2.6.14.git/net/ipv4/netfilter/ip_conntrack_proto_icmp.c
===================================================================
@@ -288,10 +288,14 @@ nfattr_failure:
 static int icmp_nfattr_to_tuple(struct nfattr *tb[],
 				struct ip_conntrack_tuple *tuple)
 {
+	if (tuple->dst.u.icmp.type >= sizeof(valid_new)
+	    || !valid_new[tuple->dst.u.icmp.type])
+		return -EINVAL;
+
 	if (!tb[CTA_PROTO_ICMP_TYPE-1]
 	    || !tb[CTA_PROTO_ICMP_CODE-1]
 	    || !tb[CTA_PROTO_ICMP_ID-1])
-		return -1;
+		return -EINVAL;
 
 	tuple->dst.u.icmp.type = 
 			*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_ICMP_TYPE-1]);
#1 lore raw

Hi, Pablo,

From: Pablo Neira Ayuso <[email protected]>
Date: Mon, 05 Dec 2005 12:20:59 +0100

Quoted 15 lines

Add proper checkings to avoid possible malformed ICMP conntracks. And return to userspace -EINVAL in case of error.

Signed-off-by Pablo Neira Ayuso <[email protected]>

Index: netfilter-2.6.14.git/net/ipv4/netfilter/ip_conntrack_proto_icmp.c
===================================================================

net/ipv4/netfilter/ip_conntrack_proto_icmp.c +2 -0
@@ -288,10 +288,14 @@ nfattr_failure:
 static int icmp_nfattr_to_tuple(struct nfattr *tb[],
 				struct ip_conntrack_tuple *tuple)
 {
+	if (tuple->dst.u.icmp.type >= sizeof(valid_new)
+	    || !valid_new[tuple->dst.u.icmp.type])

net/ipv4/netfilter/ip_conntrack_proto_icmp.c nfattr_failure: · line 301

Quoted 10 lines
net/ipv4/netfilter/ip_conntrack_proto_icmp.c +3 -1
+		return -EINVAL;
+
 	if (!tb[CTA_PROTO_ICMP_TYPE-1]
 	    || !tb[CTA_PROTO_ICMP_CODE-1]
 	    || !tb[CTA_PROTO_ICMP_ID-1])
-		return -1;
+		return -EINVAL;
 
 	tuple->dst.u.icmp.type = 
 			*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_ICMP_TYPE-1]);

This checks ICMP type in tuple before putting it.
And it's better to use invmap instead of valid_new to allow Echo reply and so on.

How about attached patch instead ?

# Please note that attached patch doesn't include your 1st patch. # You can just replace your 2nd patch with it.

Regards,

-- Yasuyuki Kozakai

[NETFILTER] ipctnetlink: Add sanity checkings for ICMP

Signed-off-by Yasuyuki Kozakai <[email protected]>

---
commit 2cae73ad22d11072b7787b70f5d38f443d110491
tree c6e05f2f2407f4c21ea48b990816bfce50c209e4
parent b01d2778bc7ca3049fed4eb4b216e063faa52477
author Yasuyuki Kozakai <[email protected]> Fri, 09 Dec 2005 02:01:49 +0900 committer Yasuyuki Kozakai <[email protected]> Fri, 09 Dec 2005 02:01:49 +0900

 net/ipv4/netfilter/ip_conntrack_proto_icmp.c |   43 +++++++++++++++-----------
 1 files changed, 24 insertions(+), 19 deletions(-)
net/ipv4/netfilter/ip_conntrack_proto_icmp.c +24 -19
index 19cc550..30fc21d 100644
@@ -47,20 +47,21 @@ static int icmp_pkt_to_tuple(const struc
 	return 1;
 }
 
+/* Add 1; spaces filled with 0. */
+static const u_int8_t invmap[] = {
+	[ICMP_ECHO] = ICMP_ECHOREPLY + 1,
+	[ICMP_ECHOREPLY] = ICMP_ECHO + 1,
+	[ICMP_TIMESTAMP] = ICMP_TIMESTAMPREPLY + 1,
+	[ICMP_TIMESTAMPREPLY] = ICMP_TIMESTAMP + 1,
+	[ICMP_INFO_REQUEST] = ICMP_INFO_REPLY + 1,
+	[ICMP_INFO_REPLY] = ICMP_INFO_REQUEST + 1,
+	[ICMP_ADDRESS] = ICMP_ADDRESSREPLY + 1,
+	[ICMP_ADDRESSREPLY] = ICMP_ADDRESS + 1
+};
+
 static int icmp_invert_tuple(struct ip_conntrack_tuple *tuple,
 			     const struct ip_conntrack_tuple *orig)
 {
-	/* Add 1; spaces filled with 0. */
-	static const u_int8_t invmap[]
-		= { [ICMP_ECHO] = ICMP_ECHOREPLY + 1,
-		    [ICMP_ECHOREPLY] = ICMP_ECHO + 1,
-		    [ICMP_TIMESTAMP] = ICMP_TIMESTAMPREPLY + 1,
-		    [ICMP_TIMESTAMPREPLY] = ICMP_TIMESTAMP + 1,
-		    [ICMP_INFO_REQUEST] = ICMP_INFO_REPLY + 1,
-		    [ICMP_INFO_REPLY] = ICMP_INFO_REQUEST + 1,
-		    [ICMP_ADDRESS] = ICMP_ADDRESSREPLY + 1,
-		    [ICMP_ADDRESSREPLY] = ICMP_ADDRESS + 1};
-
 	if (orig->dst.u.icmp.type >= sizeof(invmap)
 	    || !invmap[orig->dst.u.icmp.type])
 		return 0;

@@ -110,17 +111,17 @@ static int icmp_packet(struct ip_conntra
 	return NF_ACCEPT;
 }
 
-static const u_int8_t valid_new[] = { 
-	[ICMP_ECHO] = 1,
-	[ICMP_TIMESTAMP] = 1,
-	[ICMP_INFO_REQUEST] = 1,
-	[ICMP_ADDRESS] = 1 
-};
-
 /* Called when a new connection for this protocol found. */
 static int icmp_new(struct ip_conntrack *conntrack,
 		    const struct sk_buff *skb)
 {
+	static const u_int8_t valid_new[] = { 
+		[ICMP_ECHO] = 1,
+		[ICMP_TIMESTAMP] = 1,
+		[ICMP_INFO_REQUEST] = 1,
+		[ICMP_ADDRESS] = 1 
+	};
+
 	if (conntrack->tuplehash[0].tuple.dst.u.icmp.type >= sizeof(valid_new)
 	    || !valid_new[conntrack->tuplehash[0].tuple.dst.u.icmp.type]) {
 		/* Can't create a new ICMP `conn' with this. */

@@ -291,7 +292,7 @@ static int icmp_nfattr_to_tuple(struct n
 	if (!tb[CTA_PROTO_ICMP_TYPE-1]
 	    || !tb[CTA_PROTO_ICMP_CODE-1]
 	    || !tb[CTA_PROTO_ICMP_ID-1])
-		return -1;
+		return -EINVAL;
 
 	tuple->dst.u.icmp.type = 
 			*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_ICMP_TYPE-1]);

@@ -300,6 +301,10 @@ static int icmp_nfattr_to_tuple(struct n
 	tuple->src.u.icmp.id =
 			*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_ICMP_ID-1]);
 
+	if (tuple->dst.u.icmp.type >= sizeof(invmap)
+	    || !invmap[tuple->dst.u.icmp.type])
+		return -EINVAL;
+
 	return 0;
 }
 #endif

A message that is not in the archive (sent off-list or not yet received): [email protected]

Applied
#3 lore raw
Quoted 7 lines · Yasuyuki KOZAKAI

This checks ICMP type in tuple before putting it.
And it's better to use invmap instead of valid_new to allow Echo reply and so on.

How about attached patch instead ?

[NETFILTER] ipctnetlink: Add sanity checkings for ICMP

Applied to my 2.6.16 queue, thanks.