ofproto/ofproto_v1_5_parser: OFPMultipartReply malformed message offset fix

Recently, I discovered major multipart message parser flaw. The issue
was observed while testing Aggregate Flow Statistics message in OpenFlow
1.5 and Open vSwitch. Similar (and potentially also vulnerable) code
snippets are also present in other message parsers (e.g. OFPHello). I'd
like to ask for opinions on proposed solution. If accepted, similar
patches should also be applied for other message parsers.

Brief description (steps to reproduce the issue):
1. REST API is called to retrieve aggregate flow stats:
	curl http://localhost:8080/stats/aggregateflow/8796750139643
2. Open vSwitch replies to Aggregate Stats Request with Aggregate Stats
Reply:
	message buffer: 0x06 0x13 0x00 0x28 0x53 0xfe 0xc4 0xaf 0x00 0x02 0x00
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x00
	(note that due to incomplete OF 1.5 support in OvS, message is
malformed - ofp_stats struct filled with zeros)
3. Message is processed by Ryu parsers:
	ofproto_parser.msg -> ofproto_v1_5_parser.msg_parser ->
ofproto_v1_5_parser.OFPMultipartReply.parser
4. Here, message body contents are parsed
(ofproto_v1_5_parser.OFPMultipartReply.parser, lines 1858-1861):
     while offset < msg_len:
         b = stats_type_cls.cls_stats_body_cls.parser(msg.buf, offset)
         body.append(b)
         offset += b.length if hasattr(b, 'length') else b.len
5. Due to incorrect message format, zero-filled message part is parsed
as b=OFPAggregateStats(length=0,stats=OFPStats(oxs_fields={})),
resulting in constant offset value, as in each iteration offset += 0.
6. Parser remains trapped in a infinite loop with offset = 16, msg_len =
40. Ryu controller hangs completely.

OFPMultipartReply parser was observed to handle malformed messages
improperly. The patch introduces offset check to fix processing of
malformed messages in ofproto_v1_5_parser.OFPMultipartReply.parser.

Signed-off-by: Michal Rzepka <mrzepka@student.agh.edu.pl>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

1 files changed, 5 insertions, 1 deletions

diff --git a/ryu/ofproto/ofproto_v1_5_parser.py b/ryu/ofproto/ofproto_v1_5_parser.py
index d34ae0f8..be2e8628 100644
--- a/ryu/ofproto/ofproto_v1_5_parser.py
+++ b/ryu/ofproto/ofproto_v1_5_parser.py
@@ -26,6 +26,7 @@ import six
 from ryu.lib import addrconv
 from ryu.lib.pack_utils import msg_pack_into
 from ryu.lib.packet import packet
+from ryu import exception
 from ryu import utils
 from ryu.ofproto.ofproto_parser import StringifyMixin, MsgBase, MsgInMsgBase
 from ryu.ofproto import ether
@@ -1857,8 +1858,11 @@ class OFPMultipartReply(MsgBase):
         body = []
         while offset < msg_len:
             b = stats_type_cls.cls_stats_body_cls.parser(msg.buf, offset)
+            offset_step = b.length if hasattr(b, 'length') else b.len
+            if offset_step < 1:
+                raise exception.OFPMalformedMessage()
             body.append(b)
-            offset += b.length if hasattr(b, 'length') else b.len
+            offset += offset_step
 
         if stats_type_cls.cls_body_single_struct:
             msg.body = body[0]