Re: [PATCH V3] netfilter: h323: avoid potential attack

[Eric Dumazet]

On Mon, 2016-02-01 at 18:58 +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 29, 2016 at 11:25:35AM +0800, Zhouyi Zhou wrote:
> > 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;
> > As suggested by Eric, this module is protected by a lock (nf_h323_lock)
> > so adding a variable h323_buffer_valid_bytes that would contain
> > the number of valid bytes would not require to change prototypes of
> > get_h2x5_addr.
> > 
> > Signed-off-by: Zhouyi Zhou 
> > Signed-off-by: Eric Dumazet 
> > Reviewed-by: Sergei Shtylyov  
> > 
> > ---
> >  net/netfilter/nf_conntrack_h323_main.c | 13 +
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_conntrack_h323_main.c 
> > b/net/netfilter/nf_conntrack_h323_main.c
> > index 9511af0..65d84bc 100644
> > --- a/net/netfilter/nf_conntrack_h323_main.c
> > +++ b/net/netfilter/nf_conntrack_h323_main.c
> > @@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb,
> >  
> >  static DEFINE_SPINLOCK(nf_h323_lock);
> >  static char *h323_buffer;
> > +static unsigned int h323_buffer_valid_bytes;
> > +/* check offset overflow and out of range data reference */
> > +#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes ||
> > \
> > +  ((void *)(p) + (n) - (void *)h323_buffer \
> > +   > h323_buffer_valid_bytes))
> 
> We don't want obscure macros. You add a function for this, the
> compiler will likely inline it.

BTW, I did not signed-off this patch.

Zhouyi Zho, just add your own signature, let people add their own.

Thanks.

Re: [PATCH V3] netfilter: h323: avoid potential attack

[Pablo Neira Ayuso]

On Fri, Jan 29, 2016 at 11:25:35AM +0800, Zhouyi Zhou wrote:
> 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;
> As suggested by Eric, this module is protected by a lock (nf_h323_lock)
> so adding a variable h323_buffer_valid_bytes that would contain
> the number of valid bytes would not require to change prototypes of
> get_h2x5_addr.
> 
> Signed-off-by: Zhouyi Zhou 
> Signed-off-by: Eric Dumazet 
> Reviewed-by: Sergei Shtylyov  
> 
> ---
>  net/netfilter/nf_conntrack_h323_main.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_h323_main.c 
> b/net/netfilter/nf_conntrack_h323_main.c
> index 9511af0..65d84bc 100644
> --- a/net/netfilter/nf_conntrack_h323_main.c
> +++ b/net/netfilter/nf_conntrack_h323_main.c
> @@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb,
>  
>  static DEFINE_SPINLOCK(nf_h323_lock);
>  static char *h323_buffer;
> +static unsigned int h323_buffer_valid_bytes;
> +/* check offset overflow and out of range data reference */
> +#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes ||  \
> +((void *)(p) + (n) - (void *)h323_buffer \
> + > h323_buffer_valid_bytes))

We don't want obscure macros. You add a function for this, the
compiler will likely inline it.

Re: [PATCH V3] netfilter: h323: avoid potential attack

[Pablo Neira Ayuso]

On Fri, Jan 29, 2016 at 11:25:35AM +0800, Zhouyi Zhou wrote:
> 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;
> As suggested by Eric, this module is protected by a lock (nf_h323_lock)
> so adding a variable h323_buffer_valid_bytes that would contain
> the number of valid bytes would not require to change prototypes of
> get_h2x5_addr.
> 
> Signed-off-by: Zhouyi Zhou 
> Signed-off-by: Eric Dumazet 
> Reviewed-by: Sergei Shtylyov  
> 
> ---
>  net/netfilter/nf_conntrack_h323_main.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_h323_main.c 
> b/net/netfilter/nf_conntrack_h323_main.c
> index 9511af0..65d84bc 100644
> --- a/net/netfilter/nf_conntrack_h323_main.c
> +++ b/net/netfilter/nf_conntrack_h323_main.c
> @@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb,
>  
>  static DEFINE_SPINLOCK(nf_h323_lock);
>  static char *h323_buffer;
> +static unsigned int h323_buffer_valid_bytes;
> +/* check offset overflow and out of range data reference */
> +#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes ||  \
> +((void *)(p) + (n) - (void *)h323_buffer \
> + > h323_buffer_valid_bytes))

We don't want obscure macros. You add a function for this, the
compiler will likely inline it.

Re: [PATCH V3] netfilter: h323: avoid potential attack

[Eric Dumazet]

