Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow

Previous thread: [PATCH 0/4]-V3 TPS6507x MFD driver by Todd Fischer on Monday, April 5, 2010 - 7:23 pm. (11 messages)

Next thread: [PATCH 0/3][v3] blkio: IO controller stats by Divyesh Shah on Monday, April 5, 2010 - 8:35 pm. (9 messages)
From: wzt.wzt
Date: Monday, April 5, 2010 - 7:50 pm

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 <zhitong.wangzt@alibaba-inc.com>

---
 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(-)

diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c
index 0e58455..8143318 100644
--- a/net/netfilter/ipvs/ip_vs_proto.c
+++ b/net/netfilter/ipvs/ip_vs_proto.c
@@ -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, ...
From: Changli Gao
Date: Monday, April 5, 2010 - 7:58 pm

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

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)
--

From: Simon Horman
Date: Monday, April 5, 2010 - 8:26 pm

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.
--

From: Simon Horman
Date: Monday, April 5, 2010 - 8:22 pm

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

Acked-by: Simon Horman <horms@verge.net.au>

Patrick, please consider merging this.

--

From: Patrick McHardy
Date: Wednesday, April 7, 2010 - 9:09 am

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?
From: Simon Horman
Date: Wednesday, April 7, 2010 - 3:34 pm

Looks good to me:

Acked-by: Simon Horman <horms@verge.net.au>

--

From: Patrick McHardy
Date: Thursday, April 8, 2010 - 4:37 am

Thanks, I've applied the patch.
--

Previous thread: [PATCH 0/4]-V3 TPS6507x MFD driver by Todd Fischer on Monday, April 5, 2010 - 7:23 pm. (11 messages)

Next thread: [PATCH 0/3][v3] blkio: IO controller stats by Divyesh Shah on Monday, April 5, 2010 - 8:35 pm. (9 messages)