diff --git a/aioxmpp/xso/model.py b/aioxmpp/xso/model.py index ece2e864..c5a6adb6 100644 --- a/aioxmpp/xso/model.py +++ b/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): diff --git a/docs/api/changelog.rst b/docs/api/changelog.rst index 879caf79..a0a6cc2c 100644 --- a/docs/api/changelog.rst +++ b/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 diff --git a/tests/xso/test_model.py b/tests/xso/test_model.py index d0c07664..76b2e40c 100644 --- a/tests/xso/test_model.py +++ b/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):