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

Thanks Pablo for reviewing

From: "Pablo Neira Ayuso" <pablo@netfilter.org>
Sent Time: Saturday, March 12, 2016
To: "Zhouyi Zhou" <redacted>

On Sun, Feb 21, 2016 at 12:03:59AM +0800, Zhouyi Zhou wrote:

quoted

I think hackers chould build a malicious h323 packet to overflow

(iph->ihl * 4 + th->doff * 4);

You cannot trust the information that is available in the header. If
this is bogus this check will be defeated. That's why we pass this
protoff parameters to each function.

The length of IP header is checked in the function nf_conntrack_in which calls
get_l4proto hook to detect bogus ip header. 
There is no where in the call stack to the function set_addr to check bogus
TCP header, and my code does the job:
+		th = (void *)iph + iph->ihl * 4;
+		datalen = skb->len - (iph->ihl * 4 + th->doff * 4);
+		/* check offset overflow */
+		if (addroff > datalen)
+			return  -1;
if th->doff be too big addroff will greater than datalen.

You also refer to get_h225_addr() in your description. That function
always copies 4 or 16 bytes, so I would appreciate if you can describe
the possible issue further.

The problem of get_h225_addr lies in bogus taddr->ipAddress.ip, if this value
is too big, it may make the pointer p point to no exist address.
(gdb) list 686
681                       struct h323_ct_state *ctstate)
682     {
683             const unsigned char *p;
684             int len;
685
686             switch (taddr->choice) {
687             case eTransportAddress_ipAddress:
688                     if (nf_ct_l3num(ct) != AF_INET)
689                             return 0;
690                     p = data + taddr->ipAddress.ip;

Thanks for your time and effort
Cheers
Zhouyi