* [PATCH V2] netfilter: h323: avoid potential attack
@ 2016-01-28 9:34 Zhouyi Zhou
0 siblings, 0 replies; 6+ messages in thread
From: Zhouyi Zhou @ 2016-01-28 9:34 UTC (permalink / raw)
To: linux-kernel; +Cc: Zhouyi Zhou, Zhouyi Zhou
Thanks Eric for your review and advice.
I think hackers chould build a malicious h323 packet to overflow
the pointer p which will panic during the memcpy(addr, p, len)
For example, he may fabricate a very large taddr->ipAddress.ip;
Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
---
net/netfilter/nf_conntrack_h323_main.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..ccd08c5 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,7 @@ int (*nat_q931_hook) (struct sk_buff *skb,
static DEFINE_SPINLOCK(nf_h323_lock);
static char *h323_buffer;
+#define CHECK_BOUND(p, n) ((void *)p + n - (void *)h323_buffer > 65536)
static struct nf_conntrack_helper nf_conntrack_helper_h245;
static struct nf_conntrack_helper nf_conntrack_helper_q931[];
@@ -247,6 +248,9 @@ static int get_h245_addr(struct nf_conn *ct, const unsigned char *data,
return 0;
}
+ if (CHECK_BOUND(p, len + sizeof(__be16)))
+ return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
@@ -669,6 +673,9 @@ int get_h225_addr(struct nf_conn *ct, unsigned char *data,
return 0;
}
+ if (CHECK_BOUND(p, len + sizeof(__be16)))
+ return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] netfilter: h323: avoid potential attack
2016-01-28 8:59 Zhouyi Zhou
2016-01-28 12:57 ` Eric Dumazet
2016-01-28 14:11 ` Sergei Shtylyov
@ 2016-01-28 14:29 ` Florian Westphal
2 siblings, 0 replies; 6+ messages in thread
From: Florian Westphal @ 2016-01-28 14:29 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: eric.dumazet, pablo, kaber, kadlec, davem, netfilter-devel,
coreteam, netdev, linux-kernel, Zhouyi Zhou
Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> Thanks Eric for your review and advice.
>
> I think hackers chould build a malicious h323 packet to overflow
> the pointer p which will panic during the memcpy(addr, p, len)
>
> For example, he may fabricate a very large taddr->ipAddress.ip;
Can you be more specific?
h323_buffer is backend storage for skb_header_pointer, i.e.
this will error out early when we ask for more data than is available in
packet.
I don't understand how this could overflow anything.
Even assuming 64k packet we'd still have enough room in h323_buffer
for an ipv6 address, no? (we skip the l3/l4 header when extracting
packet payload).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] netfilter: h323: avoid potential attack
2016-01-28 14:00 ` Eric Dumazet
@ 2016-01-28 14:15 ` One Thousand Gnomes
0 siblings, 0 replies; 6+ messages in thread
From: One Thousand Gnomes @ 2016-01-28 14:15 UTC (permalink / raw)
To: Eric Dumazet
Cc: Zhouyi Zhou, Zhouyi Zhou, pablo, kaber, kadlec, davem,
netfilter-devel, coreteam, netdev, linux-kernel
On Thu, 28 Jan 2016 06:00:50 -0800
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2016-01-28 at 21:14 +0800, Zhouyi Zhou wrote:
>
> > My patch is intend to prevent kernel panic, to prevent reading garbage
> > or read data from a prior frame and leak secrets, the prototypes of the
> > get_h2x5_addr functions and the functions that call get_h2x5_addr should
> > be changed, should we do this?
>
> In term of security, panics are better than allowing attacker to read
> data from other people, like a password.
>
> BTW, are you able to trigger any panic ?
We have a smattering of panics in this code in bugzilla going back years.
https://bugzilla.kernel.org/show_bug.cgi?id=12473
etc
Alan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] netfilter: h323: avoid potential attack
2016-01-28 8:59 Zhouyi Zhou
2016-01-28 12:57 ` Eric Dumazet
@ 2016-01-28 14:11 ` Sergei Shtylyov
2016-01-28 14:29 ` Florian Westphal
2 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2016-01-28 14:11 UTC (permalink / raw)
To: Zhouyi Zhou, eric.dumazet, pablo, kaber, kadlec, davem,
netfilter-devel, coreteam, netdev, linux-kernel
Cc: Zhouyi Zhou
Hello.
On 1/28/2016 11:59 AM, Zhouyi Zhou wrote:
> Thanks Eric for your review and advice.
>
> I think hackers chould build a malicious h323 packet to overflow
> the pointer p which will panic during the memcpy(addr, p, len)
>
> For example, he may fabricate a very large taddr->ipAddress.ip;
>
> Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
> ---
> net/netfilter/nf_conntrack_h323_main.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
> index 9511af0..ccd08c5 100644
> --- a/net/netfilter/nf_conntrack_h323_main.c
> +++ b/net/netfilter/nf_conntrack_h323_main.c
> @@ -110,6 +110,7 @@ int (*nat_q931_hook) (struct sk_buff *skb,
>
> static DEFINE_SPINLOCK(nf_h323_lock);
> static char *h323_buffer;
> +#define CHECK_BOUND(p, n) ((void *)p + n - (void *)h323_buffer > 65536)
You have to enclose the macro parameters in parens when used in expression.
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] netfilter: h323: avoid potential attack
2016-01-28 8:59 Zhouyi Zhou
@ 2016-01-28 12:57 ` Eric Dumazet
2016-01-28 13:14 ` Zhouyi Zhou
2016-01-28 14:11 ` Sergei Shtylyov
2016-01-28 14:29 ` Florian Westphal
2 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2016-01-28 12:57 UTC (permalink / raw)
To: Zhouyi Zhou
Cc: pablo, kaber, kadlec, davem, netfilter-devel, coreteam, netdev,
linux-kernel, Zhouyi Zhou
On Thu, 2016-01-28 at 16:59 +0800, Zhouyi Zhou wrote:
> Thanks Eric for your review and advice.
>
> I think hackers chould build a malicious h323 packet to overflow
> the pointer p which will panic during the memcpy(addr, p, len)
>
> For example, he may fabricate a very large taddr->ipAddress.ip;
>
> Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
> ---
Except you did not address my feedback about potentially reading not
initialized memory.
if the frame length was 1000 bytes, then surely accessing memory at
offset 8000 will either read garbage, or read data from a prior frame
and leak secrets.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2] netfilter: h323: avoid potential attack
@ 2016-01-28 8:59 Zhouyi Zhou
2016-01-28 12:57 ` Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Zhouyi Zhou @ 2016-01-28 8:59 UTC (permalink / raw)
To: eric.dumazet, pablo, kaber, kadlec, davem, netfilter-devel,
coreteam, netdev, linux-kernel
Cc: Zhouyi Zhou, Zhouyi Zhou
Thanks Eric for your review and advice.
I think hackers chould build a malicious h323 packet to overflow
the pointer p which will panic during the memcpy(addr, p, len)
For example, he may fabricate a very large taddr->ipAddress.ip;
Signed-off-by: Zhouyi Zhou <yizhouzhou@ict.ac.cn>
---
net/netfilter/nf_conntrack_h323_main.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/netfilter/nf_conntrack_h323_main.c b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..ccd08c5 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,7 @@ int (*nat_q931_hook) (struct sk_buff *skb,
static DEFINE_SPINLOCK(nf_h323_lock);
static char *h323_buffer;
+#define CHECK_BOUND(p, n) ((void *)p + n - (void *)h323_buffer > 65536)
static struct nf_conntrack_helper nf_conntrack_helper_h245;
static struct nf_conntrack_helper nf_conntrack_helper_q931[];
@@ -247,6 +248,9 @@ static int get_h245_addr(struct nf_conn *ct, const unsigned char *data,
return 0;
}
+ if (CHECK_BOUND(p, len + sizeof(__be16)))
+ return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
@@ -669,6 +673,9 @@ int get_h225_addr(struct nf_conn *ct, unsigned char *data,
return 0;
}
+ if (CHECK_BOUND(p, len + sizeof(__be16)))
+ return 0;
+
memcpy(addr, p, len);
memset((void *)addr + len, 0, sizeof(*addr) - len);
memcpy(port, p + len, sizeof(__be16));
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-28 14:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-28 9:34 [PATCH V2] netfilter: h323: avoid potential attack Zhouyi Zhou
-- strict thread matches above, loose matches on Subject: below --
2016-01-28 8:59 Zhouyi Zhou
2016-01-28 12:57 ` Eric Dumazet
2016-01-28 13:14 ` Zhouyi Zhou
2016-01-28 14:00 ` Eric Dumazet
2016-01-28 14:15 ` One Thousand Gnomes
2016-01-28 14:11 ` Sergei Shtylyov
2016-01-28 14:29 ` Florian Westphal
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.