Re: IPv6 packet.lua review

[Xu Weilin <mzweilin () gmail com>]

On Thu, Jul 21, 2011 at 2:14 PM, David Fifield <david () bamsoftware com>wrote:

> 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?

Sorry, I neglected the other layer3 protocols.


>  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?

We need a Packet object before building the packet. The previous code is
just designed for parsing packets.


> +       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.

Agree.


> +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.

Reasonable.


> +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?
Toni has a better implementation of ip6tobin() which is nearer to our goal.


> +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.

The style is inherited from the original packet.lua. So we have to change
other functions at the same time, such as toip().


> +--- 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.
Thanks for your advice.

-- 
Regards
Xu Weilin 许伟林
_______________________________________________
Sent through the nmap-dev mailing list
http://cgi.insecure.org/mailman/listinfo/nmap-dev
Archived at http://seclists.org/nmap-dev/