[arp] possible DoS, fixes and improvements

[Chuck Swiger]

Hi, Rozhuk--

On Dec 7, 2010, at 11:19 AM, rozhuk.im at gmail.com wrote:
> Hi!
> 
> 1. ah->ar_hln - is depend from ar_hrd?
> Yes, and for ARPHRD_ETHER is 6 (ETHER_ADDR_LEN)
> For ARPHRD_IEEE1394 - sizeof(struct fw_hwaddr)
> ah->ar_hln ignored in ether_output: bcopy(ar_tha(ah), edst, ETHER_ADDR_LEN);

If you know that ar_hrd is ARPHRD_ETHER, then you can either assume the length is ETHER_ADDR_LEN, or optionally check it, per RFC 826:

"When an address resolution packet is received, the receiving
Ethernet module gives the packet to the Address Resolution module
which goes through an algorithm similar to the following.
Negative conditionals indicate an end of processing and a
discarding of the packet.

?Do I have the hardware type in ar$hrd?
Yes: (almost definitely)
  [optionally check the hardware length ar$hln]
  ?Do I speak the protocol in ar$pro?
  Yes:
    [optionally check the protocol length ar$pln]"

> check in in_arpinput:
> 		if (ifp->if_addrlen != ah->ar_hln) {
> 			LLE_WUNLOCK(la);
> 			log(LOG_WARNING,
> 			    "arp from %*D: addr len: new %d, i/f %d (ignored)",
> 			    ifp->if_addrlen, (u_char *) ar_sha(ah), ":",
> 			    ah->ar_hln, ifp->if_addrlen);
> 			goto reply;
> 		}
> NO DROP!!!!

I wonder which version of netinet/if_ether.c you are working from?
In 7-STABLE sources, it breaks rather than going to generate a reply:

                        if (ifp->if_addrlen != ah->ar_hln) {
                                log(LOG_WARNING,
                                    "arp from %*D: addr len: "
                                    "new %d, i/f %d (ignored)",
                                    ifp->if_addrlen, (u_char *) ar_sha(ah),
                                    ":", ah->ar_hln, ifp->if_addrlen);
                                RT_UNLOCK(rt);
                                break;
                        }

> In reply we get:
> 		(void)memcpy(ar_tha(ah), ar_sha(ah), ah->ar_hln);
> 		(void)memcpy(ar_sha(ah), enaddr, ah->ar_hln);
> Or 
> 			(void)memcpy(ar_tha(ah), ar_sha(ah), ah->ar_hln);
> 			(void)memcpy(ar_sha(ah), &lle->ll_addr, ah->ar_hln);
> 
> How to use it see below.
> 
> 
> 2. ah->ar_pln - does not checked!
> We can make big arp request (512 nulls after struct arphdr + 2*6 + 2*4) ,
> valid for host, set ar_plt = 255
> And in reply will receive part of stack or core panic:
> in_arpinput:
> (void)memcpy(ar_spa(ah), &itaddr, ah->ar_pln);
> ...
> m->m_len = sizeof(*ah) + (2 * ah->ar_pln) + (2 * ah->ar_hln);
> ( eq arphdr_len(ah) )

I think I agree that this is not being checked for properly....

Regards,
-- 
-Chuck