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 <[email protected]>
---
ryu/ofproto/ofproto_v1_5_parser.py | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/ryu/ofproto/ofproto_v1_5_parser.py
b/ryu/ofproto/ofproto_v1_5_parser.py
index d34ae0f..be2e862 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]
--
Michał Rzepka
------------------------------------------------------------------------------
_______________________________________________
Ryu-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ryu-devel
