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>
This commit is contained in:
Michał Rzepka 2016-09-27 11:19:21 +02:00 committed by FUJITA Tomonori
parent 513a9d93b2
commit cb83c858cd

View File

@ -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]