net/icmpv6: Discard Neighbor Discovery packet if optlen is 0

[acassis]

Summary

This commit make ICMPv6 on NuttX compliant with RFC4861 ignoring a DN packet when its optlen is 0.

Impact

Make NuttX compliant with RFC 4861.

Testing

Using sim:nettest

$ sudo ./nuttx
...
nsh> ifup eth0
ifup eth0...OK
nsh> ifconfig
eth0	Link encap:Ethernet HWaddr 42:29:9f:00:77:89 at RUNNING mtu 1500
	inet addr:10.0.1.2 DRaddr:10.0.1.1 Mask:255.255.255.0
	inet6 addr: fc00::2/112
	inet6 DRaddr: fc00::1

	RX: Received Fragment Errors   Bytes   
	    0000001e 00000000 00000000 109d            
	    IPv4     IPv6     ARP      Dropped 
	    00000000 0000001e 00000000 00000000
	TX: Queued   Sent     Errors   Timeouts Bytes   
	    00000000 00000000 00000000 00000000 0                
	Total Errors: 00000000

lo	Link encap:Local Loopback at RUNNING mtu 1518
	inet addr:127.0.0.1 DRaddr:127.0.0.1 Mask:255.0.0.0
	inet6 addr: ::1/128
	inet6 DRaddr: ::1

	RX: Received Fragment Errors   Bytes   
	    00000000 00000000 00000000 0               
	    IPv4     IPv6     ARP      Dropped 
	    00000000 00000000 00000000 00000000
	TX: Queued   Sent     Errors   Timeouts Bytes   
	    00000000 00000000 00000000 00000000 0                
	Total Errors: 00000000

             IPv4  IPv6   TCP   UDP  ICMP ICMPv6
Received     0000  001e  0000  000f  0000  000f
Dropped      0000  0000  0000  0000  0000  000f
  IPv4        VHL: 0000   Frg: 0000
  IPv6        VHL: 0000
  Checksum   0000  ----  0000  0000  ----  ----
  TCP         ACK: 0000   SYN: 0000
              RST: 0000  0000
  Type       0000  0000  ----  ----  0000  000e
Sent         0000  0000  0000  0000  0000  0000
  Rexmit     ----  ----  0000  ----  ----  ----
nsh> 

In another Linux terminal:

$ sudo ip -6 addr add fc00::1/112 dev tap0
$ sudo ip link set tap0 up
$ ping6 -I tap0 -c3 fc00::2
``

[ankohuu]

• Do we need check in other ND messages?
• Do we need to verify that the generic option header is fully present before checking opt->optlen == 0? I mean the remaining bytes should be at least the generic option header size to keep the optlen is meaningful.
• If there are multiple options and the last one optlen == 0, we need to drop the whole packet, right? Now other options may take effect first. e.g. RA packets ICMPv6_OPT_SRCLLADDR + 0 len option

[acassis]

• Do we need check in other ND messages?

@ankohuu yes, others ND msg also should be checked according with RFC 4861, so ICMPv6_NEIGHBOR_SOLICIT and ICMPv6_NEIGHBOR_ADVERTISE also should be checked (I will send a new PR to fix). But differently from ICMPV6_ROUTER_ADVERTISE they check only a single option (so no loop). That loop was causing an issue that could stall the devices.

* Do we need to verify that the generic option header is fully present before checking `opt->optlen == 0`?  I mean the remaining bytes should be at least the generic option header size to keep the optlen is meaningful.

You are right, if ndx + sizeof(struct icmpv6_generic_s) > optlen then we will have an OOB, I will fix it as well. Thank you very much!

* If there are multiple options and the last one `optlen == 0`, we need to drop the whole packet, right? Now other options may take effect first.    e.g.  RA packets  `ICMPv6_OPT_SRCLLADDR` + 0 len option

Yes, unfortunately the RFC is ambiguous, it only said that I need to discard a packet with optlen 0. Fortunately my solution already fixes it because I do a "goto icmpv6_drop_packet" that will exit the entire loop.

[ankohuu]

Yes, unfortunately the RFC is ambiguous, it only said that I need to discard a packet with optlen 0. Fortunately my solution already fixes it because I do a "goto icmpv6_drop_packet" that will exit the entire loop.

Before drop, neighbor_add(dev, ipv6->srcipaddr, sllopt->srclladdr); the neighbor is added, but we don't need this effect.

[acassis]

Yes, unfortunately the RFC is ambiguous, it only said that I need to discard a packet with optlen 0. Fortunately my solution already fixes it because I do a "goto icmpv6_drop_packet" that will exit the entire loop.

Before drop, neighbor_add(dev, ipv6->srcipaddr, sllopt->srclladdr); the neighbor is added, but we don't need this effect.

Hmm, you are right, I think I should change the generic option header first, I will add this check:

    if (scan + sizeof(struct icmpv6_generic_s) > optlen)
      {
        goto icmpv6_drop_packet;
      }

Do you see any other issue?