Re: [Ryu-devel] Possible bugs in dhcp parser

[FUJITA T. <fuj...@la...>]

Hi, thanks for the report!

On Fri, 28 Aug 2015 16:52:41 -0400
A Sydney <asy...@gm...> wrote:

> I believe I have found a bug in the parser and have provided a possible fix
> below.  In addition, I believe there may be a second bug.  I would
> appreciate your feedback on both.
> 
> The text at the bottom of the email highlights the steps I took which
> generate the first bug and the possible fix.
> 
> Questions:
> 
> a) Are the steps in 3 below correct? If not, can you please provide
> feedback?
> 
> b) Is the output of 4 below what one should expect?
> 
> c) Second bug: In the following snippet (from ryu/lib/packet/dhcp.py), it
> appears that "parse_opt" is only set if len(buf) is greater than min_len
> (since there are no other instances of parse_opt in this file). In the
> event that len(buf) =< min_len, would this not result in an error at the
> "return" statement?
> 
> ...
> if len(buf) > min_len:
>     parse_opt = options.parser(buf[min_len:])
>     length += parse_opt.options_len
> 
> return (cls(op, addrconv.mac.bin_to_text(chaddr), parse_opt,
>             htype, hlen, hops, xid, secs, flags,
>             addrconv.ipv4.bin_to_text(ciaddr),
>             addrconv.ipv4.bin_to_text(yiaddr),
>             addrconv.ipv4.bin_to_text(siaddr),
>             addrconv.ipv4.bin_to_text(giaddr),
>             sname.decode('ascii'), boot_file),
>        None, buf[length:])
> ...

Looks like the code works as you said. That's a bug? At that point,
the parser consumes 236 bytes. If there is still the buffer to parse,
the parse does, right?

> Thank you in advance for your assistance,
> Ali
> 
> ==== FIRST BUG REPORT AND PROPOSED FIX ===
> 
> 1. Action:
> 
> Since the switch truncates pkts to the controller, I use the following to
> have the switch send the entire packet (I've assumed a max pkt size of 1500
> bytes).
> ...
> req = parser.OFPSetConfig(datapath, ofproto_v1_3.OFPC_FRAG_NORMAL, 1500)
> datapath.send_msg(req)
> ...
> 
> I also use the following to grab the dhcp header:
> 
> ...
> dhcp_pkt = packet.Packet(pkt.protocols[3], parse_cls=dhcp.dhcp)
> ...
> 
> 2. Result:
> 
> The following error results:
> 
> ...
>   dhcp_pkt = packet.Packet(pkt.protocols[3], parse_cls=dhcp.dhcp)
>   File "/usr/local/lib/python2.7/site-packages/ryu/lib/packet/packet.py",
> line 46, in __init__
>     self._parser(parse_cls)
>   File "/usr/local/lib/python2.7/site-packages/ryu/lib/packet/packet.py",
> line 55, in _parser
>     if proto:
>   File
> "/usr/local/lib/python2.7/site-packages/ryu/lib/packet/packet_base.py",
> line 46, in __len__
>     return self._MIN_LEN
> AttributeError: 'dhcp' object has no attribute '_MIN_LEN'
> ...
> 
> 3. Action:
> 
> I added the following two lines to the start of the dhcp class in the
> dhcp.py file around line 135 (PS. These lines also exist in the option
> class):
> 
> _UNPACK_STR = '!B'
> _MIN_LEN = struct.calcsize(_UNPACK_STR)
> # existing code is below
> _HLEN_UNPACK_STR = '!BBB'
> _HLEN_UNPACK_LEN = struct.calcsize(_HLEN_UNPACK_STR)

I think that the dhcp parser expects at least 236 bytes so _MIN_LEN
should be 236. If _MIN_LEN is 1, then the parse could get 1 byte
buffer (and then the parser crashes).


btw, I think that the udp parse should properly handle dhcp. With the
following patch, you can just do:

pkt = packet.Packet(buf)

pkt should includes ethernet, ip, udp, and dhcp headers.

diff --git a/ryu/lib/packet/udp.py b/ryu/lib/packet/udp.py
index a80e101..bae6d73 100644
--- a/ryu/lib/packet/udp.py
+++ b/ryu/lib/packet/udp.py
@@ -17,6 +17,7 @@ import struct
 
 from . import packet_base
 from . import packet_utils
+from . import dhcp
 
 
 class udp(packet_base.PacketBase):
@@ -49,11 +50,17 @@ class udp(packet_base.PacketBase):
         self.csum = csum
 
     @classmethod
+    def get_packet_type(cls, src_port, dst_port):
+        if (src_port == 68 and dst_port == 67) or (src_port == 67 and dst_port == 68):
+            return dhcp.dhcp
+        return None
+
+    @classmethod
     def parser(cls, buf):
         (src_port, dst_port, total_length, csum) = struct.unpack_from(
             cls._PACK_STR, buf)
         msg = cls(src_port, dst_port, total_length, csum)
-        return msg, None, buf[msg._MIN_LEN:total_length]
+        return msg, cls.get_packet_type(src_port, dst_port), buf[msg._MIN_LEN:total_length]
 
     def serialize(self, payload, prev):
         if self.total_length == 0: