* [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
@ 2010-04-06 2:50 wzt.wzt
2010-04-06 2:58 ` Changli Gao
2010-04-06 3:22 ` Simon Horman
0 siblings, 2 replies; 8+ messages in thread
From: wzt.wzt @ 2010-04-06 2:50 UTC (permalink / raw)
To: linux-kernel; +Cc: wensong, netdev, lvs-devel
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, "%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]));
diff --git a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
index c30b43c..ce795ab 100644
--- a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
@@ -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);
--
1.6.5.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
2010-04-06 2:50 [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow wzt.wzt
@ 2010-04-06 2:58 ` Changli Gao
2010-04-06 3:22 ` Simon Horman
1 sibling, 0 replies; 8+ messages in thread
From: Changli Gao @ 2010-04-06 2:58 UTC (permalink / raw)
To: wzt.wzt; +Cc: linux-kernel, wensong, netdev, lvs-devel
On Tue, Apr 6, 2010 at 10:50 AM, <wzt.wzt@gmail.com> 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.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
@ 2010-04-06 2:58 ` Changli Gao
0 siblings, 0 replies; 8+ messages in thread
From: Changli Gao @ 2010-04-06 2:58 UTC (permalink / raw)
To: wzt.wzt; +Cc: linux-kernel, wensong, netdev, lvs-devel
On Tue, Apr 6, 2010 at 10:50 AM, <wzt.wzt@gmail.com> 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.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
2010-04-06 2:50 [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow wzt.wzt
2010-04-06 2:58 ` Changli Gao
@ 2010-04-06 3:22 ` Simon Horman
2010-04-07 16:09 ` Patrick McHardy
1 sibling, 1 reply; 8+ messages in thread
From: Simon Horman @ 2010-04-06 3:22 UTC (permalink / raw)
To: wzt.wzt
Cc: linux-kernel, Wensong Zhang, Julian Anastasov, netdev, lvs-devel,
Patrick McHardy
On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com 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 <zhitong.wangzt@alibaba-inc.com>
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
2010-04-06 2:58 ` Changli Gao
(?)
@ 2010-04-06 3:26 ` Simon Horman
-1 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2010-04-06 3:26 UTC (permalink / raw)
To: Changli Gao
Cc: wzt.wzt, linux-kernel, wensong, Julian Anastasov, netdev,
lvs-devel, Patrick McHardy
On Tue, Apr 06, 2010 at 10:58:28AM +0800, Changli Gao wrote:
> On Tue, Apr 6, 2010 at 10:50 AM, <wzt.wzt@gmail.com> 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.
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
2010-04-06 3:22 ` Simon Horman
@ 2010-04-07 16:09 ` Patrick McHardy
2010-04-07 22:34 ` Simon Horman
0 siblings, 1 reply; 8+ messages in thread
From: Patrick McHardy @ 2010-04-07 16:09 UTC (permalink / raw)
To: Simon Horman
Cc: wzt.wzt, linux-kernel, Wensong Zhang, Julian Anastasov, netdev,
lvs-devel
[-- Attachment #1: Type: text/plain, Size: 968 bytes --]
Simon Horman wrote:
> On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com 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 <zhitong.wangzt@alibaba-inc.com>
>
> I think that the simple answer is, don't do that.
Indeed.
> But your patch seems entirely reasonable to me.
>
> Acked-by: Simon Horman <horms@verge.net.au>
>
> 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?
[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3385 bytes --]
diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c
index 0e58455..27add97 100644
--- a/net/netfilter/ipvs/ip_vs_proto.c
+++ b/net/netfilter/ipvs/ip_vs_proto.c
@@ -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
diff --git a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
index c30b43c..1892dfc 100644
--- a/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_ah_esp.c
@@ -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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
2010-04-07 16:09 ` Patrick McHardy
@ 2010-04-07 22:34 ` Simon Horman
2010-04-08 11:37 ` Patrick McHardy
0 siblings, 1 reply; 8+ messages in thread
From: Simon Horman @ 2010-04-07 22:34 UTC (permalink / raw)
To: Patrick McHardy
Cc: wzt.wzt, linux-kernel, Wensong Zhang, Julian Anastasov, netdev,
lvs-devel
On Wed, Apr 07, 2010 at 06:09:54PM +0200, Patrick McHardy wrote:
> Simon Horman wrote:
> > On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com 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 <zhitong.wangzt@alibaba-inc.com>
> >
> > I think that the simple answer is, don't do that.
>
> Indeed.
>
> > But your patch seems entirely reasonable to me.
> >
> > Acked-by: Simon Horman <horms@verge.net.au>
> >
> > 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 <horms@verge.net.au>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow
2010-04-07 22:34 ` Simon Horman
@ 2010-04-08 11:37 ` Patrick McHardy
0 siblings, 0 replies; 8+ messages in thread
From: Patrick McHardy @ 2010-04-08 11:37 UTC (permalink / raw)
To: Simon Horman
Cc: wzt.wzt, linux-kernel, Wensong Zhang, Julian Anastasov, netdev,
lvs-devel
Simon Horman wrote:
> On Wed, Apr 07, 2010 at 06:09:54PM +0200, Patrick McHardy wrote:
>> Simon Horman wrote:
>>> On Tue, Apr 06, 2010 at 10:50:20AM +0800, wzt.wzt@gmail.com 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 <zhitong.wangzt@alibaba-inc.com>
>>> I think that the simple answer is, don't do that.
>> Indeed.
>>
>>> But your patch seems entirely reasonable to me.
>>>
>>> Acked-by: Simon Horman <horms@verge.net.au>
>>>
>>> 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 <horms@verge.net.au>
Thanks, I've applied the patch.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-04-08 11:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-06 2:50 [PATCH] IPVS: replace sprintf to snprintf to avoid stack buffer overflow wzt.wzt
2010-04-06 2:58 ` Changli Gao
2010-04-06 2:58 ` Changli Gao
2010-04-06 3:26 ` Simon Horman
2010-04-06 3:22 ` Simon Horman
2010-04-07 16:09 ` Patrick McHardy
2010-04-07 22:34 ` Simon Horman
2010-04-08 11:37 ` Patrick McHardy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.