On Mon, 2016-02-01 at 18:58 +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 29, 2016 at 11:25:35AM +0800, Zhouyi Zhou wrote:
> > 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;
> > As suggested by Eric, this module is protected by a lock (nf_h323_lock)
> > so adding a variable h323_buffer_valid_bytes that would contain
> > the number of valid bytes would not require to change prototypes of
> > get_h2x5_addr.
> > 
> > Signed-off-by: Zhouyi Zhou 
> > Signed-off-by: Eric Dumazet 
> > Reviewed-by: Sergei Shtylyov  
> > 
> > ---
> >  net/netfilter/nf_conntrack_h323_main.c | 13 +
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_conntrack_h323_main.c 
> > b/net/netfilter/nf_conntrack_h323_main.c
> > index 9511af0..65d84bc 100644
> > --- a/net/netfilter/nf_conntrack_h323_main.c
> > +++ b/net/netfilter/nf_conntrack_h323_main.c
> > @@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb,
> >  
> >  static DEFINE_SPINLOCK(nf_h323_lock);
> >  static char *h323_buffer;
> > +static unsigned int h323_buffer_valid_bytes;
> > +/* check offset overflow and out of range data reference */
> > +#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes ||
> > \
> > +  ((void *)(p) + (n) - (void *)h323_buffer \
> > +   > h323_buffer_valid_bytes))
> 
> We don't want obscure macros. You add a function for this, the
> compiler will likely inline it.

BTW, I did not signed-off this patch.

Zhouyi Zho, just add your own signature, let people add their own.

Thanks.

[PATCH V3] netfilter: h323: avoid potential attack

[Zhouyi Zhou]

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;
As suggested by Eric, this module is protected by a lock (nf_h323_lock)
so adding a variable h323_buffer_valid_bytes that would contain
the number of valid bytes would not require to change prototypes of
get_h2x5_addr.

Signed-off-by: Zhouyi Zhou 
Signed-off-by: Eric Dumazet 
Reviewed-by: Sergei Shtylyov  

---
 net/netfilter/nf_conntrack_h323_main.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_main.c 
b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..65d84bc 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb,
 
 static DEFINE_SPINLOCK(nf_h323_lock);
 static char *h323_buffer;
+static unsigned int h323_buffer_valid_bytes;
+/* check offset overflow and out of range data reference */
+#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes ||\
+  ((void *)(p) + (n) - (void *)h323_buffer \
+   > h323_buffer_valid_bytes))
 
 static struct nf_conntrack_helper nf_conntrack_helper_h245;
 static struct nf_conntrack_helper nf_conntrack_helper_q931[];
@@ -145,6 +150,7 @@ static int get_tpkt_data(struct sk_buff *skb, unsigned int 
protoff,
 
if (*data == NULL) {/* first TPKT */
/* Get first TPKT pointer */
+   h323_buffer_valid_bytes = tcpdatalen;
tpkt = skb_header_pointer(skb, tcpdataoff, tcpdatalen,
  h323_buffer);
BUG_ON(tpkt == NULL);
@@ -247,6 +253,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 +678,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));
@@ -1248,6 +1260,7 @@ static unsigned char *get_udp_data(struct sk_buff *skb, 
unsigned int protoff,
if (dataoff >= skb->len)
return NULL;
*datalen = skb->len - dataoff;
+   h323_buffer_valid_bytes = *datalen;
return skb_header_pointer(skb, dataoff, *datalen, h323_buffer);
 }
 
-- 
1.9.1

[PATCH V3] netfilter: h323: avoid potential attack

[Zhouyi Zhou]

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;
As suggested by Eric, this module is protected by a lock (nf_h323_lock)
so adding a variable h323_buffer_valid_bytes that would contain
the number of valid bytes would not require to change prototypes of
get_h2x5_addr.

Signed-off-by: Zhouyi Zhou 
Signed-off-by: Eric Dumazet 
Reviewed-by: Sergei Shtylyov  

---
 net/netfilter/nf_conntrack_h323_main.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_main.c 
b/net/netfilter/nf_conntrack_h323_main.c
index 9511af0..65d84bc 100644
--- a/net/netfilter/nf_conntrack_h323_main.c
+++ b/net/netfilter/nf_conntrack_h323_main.c
@@ -110,6 +110,11 @@ int (*nat_q931_hook) (struct sk_buff *skb,
 
 static DEFINE_SPINLOCK(nf_h323_lock);
 static char *h323_buffer;
+static unsigned int h323_buffer_valid_bytes;
+/* check offset overflow and out of range data reference */
+#define CHECK_BOUND(p, n) ((n) > h323_buffer_valid_bytes ||\
+  ((void *)(p) + (n) - (void *)h323_buffer \
+   > h323_buffer_valid_bytes))
 
 static struct nf_conntrack_helper nf_conntrack_helper_h245;
 static struct nf_conntrack_helper nf_conntrack_helper_q931[];
@@ -145,6 +150,7 @@ static int get_tpkt_data(struct sk_buff *skb, unsigned int 
protoff,
 
if (*data == NULL) {/* first TPKT */
/* Get first TPKT pointer */
+   h323_buffer_valid_bytes = tcpdatalen;
tpkt = skb_header_pointer(skb, tcpdataoff, tcpdatalen,
  h323_buffer);
BUG_ON(tpkt == NULL);
@@ -247,6 +253,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 +678,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));
@@ -1248,6 +1260,7 @@ static unsigned char *get_udp_data(struct sk_buff *skb, 
unsigned int protoff,
if (dataoff >= skb->len)
return NULL;
*datalen = skb->len - dataoff;
+   h323_buffer_valid_bytes = *datalen;
return skb_header_pointer(skb, dataoff, *datalen, h323_buffer);
 }
 
-- 
1.9.1