Skip to contentSkip to conversation

IPVS: replace sprintf to snprintf to avoid stack buffer overflow

PATCH In review [email protected] 4 participants 7 messages Apr 6, 2010 – Apr 8, 2010

Subsystems (4)
Networking Network drivers netfilter IPVS
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 IPVS: replace sprintf to snprintf to avoid stack buffer overflow

IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as:
struct ip_vs_protocol ip_vs_test = {
.name = "aaaaaaaa....128...aaa",
.debug_packet = ip_vs_tcpudp_debug_packet,
};

when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow.

Signed-off-by Zhitong Wang <[email protected]>
---
 net/netfilter/ipvs/ip_vs_proto.c        |   16 ++++++++--------
 net/netfilter/ipvs/ip_vs_proto_ah_esp.c |    8 ++++----
 2 files changed, 12 insertions(+), 12 deletions(-)
net/netfilter/ipvs/ip_vs_proto.c +8 -8
index 0e58455..8143318 100644
@@ -166,9 +166,9 @@ ip_vs_tcpudp_debug_packet_v4(struct ip_vs_protocol *pp,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name);
 	else if (ih->frag_off & htons(IP_OFFSET))
-		sprintf(buf, "%s %pI4->%pI4 frag",
+		snprintf(buf, sizeof(buf), "%s %pI4->%pI4 frag",
 			pp->name, &ih->saddr, &ih->daddr);
 	else {
 		__be16 _ports[2], *pptr

@@ -176,10 +176,10 @@ ip_vs_tcpudp_debug_packet_v4(struct ip_vs_protocol *pp,
 		pptr = skb_header_pointer(skb, offset + ih->ihl*4,
 					  sizeof(_ports), _ports);
 		if (pptr == NULL)
-			sprintf(buf, "%s TRUNCATED %pI4->%pI4",
+			snprintf(buf, sizeof(buf), "%s TRUNCATED %pI4->%pI4",
 				pp->name, &ih->saddr, &ih->daddr);
 		else
-			sprintf(buf, "%s %pI4:%u->%pI4:%u",
+			snprintf(buf, sizeof(buf), "%s %pI4:%u->%pI4:%u",
 				pp->name,
 				&ih->saddr, ntohs(pptr[0]),
 				&ih->daddr, ntohs(pptr[1]));

@@ -200,9 +200,9 @@ ip_vs_tcpudp_debug_packet_v6(struct ip_vs_protocol *pp,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name);
 	else if (ih->nexthdr == IPPROTO_FRAGMENT)
-		sprintf(buf, "%s %pI6->%pI6 frag",
+		snprintf(buf, sizeof(buf), "%s %pI6->%pI6 frag",
 			pp->name, &ih->saddr, &ih->daddr);
 	else {
 		__be16 _ports[2], *pptr;

@@ -210,10 +210,10 @@ ip_vs_tcpudp_debug_packet_v6(struct ip_vs_protocol *pp,
 		pptr = skb_header_pointer(skb, offset + sizeof(struct ipv6hdr),
 					  sizeof(_ports), _ports);
 		if (pptr == NULL)
-			sprintf(buf, "%s TRUNCATED %pI6->%pI6",
+			snprintf(buf, sizeof(buf), "%s TRUNCATED %pI6->%pI6",
 				pp->name, &ih->saddr, &ih->daddr);
 		else
-			sprintf(buf, "%s %pI6:%u->%pI6:%u",
+			snprintf(buf, sizeof(buf), "%s %pI6:%u->%pI6:%u",
 				pp->name,
 				&ih->saddr, ntohs(pptr[0]),
 				&ih->daddr, ntohs(pptr[1]));
net/netfilter/ipvs/ip_vs_proto_ah_esp.c +4 -4
index c30b43c..ce795ab 100644
@@ -136,9 +136,9 @@ ah_esp_debug_packet_v4(struct ip_vs_protocol *pp, const struct sk_buff *skb,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name);
 	else
-		sprintf(buf, "%s %pI4->%pI4",
+		snprintf(buf, sizeof(buf), "%s %pI4->%pI4",
 			pp->name, &ih->saddr, &ih->daddr);
 
 	pr_debug("%s: %s\n", msg, buf);

@@ -154,9 +154,9 @@ ah_esp_debug_packet_v6(struct ip_vs_protocol *pp, const struct sk_buff *skb,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		snprintf(buf, sizeof(buf), "%s TRUNCATED", pp->name);
 	else
-		sprintf(buf, "%s %pI6->%pI6",
+		snprintf(buf, sizeof(buf), "%s %pI6->%pI6",
 			pp->name, &ih->saddr, &ih->daddr);
 
 	pr_debug("%s: %s\n", msg, buf);
Signature · 1 line
1.6.5.3
#1 lore raw
Quoted 10 lines · On Tue, Apr 6, 2010 at 10:50 AM, <[email protected]> wrote:

IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as:
struct ip_vs_protocol ip_vs_test = {
       .name =                 "aaaaaaaa....128...aaa",
       .debug_packet =         ip_vs_tcpudp_debug_packet,
};

when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow.

Long messages will be truncated instead of buffer overflow. We need to find a way to handle long messages elegantly.

Signature · 2 lines
Regards，
Changli Gao([email protected])
maintainer
#2 lore raw
Quoted 8 lines · Changli Gao
Quoted 7 lines · On Tue, Apr 6, 2010 at 10:50 AM, <[email protected]> wrote:

IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as:
struct ip_vs_protocol ip_vs_test = {
       .name =                 "aaaaaaaa....128...aaa",
       .debug_packet =         ip_vs_tcpudp_debug_packet,
};

Quoted 6 lines
Quoted 3 lines

when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow.

Long messages will be truncated instead of buffer overflow. We need to find a way to handle long messages elegantly.

Its really a corner case. In practice protocol modules don't have really long names. And if one was merged that did, the buffer size could be increased at that time.

So while I think its reasonable to protect against something unexpected in a protocol-module name crashing the system. Especially as that can be achieved without any real overhead. I don't think we need to sanitise the output.

maintainer Acked-by on v1 1/1
#3 lore raw
In reply to [email protected]
Quoted 11 lines · On Tue, Apr 06, 2010 at 10:50:20AM +0800, [email protected] wrote:

IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as:
struct ip_vs_protocol ip_vs_test = {
.name = "aaaaaaaa....128...aaa",
.debug_packet = ip_vs_tcpudp_debug_packet,
};

when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow.

Signed-off-by Zhitong Wang <[email protected]>

I think that the simple answer is, don't do that.
But your patch seems entirely reasonable to me.

Acked-by Simon Horman <[email protected]>

Patrick, please consider merging this.

#4 lore raw
Quoted 8 lines · Simon Horman
Quoted 7 lines · On Tue, Apr 06, 2010 at 10:50:20AM +0800, [email protected] wrote:

IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as:
struct ip_vs_protocol ip_vs_test = {
.name = "aaaaaaaa....128...aaa",
.debug_packet = ip_vs_tcpudp_debug_packet,
};

Quoted 6 lines
Quoted 4 lines

when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow.

Signed-off-by Zhitong Wang <[email protected]>

I think that the simple answer is, don't do that.

Indeed.

Quoted 5 lines

But your patch seems entirely reasonable to me.

Acked-by Simon Horman <[email protected]>

Patrick, please consider merging this.

I think this fix is a bit silly, we can simply print the name in the pr_debug() statement and avoid both the potential overflow and truncation.

How does this look?

net/netfilter/ipvs/ip_vs_proto.c +12 -16
index 0e58455..27add97 100644
@@ -166,26 +166,24 @@ ip_vs_tcpudp_debug_packet_v4(struct ip_vs_protocol *pp,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		sprintf(buf, "TRUNCATED");
 	else if (ih->frag_off & htons(IP_OFFSET))
-		sprintf(buf, "%s %pI4->%pI4 frag",
-			pp->name, &ih->saddr, &ih->daddr);
+		sprintf(buf, "%pI4->%pI4 frag", &ih->saddr, &ih->daddr);
 	else {
 		__be16 _ports[2], *pptr
 ;
 		pptr = skb_header_pointer(skb, offset + ih->ihl*4,
 					  sizeof(_ports), _ports);
 		if (pptr == NULL)
-			sprintf(buf, "%s TRUNCATED %pI4->%pI4",
-				pp->name, &ih->saddr, &ih->daddr);
+			sprintf(buf, "TRUNCATED %pI4->%pI4",
+				&ih->saddr, &ih->daddr);
 		else
-			sprintf(buf, "%s %pI4:%u->%pI4:%u",
-				pp->name,
+			sprintf(buf, "%pI4:%u->%pI4:%u",
 				&ih->saddr, ntohs(pptr[0]),
 				&ih->daddr, ntohs(pptr[1]));
 	}
 
-	pr_debug("%s: %s\n", msg, buf);
+	pr_debug("%s: %s %s\n", msg, pp->name, buf);
 }
 
 #ifdef CONFIG_IP_VS_IPV6

@@ -200,26 +198,24 @@ ip_vs_tcpudp_debug_packet_v6(struct ip_vs_protocol *pp,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		sprintf(buf, "TRUNCATED");
 	else if (ih->nexthdr == IPPROTO_FRAGMENT)
-		sprintf(buf, "%s %pI6->%pI6 frag",
-			pp->name, &ih->saddr, &ih->daddr);
+		sprintf(buf, "%pI6->%pI6 frag",	&ih->saddr, &ih->daddr);
 	else {
 		__be16 _ports[2], *pptr;
 
 		pptr = skb_header_pointer(skb, offset + sizeof(struct ipv6hdr),
 					  sizeof(_ports), _ports);
 		if (pptr == NULL)
-			sprintf(buf, "%s TRUNCATED %pI6->%pI6",
-				pp->name, &ih->saddr, &ih->daddr);
+			sprintf(buf, "TRUNCATED %pI6->%pI6",
+				&ih->saddr, &ih->daddr);
 		else
-			sprintf(buf, "%s %pI6:%u->%pI6:%u",
-				pp->name,
+			sprintf(buf, "%pI6:%u->%pI6:%u",
 				&ih->saddr, ntohs(pptr[0]),
 				&ih->daddr, ntohs(pptr[1]));
 	}
 
-	pr_debug("%s: %s\n", msg, buf);
+	pr_debug("%s: %s %s\n", msg, pp->name, buf);
 }
 #endif
net/netfilter/ipvs/ip_vs_proto_ah_esp.c +6 -8
index c30b43c..1892dfc 100644
@@ -136,12 +136,11 @@ ah_esp_debug_packet_v4(struct ip_vs_protocol *pp, const struct sk_buff *skb,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		sprintf(buf, "TRUNCATED");
 	else
-		sprintf(buf, "%s %pI4->%pI4",
-			pp->name, &ih->saddr, &ih->daddr);
+		sprintf(buf, "%pI4->%pI4", &ih->saddr, &ih->daddr);
 
-	pr_debug("%s: %s\n", msg, buf);
+	pr_debug("%s: %s %s\n", msg, pp->name, buf);
 }
 
 #ifdef CONFIG_IP_VS_IPV6

@@ -154,12 +153,11 @@ ah_esp_debug_packet_v6(struct ip_vs_protocol *pp, const struct sk_buff *skb,
 
 	ih = skb_header_pointer(skb, offset, sizeof(_iph), &_iph);
 	if (ih == NULL)
-		sprintf(buf, "%s TRUNCATED", pp->name);
+		sprintf(buf, "TRUNCATED");
 	else
-		sprintf(buf, "%s %pI6->%pI6",
-			pp->name, &ih->saddr, &ih->daddr);
+		sprintf(buf, "%pI6->%pI6", &ih->saddr, &ih->daddr);
 
-	pr_debug("%s: %s\n", msg, buf);
+	pr_debug("%s: %s %s\n", msg, pp->name, buf);
 }
 #endif
maintainer Acked-by
#5 lore raw
Quoted 22 lines · Patrick McHardy
Quoted 8 lines · Simon Horman
Quoted 7 lines · On Tue, Apr 06, 2010 at 10:50:20AM +0800, [email protected] wrote:

IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as:
struct ip_vs_protocol ip_vs_test = {
.name = "aaaaaaaa....128...aaa",
.debug_packet = ip_vs_tcpudp_debug_packet,
};

Quoted 6 lines
Quoted 4 lines

when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow.

Signed-off-by Zhitong Wang <[email protected]>

I think that the simple answer is, don't do that.

Indeed.

Quoted 4 lines

But your patch seems entirely reasonable to me.

Acked-by Simon Horman <[email protected]>
Quoted 7 lines
Quoted 1 line

Patrick, please consider merging this.

I think this fix is a bit silly, we can simply print the name in the pr_debug() statement and avoid both the potential overflow and truncation.

How does this look?

Looks good to me:

Acked-by Simon Horman <[email protected]>
#6 lore raw
Quoted 26 lines · Simon Horman
Quoted 25 lines · Patrick McHardy
Quoted 8 lines · Simon Horman
Quoted 7 lines · On Tue, Apr 06, 2010 at 10:50:20AM +0800, [email protected] wrote:

IPVS not check the length of pp->name, use sprintf will cause stack buffer overflow. struct ip_vs_protocol{} declare name as char *, if register a protocol as:
struct ip_vs_protocol ip_vs_test = {
.name = "aaaaaaaa....128...aaa",
.debug_packet = ip_vs_tcpudp_debug_packet,
};

Quoted 5 lines
Quoted 4 lines

when called ip_vs_tcpudp_debug_packet(), sprintf(buf, "%s TRUNCATED", pp->name); will cause stack buffer overflow.

Signed-off-by Zhitong Wang <[email protected]>

I think that the simple answer is, don't do that.

Indeed.

Quoted 5 lines

But your patch seems entirely reasonable to me.

Acked-by Simon Horman <[email protected]>

Patrick, please consider merging this.

I think this fix is a bit silly, we can simply print the name in the pr_debug() statement and avoid both the potential overflow and truncation.

Quoted 5 lines
Quoted 1 line

How does this look?

Looks good to me:

Acked-by Simon Horman <[email protected]>

Thanks, I've applied the patch.