From 2968b8bbb743ada6df7e36c969228bef42830224 Mon Sep 17 00:00:00 2001 From: Eyal Date: Thu, 12 Dec 2019 14:49:13 +0200 Subject: [PATCH] Disable the use of anchors when parsing yaml This can be used as a DDoS attack Closes-Bug: 1785657 Change-Id: Icf460fea113e9279715cae87df3ef88a77575e04 --- mistral/event_engine/default_event_engine.py | 6 +- mistral/lang/parser.py | 4 +- mistral/services/workflows.py | 46 ++++++++++--- mistral/tests/unit/lang/v2/base.py | 9 +-- mistral/tests/unit/utils/test_safeLoader.py | 70 ++++++++++++++++++++ mistral/utils/safe_yaml.py | 62 +++++++++++++++++ 6 files changed, 179 insertions(+), 18 deletions(-) create mode 100644 mistral/tests/unit/utils/test_safeLoader.py create mode 100644 mistral/utils/safe_yaml.py diff --git a/mistral/event_engine/default_event_engine.py b/mistral/event_engine/default_event_engine.py index e0f68e6f8..aaa78a91a 100644 --- a/mistral/event_engine/default_event_engine.py +++ b/mistral/event_engine/default_event_engine.py @@ -23,7 +23,6 @@ from oslo_log import log as logging from oslo_service import threadgroup from oslo_utils import fnmatch import six -import yaml from mistral import context as auth_ctx from mistral.db.v2 import api as db_api @@ -33,6 +32,7 @@ from mistral import expressions from mistral import messaging as mistral_messaging from mistral.rpc import clients as rpc from mistral.services import security +from mistral.utils import safe_yaml LOG = logging.getLogger(__name__) @@ -83,8 +83,8 @@ class NotificationsConverter(object): config = cf.read() try: - definition_cfg = yaml.safe_load(config) - except yaml.YAMLError as err: + definition_cfg = safe_yaml.load(config) + except safe_yaml.YAMLError as err: if hasattr(err, 'problem_mark'): mark = err.problem_mark errmsg = ( diff --git a/mistral/lang/parser.py b/mistral/lang/parser.py index 0af0bf9b9..6909364e1 100644 --- a/mistral/lang/parser.py +++ b/mistral/lang/parser.py @@ -15,7 +15,6 @@ import cachetools import threading -import yaml from yaml import error import six @@ -27,6 +26,7 @@ from mistral.lang.v2 import actions as actions_v2 from mistral.lang.v2 import tasks as tasks_v2 from mistral.lang.v2 import workbook as wb_v2 from mistral.lang.v2 import workflows as wf_v2 +from mistral.utils import safe_yaml V2_0 = '2.0' @@ -50,7 +50,7 @@ def parse_yaml(text): """ try: - return yaml.safe_load(text) or {} + return safe_yaml.load(text) or {} except error.YAMLError as e: raise exc.DSLParsingException( "Definition could not be parsed: %s\n" % e diff --git a/mistral/services/workflows.py b/mistral/services/workflows.py index 7a49ccb00..bb88e848f 100644 --- a/mistral/services/workflows.py +++ b/mistral/services/workflows.py @@ -16,6 +16,7 @@ from mistral.db.v2 import api as db_api from mistral import exceptions as exc from mistral.lang import parser as spec_parser from mistral import utils +from mistral.utils import safe_yaml from mistral.workflow import states from oslo_log import log as logging @@ -84,7 +85,17 @@ def create_workflows(definition, scope='private', is_system=False, def _append_all_workflows(definition, is_system, scope, namespace, wf_list_spec, db_wfs): - for wf_spec in wf_list_spec.get_workflows(): + wfs = wf_list_spec.get_workflows() + + wfs_yaml = safe_yaml.load(definition) if len(wfs) != 1 else None + + for wf_spec in wfs: + if len(wfs) != 1: + definition = _cut_wf_definition_from_all( + wfs_yaml, + wf_spec.get_name() + ) + db_wfs.append( _create_workflow( wf_spec, @@ -114,15 +125,25 @@ def update_workflows(definition, scope='private', identifier=None, db_wfs = [] + wfs_yaml = safe_yaml.load(definition) if len(wfs) != 1 else None + with db_api.transaction(): - for wf_spec in wf_list_spec.get_workflows(): - db_wfs.append(_update_workflow( - wf_spec, - definition, - scope, - namespace=namespace, - identifier=identifier - )) + for wf_spec in wfs: + if len(wfs) != 1: + definition = _cut_wf_definition_from_all( + wfs_yaml, + wf_spec.get_name() + ) + + db_wfs.append( + _update_workflow( + wf_spec, + definition, + scope, + namespace=namespace, + identifier=identifier + ) + ) return db_wfs @@ -171,3 +192,10 @@ def _update_workflow(wf_spec, definition, scope, identifier=None, identifier if identifier else values['name'], values ) + + +def _cut_wf_definition_from_all(wfs_yaml, wf_name): + return safe_yaml.dump({ + 'version': wfs_yaml['version'], + wf_name: wfs_yaml[wf_name] + }) diff --git a/mistral/tests/unit/lang/v2/base.py b/mistral/tests/unit/lang/v2/base.py index 632e9ac1f..56215e0ea 100644 --- a/mistral/tests/unit/lang/v2/base.py +++ b/mistral/tests/unit/lang/v2/base.py @@ -14,12 +14,12 @@ import copy -import yaml from mistral import exceptions as exc from mistral.lang import parser as spec_parser from mistral.tests.unit import base from mistral import utils +from mistral.utils import safe_yaml class WorkflowSpecValidationTestCase(base.BaseTest): @@ -75,9 +75,10 @@ class WorkflowSpecValidationTestCase(base.BaseTest): dsl_yaml = base.get_resource(self._resource_path + '/' + dsl_file) if changes: - dsl_dict = yaml.safe_load(dsl_yaml) + dsl_dict = safe_yaml.safe_load(dsl_yaml) utils.merge_dicts(dsl_dict, changes) - dsl_yaml = yaml.safe_dump(dsl_dict, default_flow_style=False) + dsl_yaml = safe_yaml.safe_dump(dsl_dict, + default_flow_style=False) else: dsl_dict = copy.deepcopy(self._dsl_blank) @@ -87,7 +88,7 @@ class WorkflowSpecValidationTestCase(base.BaseTest): if changes: utils.merge_dicts(dsl_dict, changes) - dsl_yaml = yaml.safe_dump(dsl_dict, default_flow_style=False) + dsl_yaml = safe_yaml.safe_dump(dsl_dict, default_flow_style=False) if not expect_error: return self._spec_parser(dsl_yaml) diff --git a/mistral/tests/unit/utils/test_safeLoader.py b/mistral/tests/unit/utils/test_safeLoader.py new file mode 100644 index 000000000..0ba6344e4 --- /dev/null +++ b/mistral/tests/unit/utils/test_safeLoader.py @@ -0,0 +1,70 @@ +# Copyright 2019 - Nokia Corporation +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from unittest import TestCase + +from mistral.utils import safe_yaml + + +class TestSafeLoader(TestCase): + def test_safe_load(self): + yaml_text = """ + version: '2.0' + + wf1: + type: direct + + input: + - a: &a ["lol","lol","lol","lol","lol"] + - b: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a] + - c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b] + - d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c] + - e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d] + - f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e] + - g: &g [*f,*f,*f,*f,*f,*f,*f,*f,*f] + - h: &h [*g,*g,*g,*g,*g,*g,*g,*g,*g] + - i: &i [*h,*h,*h,*h,*h,*h,*h,*h,*h] + + + tasks: + hello: + action: std.echo output="Hello" + wait-before: 1 + publish: + result: <% task(hello).result %> + """ + + result = { + 'version': '2.0', + 'wf1': + {'type': 'direct', + 'input': [ + {'a': '&a ["lol","lol","lol","lol","lol"]'}, + {'b': '&b [*a,*a,*a,*a,*a,*a,*a,*a,*a]'}, + {'c': '&c [*b,*b,*b,*b,*b,*b,*b,*b,*b]'}, + {'d': '&d [*c,*c,*c,*c,*c,*c,*c,*c,*c]'}, + {'e': '&e [*d,*d,*d,*d,*d,*d,*d,*d,*d]'}, + {'f': '&f [*e,*e,*e,*e,*e,*e,*e,*e,*e]'}, + {'g': '&g [*f,*f,*f,*f,*f,*f,*f,*f,*f]'}, + {'h': '&h [*g,*g,*g,*g,*g,*g,*g,*g,*g]'}, + {'i': '&i [*h,*h,*h,*h,*h,*h,*h,*h,*h]'}], + 'tasks': + {'hello': { + 'action': 'std.echo output="Hello"', + 'wait-before': 1, 'publish': + {'result': '<% task(hello).result %>'} + }} + } + } + self.assertEqual(result, safe_yaml.load(yaml_text)) diff --git a/mistral/utils/safe_yaml.py b/mistral/utils/safe_yaml.py new file mode 100644 index 000000000..050e1eefd --- /dev/null +++ b/mistral/utils/safe_yaml.py @@ -0,0 +1,62 @@ +# Copyright 2019 - Nokia Corporation +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import yaml +from yaml import * # noqa + +yaml.SafeDumper.ignore_aliases = lambda *args: True + + +class SafeLoader(yaml.SafeLoader): + """Treat '@', '&', '*' as plain string. + + Anchors are not used in mistral workflow. It's better to + disable them completely. Anchors can be used as an exploit to a + Denial of service attack through expansion (Billion Laughs) + see https://en.wikipedia.org/wiki/Billion_laughs_attack. + Also this module uses the safe loader by default which is always + a better loader. + + When using yaml module to load a yaml file or a string use this + module instead of yaml. + + Example: + import mistral.utils.safe_yaml as safe_yaml + ... + ... + + safe_yaml.load(...) + + """ + + def fetch_alias(self): + return self.fetch_plain() + + def fetch_anchor(self): + return self.fetch_plain() + + def check_plain(self): + # Modified: allow '@' + if self.peek() == '@': + return True + else: + return super(SafeLoader, self).check_plain() + + +def load(stream): + return yaml.load(stream, SafeLoader) + + +def safe_load(stream): + return load(stream)