[CVE-2019-1000007] xso: fix parser error handling

aioxmpp/xso/model.py

@@ -2536,26 +2536,26 @@ def enforce_unknown_child_policy(policy, ev_args, error_handler=None):
 
 
 def guard(dest, ev_args):
-    next(dest)
     depth = 1
-    while True:
-        ev = yield
-        if ev[0] == "start":
-            depth += 1
-        elif ev[0] == "end":
-            depth -= 1
-        try:
-            dest.send(ev)
-        except StopIteration as exc:
-            return exc.value
-        except Exception as exc:
-            error = exc
-            break
-    while depth > 0:
-        ev_type, *_ = yield
-        if ev_type == "end":
-            depth -= 1
-    raise error
+    try:
+        next(dest)
+        while True:
+            ev = yield
+            if ev[0] == "start":
+                depth += 1
+            elif ev[0] == "end":
+                depth -= 1
+            try:
+                dest.send(ev)
+            except StopIteration as exc:
+                return exc.value
+    finally:
+        while depth > 0:
+            ev_type, *_ = yield
+            if ev_type == "end":
+                depth -= 1
+            elif ev_type == "start":
+                depth += 1
 
 
 def lang_attr(instance, ctx):

docs/api/changelog.rst

@@ -52,6 +52,20 @@ Version 0.11
 
 * :mod:`aioxmpp.ibb` (:xep:`47`) Support for In-Band Bytestreams.
 
+* Fix incorrect error handling in :mod:`aioxmpp.xso` when a supressing
+  :meth:`aioxmpp.xso.XSO.xso_error_handler` is in use.
+
+  Under certain circumstances, it is possible that the handling of supressed
+  error causes another error later on because the parsing stack mis-counts the
+  depth in which it is inside the XML tree. This makes elements appear in the
+  wrong place, typically leading to further errors.
+
+  In the worst case, using a supressing
+  :meth:`~aioxmpp.xso.XSO.xso_error_handler` in specific circumstances can be
+  vulnerable to denial of service and data injection into the XML stream.
+
+  (A CVE will be allocated for this.)
+
 .. _api-changelog-0.10:
 
 Version 0.10

tests/xso/test_model.py

@@ -4933,6 +4933,97 @@ def test_return_only_after_end_even_on_exception_and_reraise(self):
             ctx.exception
         )
 
+    def test_eat_end_after_exception_on_start(self):
+        class FooException(Exception):
+            pass
+
+        def processor():
+            raise FooException()
+            yield
+
+        cmd_sequence = [
+            ("start", None, "foo", {}),
+            ("end",),
+        ]
+
+        dest = processor()
+        guard = xso_model.guard(dest, cmd_sequence[0][1:])
+        next(guard)
+
+        with self.assertRaises(FooException):
+            guard.send(cmd_sequence[1])
+
+    def test_handles_increasing_nesting_while_dropping(self):
+        cmd_sequence = [
+            ("start", None, "foo", {}),
+            ("start", None, "bar", {}),
+            ("text", "fnord"),
+            ("end",),
+            ("start", None, "bar", {}),
+            ("text", "fnord"),
+            ("end",),
+            ("end",),
+        ]
+
+        dest = unittest.mock.MagicMock()
+        guard = xso_model.guard(dest, cmd_sequence[0][1:])
+        next(guard)
+
+        for cmd in cmd_sequence[1:2]:
+            guard.send(cmd)
+
+        exc = ValueError()
+        dest.send.side_effect = exc
+
+        for cmd in cmd_sequence[2:-1]:
+            guard.send(cmd)
+
+        with self.assertRaises(ValueError) as ctx:
+            guard.send(cmd_sequence[-1])
+
+        self.assertSequenceEqual(
+            [
+                unittest.mock.call.__next__(),
+            ]+[
+                unittest.mock.call.send(cmd)
+                for cmd in cmd_sequence[1:3]
+            ],
+            dest.mock_calls
+        )
+
+        self.assertIs(
+            exc,
+            ctx.exception
+        )
+    def test_handles_increasing_nesting_while_after_error_during_start(self):
+        class FooException(Exception):
+            pass
+
+        def processor():
+            raise FooException()
+            yield
+
+        cmd_sequence = [
+            ("start", None, "foo", {}),
+            ("start", None, "bar", {}),
+            ("text", "fnord"),
+            ("end",),
+            ("start", None, "bar", {}),
+            ("text", "fnord"),
+            ("end",),
+            ("end",),
+        ]
+
+        dest = processor()
+        guard = xso_model.guard(dest, cmd_sequence[0][1:])
+        next(guard)
+
+        for cmd in cmd_sequence[1:-1]:
+            guard.send(cmd)
+
+        with self.assertRaises(FooException):
+            guard.send(cmd_sequence[-1])
+
 
 class TestSAXDriver(unittest.TestCase):
     def setUp(self):