IPv6 packet.lua review

[David Fifield <david () bamsoftware com>]

This is a quick review of the changes Weilin has made to the packet.lua
library to support IPv6 packet construction. The file is in
/nmap-exp/weilin/host_discovery_scripts/packet.lua.

+--- Build an Ethernet frame.
+-- @param mac_dst six-byte string of the destination MAC address.
+-- @param mac_src six-byte string of the source MAC address.
+-- @param packet string of the payload.
+-- @return frame string of the Ether frame.
+function Frame:build_ether_frame(mac_dst, mac_src, packet)
+       self.mac_dst = mac_dst or self.mac_dst
+       self.mac_src = mac_src or self.mac_src
+       self.buf = packet or self.buf
+       local l3_type
+       if self.ip_v == 4 then
+               l3_type = string.char(0x08, 0x00)
+       elseif self.ip_v == 6 then
+               l3_type = string.char(0x86, 0xdd)
+       else
+               return nil, "Unknown packet."
+       end
+       self.frame_buf = self.mac_dst..self.mac_src..l3_type..self.buf
+end

How does self.ip_v get set? Why should IPv4 aznd IPv6 be given special
status here? Shouldn't the upper-layer protocol be a parameter?

 function Packet:new(packet, packet_len, force_continue)
        local o = setmetatable({}, {__index = Packet})
+       if not packet then
+               return o
+       end
        o.buf           = packet

Why is this handling for packet == nil now necessary?

+       self.buf =
+               set_u32("....",0,ver_tc_fl) ..
+               set_u16("..",0,#(self.exheader or "")+#(self.l4_packet or "")) ..--string.char(0x00,0x10) .. --payload 
length

Using set_u32 and set_u16 in this way is confusing. If necessary, define
another function that just returns the string you want, instead of
forcing the caller to provide a dummy string of the proper length.

+function Packet:build_invalid_extension_header(exheader_type)
+       local ex_invalid_opt = string.char(0x80,0x01,0xfe,0x18,0xfe,0x18,0xfe,0x18,0x0,0x0,0x0,0x0,0x0,0x0)
+function Packet:build_router_advert(mac_src,prefix,prefix_len,valid_time,preferred_time)
+       self.ip6_src = mac_to_lladdr(mac_src)

These functions are too specialized to be in the packet.lua library.
Special parameters like these belong in a script.

+function inet6_pton_simple(str)
+function ipv6tobin(str)

These functions are hard to understand for what they do. They don't
handle addresses of the form "::ffff:a.b.c.d". There is not enough error
checking; try nonsense addresses like "1::2::3" and "1:2:3" and
"1:2:3:4:5:6:7:8:9". Rather than trying to debug all these cases;
perhaps it would be better to provide a wrapper around getaddrinfo?

+function mactobin(str)
+       if not str then
+               return mactobin("00:00:00:00:00:00")
+       end
+function toipv6(raw_ipv6_addr)
+       if not raw_ipv6_addr then
+               return "?::?"
+       end
+function mac_to_lladdr(mac)
+       if not mac then
+               return "?::?"
+       end

I would rather see an error about a nil variable than silently insert
default values like this.

+--- Pare an IPv6 extension header. Just jump over it at the moment.
+-- @param force_continue Ignored.
+-- @return Whether the parsing succeeded.
+function Packet:ipv6_ext_header_parse(force_continue)
+       local ext_hdr_len = self.u8(self.ip6_data_offset + 1)
+       ext_hdr_len = ext_hdr_len*8 + 8
+       self.ip6_data_offset = self.ip6_data_offset + ext_hdr_len
+       self.ip6_nhdr = self.u8(self.ip6_data_offset)
+end

I think we must be more careful than this with extension headers. All
the currently defined extension headers have the nh value in the first
byte, but I hven't seen a guarantee that it will always be so. Also, the
way the function is called, the code will attempt to skip over any nh
value that isn't IPPROTO_TCP, IPPROTO_UDP, and IPPROTO_ICMPV6. What
about other things like IPPROTO_SCTP? That will produce nonsense if
parsed this way. Instead of looping until you get a known upper-layer
protocol (assuming everything else is an extension header), loop as long
as you have a known extension header (and assume everything else is an
upper-layer protocol). See ipv6_get_data_primitive in netutil.cc.

David Fifield
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/