Skip to contentSkip to conversation

netfilter: h323: avoid potential attack

PATCH Superseded Zhouyi Zhou 6 participants 9 messages Jan 28, 2016

Subsystems (3)
Networking Network drivers netfilter

v2 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Zhouyi Zhou

author
#0 lore raw

PATCH v2 1/1 netfilter: h323: avoid potential attack

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 <[email protected]>
---
 net/netfilter/nf_conntrack_h323_main.c |    7 +++++++
 1 file changed, 7 insertions(+)
net/netfilter/nf_conntrack_h323_main.c +7 -0
index 9511af0..ccd08c5 100644
@@ -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));
Signature · 1 line
1.7.10.4
#1 lore raw
Quoted 9 lines · 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 <[email protected]>

---

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.

#2 lore raw

Thanks Eric for replying

Quoted 18 lines

-----Original Messages-----
From: "Eric Dumazet" <[email protected]>
Sent Time: Thursday, January 28, 2016
To: "Zhouyi Zhou" <[email protected]>
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], "Zhouyi Zhou" <[email protected]>
Subject: Re: [PATCH V2] netfilter: h323: avoid potential attack

Quoted 9 lines · 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 <[email protected]>

---

Quoted 6 lines

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.

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?

Quoted 3 lines

Cheers
Zhouyi

#3 lore raw
Quoted 4 lines · Zhouyi Zhou

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 ?

I am not familiar with this code, it is not obvious.

If a fix is needed, better doing it right.

#4 lore raw
Quoted 7 lines · Eric Dumazet
Quoted 4 lines · Zhouyi Zhou

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?

Quoted 9 lines

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 ?

I am not familiar with this code, it is not obvious.

If a fix is needed, better doing it right.

BTW, this module is protected by a lock (nf_h323_lock)

So adding a variable like 'h323_buffer_valid_bytes' that would contain the number of valid bytes would not require to change prototypes.

This variable would be written when skb_header_pointer() is used in get_tpkt_data() / get_udp_data()

#5 lore raw
In reply to Eric Dumazet
Quoted 11 lines · On Thu, 28 Jan 2016 06:00:50 -0800 Eric Dumazet <[email protected]> wrote:
Quoted 4 lines · Zhouyi Zhou

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

#6 lore raw
In reply to Zhouyi Zhou

Hello.

Quoted 17 lines · 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 <[email protected]>
---
  net/netfilter/nf_conntrack_h323_main.c |    7 +++++++
  1 file changed, 7 insertions(+)
net/netfilter/nf_conntrack_h323_main.c +0 -0
index 9511af0..ccd08c5 100644

net/netfilter/nf_conntrack_h323_main.c int (*nat_q931_hook) (struct sk_buff *skb, · line 113

Quoted 4 lines
net/netfilter/nf_conntrack_h323_main.c +1 -0
@@ -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

maintainer
#7 lore raw
In reply to Zhouyi Zhou
Quoted 6 lines · 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;

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

#8 lore raw

Thanks for your advices.
I will take your advice if I could have the opportunity to write the final code.

As matter of factor, I trigger this bug when I tried to migrate H323 code to other operating systems.

This could trigger a panic because get_h2x5_addr functions is the first time we ever try to ask for potentially out of range data because of H323 style decodings (the value taddr->ipAddress.ip is decoded in the midway between calling skb_header_pointer and calling get_h2x5_addr)

Quoted 26 lines

-----Original Messages-----
From: "Florian Westphal" <[email protected]>
Sent Time: Thursday, January 28, 2016
To: "Zhouyi Zhou" <[email protected]>
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], "Zhouyi Zhou" <[email protected]>
Subject: Re: [PATCH V2] netfilter: h323: avoid potential attack

Quoted 6 lines · 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;

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