From 381edc3bc7810f92c284c27c77c8d626b9cd0c27 Mon Sep 17 00:00:00 2001 From: Michael Howitz Date: Thu, 14 Sep 2017 11:39:28 +0200 Subject: [PATCH] Security fix deny using `format` method of str/unicode as it is not safe. See http://lucumr.pocoo.org/2016/12/29/careful-with-str-format/ --- docs/CHANGES.rst | 3 + src/RestrictedPython/Eval.py | 8 ++- src/RestrictedPython/Guards.py | 26 +++++++- src/RestrictedPython/README.rst | 19 ++++-- src/RestrictedPython/transformer.py | 11 +++- tests/__init__.py | 2 + tests/test_compile_restricted_function.py | 2 + tests/test_print_function.py | 21 ++++-- tests/test_print_stmt.py | 30 +++++++-- tests/transformer/test_attribute.py | 1 + tests/transformer/test_base_types.py | 79 ++++++++++++++++++++++- tests/transformer/test_call.py | 1 + tests/transformer/test_comparators.py | 4 +- tests/transformer/test_subscript.py | 15 ++++- tests/transformer/test_try.py | 15 +++-- tests/transformer/test_with_stmt.py | 5 +- 16 files changed, 209 insertions(+), 33 deletions(-) diff --git a/docs/CHANGES.rst b/docs/CHANGES.rst index dbb8a53..b83513f 100644 --- a/docs/CHANGES.rst +++ b/docs/CHANGES.rst @@ -10,6 +10,9 @@ Changes - Drop support of PyPy as there currently seems to be no way to restrict the builtins. See https://bitbucket.org/pypy/pypy/issues/2653. +- Security issue: No longer allow using the ``format`` method of str resp. + unicode. See http://lucumr.pocoo.org/2016/12/29/careful-with-str-format/. + 4.0a3 (2017-06-20) ------------------ diff --git a/src/RestrictedPython/Eval.py b/src/RestrictedPython/Eval.py index 836ea5e..19ab7e1 100644 --- a/src/RestrictedPython/Eval.py +++ b/src/RestrictedPython/Eval.py @@ -12,6 +12,7 @@ ############################################################################## """Restricted Python Expressions.""" +from . import Guards from ._compat import IS_PY2 from .compile import compile_restricted_eval @@ -38,7 +39,12 @@ def default_guarded_getitem(ob, index): class RestrictionCapableEval(object): """A base class for restricted code.""" - globals = {'__builtins__': None} + globals = { + '__builtins__': None, + '_str_': Guards.SecureStr, + } + if IS_PY2: + globals['_unicode_'] = Guards.SecureUnicode # restricted rcode = None diff --git a/src/RestrictedPython/Guards.py b/src/RestrictedPython/Guards.py index 2a0562b..e5e1d98 100644 --- a/src/RestrictedPython/Guards.py +++ b/src/RestrictedPython/Guards.py @@ -52,7 +52,6 @@ 'repr', 'round', 'slice', - 'str', 'tuple', 'zip' ] @@ -109,11 +108,10 @@ if IS_PY2: _safe_names.extend([ - 'basestring', + 'basestring', # cannot be instantiated 'cmp', 'long', 'unichr', - 'unicode', 'xrange', ]) _safe_exceptions.extend([ @@ -189,6 +187,28 @@ # type +class SecureStr(str): + """Str class which does not allow unsafe methods.""" + + def format(*args, **kw): + raise NotImplementedError('Using format() is not safe.') + + +safe_builtins['str'] = SecureStr +safe_builtins['_str_'] = SecureStr + + +if IS_PY2: + class SecureUnicode(unicode): + """Unicode class which does not allow unsafe methods.""" + + def format(*args, **kw): + raise NotImplementedError('Using format() is not safe.') + + safe_builtins['unicode'] = SecureUnicode + safe_builtins['_unicode_'] = SecureUnicode + + def _write_wrapper(): # Construct the write wrapper class def _handler(secattr, error_msg): diff --git a/src/RestrictedPython/README.rst b/src/RestrictedPython/README.rst index dd59ee5..628725f 100644 --- a/src/RestrictedPython/README.rst +++ b/src/RestrictedPython/README.rst @@ -11,17 +11,18 @@ controlled and restricted execution of code: ... def hello_world(): ... return "Hello World!" ... ''' - >>> from RestrictedPython import compile_restricted + >>> from RestrictedPython import compile_restricted, safe_builtins >>> code = compile_restricted(src, '', 'exec') The resulting code can be executed using the ``exec`` built-in: - >>> exec(code) + >>> glob = {'__builtins__': safe_builtins} + >>> exec(code, glob) -As a result, the ``hello_world`` function is now available in the -global namespace: +As a result, the ``hello_world`` function is now available in the ``glob`` +dict: - >>> hello_world() + >>> glob['hello_world']() 'Hello World!' Compatibility @@ -60,7 +61,12 @@ Specifically: 4. ``__import__`` is the normal Python import hook, and should be used to control access to Python packages and modules. -5. ``__builtins__`` is the normal Python builtins dictionary, which +5. ``_str_`` and ``_unicode_`` (the latter is Python 2 only) are the factories + to create string resp. unicode objects. If using ``safe_builtins`` + (see next item) they are predefined with secured subclasses of ``str`` resp. + ``unicode``. + +6. ``__builtins__`` is the normal Python builtins dictionary, which should be weeded down to a set that cannot be used to get around your restrictions. A usable "safe" set is ``RestrictedPython.Guards.safe_builtins``. @@ -100,6 +106,7 @@ callable, from which the restricted machinery will create the object): >>> from RestrictedPython.PrintCollector import PrintCollector >>> _print_ = PrintCollector >>> _getattr_ = getattr + >>> _str_ = str >>> src = ''' ... print("Hello World!") diff --git a/src/RestrictedPython/transformer.py b/src/RestrictedPython/transformer.py index abdeed3..ae30be4 100644 --- a/src/RestrictedPython/transformer.py +++ b/src/RestrictedPython/transformer.py @@ -510,7 +510,16 @@ def visit_Num(self, node): def visit_Str(self, node): """Allow string literals without restrictions.""" - return self.node_contents_visit(node) + node = self.node_contents_visit(node) + factory_name = '_str_' + if IS_PY2 and isinstance(node.s, unicode): + factory_name = '_unicode_' + new_node = ast.Call( + func=ast.Name(factory_name, ast.Load()), + args=[node], + keywords=[]) + copy_locations(new_node, node) + return new_node def visit_Bytes(self, node): """Allow bytes literals without restrictions. diff --git a/tests/__init__.py b/tests/__init__.py index e93f012..17f7df3 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -31,6 +31,8 @@ def _eval(source, glb=None): if glb is None: glb = {} return eval(code, glb) + # The next line can be dropped after the old implementation was dropped. + _eval.compile_func = compile_func return _eval diff --git a/tests/test_compile_restricted_function.py b/tests/test_compile_restricted_function.py index ca51aba..8d0c3c7 100644 --- a/tests/test_compile_restricted_function.py +++ b/tests/test_compile_restricted_function.py @@ -164,6 +164,7 @@ def test_compile_restricted_function_pretends_the_code_is_executed_in_a_global_s safe_globals = { '__name__': 'script', 'output': 'foo', + '_str_': str, } # safe_globals.update(safe_builtins) safe_locals = {} @@ -195,6 +196,7 @@ def test_compile_restricted_function_allows_invalid_python_identifiers_as_functi safe_globals = { '__name__': 'script', 'output': 'foo', + '_str_': str, } # safe_globals.update(safe_builtins) safe_locals = {} diff --git a/tests/test_print_function.py b/tests/test_print_function.py index 047e0fe..f8284d1 100644 --- a/tests/test_print_function.py +++ b/tests/test_print_function.py @@ -46,7 +46,11 @@ def test_print_function__simple_prints(): - glb = {'_print_': PrintCollector, '_getattr_': None} + glb = { + '_print_': PrintCollector, + '_getattr_': None, + '_str_': str, + } code, errors = compiler(ALLOWED_PRINT_FUNCTION)[:2] assert errors == () @@ -131,7 +135,8 @@ def test_print_function_with_kw_args(mocker): glb = { '_print_': PrintCollector, '_getattr_': None, - "_apply_": _apply_ + "_apply_": _apply_, + '_str_': str, } code, errors = compiler(ALLOWED_PRINT_FUNCTION_WITH_KWARGS)[:2] @@ -164,7 +169,8 @@ def test_print_function__protect_file(mocker): glb = { '_print_': PrintCollector, '_getattr_': _getattr_, - 'stream': stream + 'stream': stream, + '_str_': str, } code, errors = compiler(PROTECT_WRITE_ON_FILE)[:2] @@ -207,7 +213,11 @@ def main(): def test_print_function__nested_print_collector(): code, errors = compiler(INJECT_PRINT_COLLECTOR_NESTED)[:2] - glb = {"_print_": PrintCollector, '_getattr_': None} + glb = { + '_print_': PrintCollector, + '_getattr_': None, + '_str_': str, + } exec(code, glb) ret = glb['main']() @@ -307,7 +317,8 @@ def test_print_function_no_new_scope(): '_print_': PrintCollector, '__metaclass__': type, '_getattr_': None, - '_getiter_': lambda ob: ob + '_getiter_': lambda ob: ob, + '_str_': str, } exec(code, glb) diff --git a/tests/test_print_stmt.py b/tests/test_print_stmt.py index 1165600..b2ae378 100644 --- a/tests/test_print_stmt.py +++ b/tests/test_print_stmt.py @@ -36,7 +36,11 @@ @pytest.mark.parametrize(*c_exec) def test_print_stmt__simple_prints(c_exec): - glb = {'_print_': PrintCollector, '_getattr_': None} + glb = { + '_print_': PrintCollector, + '_getattr_': None, + '_str_': str, + } code, errors = c_exec(ALLOWED_PRINT_STATEMENT)[:2] assert code is not None @@ -96,7 +100,11 @@ def test_print_stmt__protect_chevron_print(c_exec, mocker): _getattr_ = mocker.stub() _getattr_.side_effect = getattr - glb = {'_getattr_': _getattr_, '_print_': PrintCollector} + glb = { + '_getattr_': _getattr_, + '_print_': PrintCollector, + '_str_': str, + } exec(code, glb) @@ -137,7 +145,11 @@ def main(): def test_print_stmt__nested_print_collector(c_exec, mocker): code, errors = c_exec(INJECT_PRINT_COLLECTOR_NESTED)[:2] - glb = {"_print_": PrintCollector, '_getattr_': None} + glb = { + '_print_': PrintCollector, + '_getattr_': None, + '_str_': str, + } exec(code, glb) ret = glb['main']() @@ -255,7 +267,11 @@ class A: @pytest.mark.parametrize(*c_exec) def test_print_stmt_no_new_scope(c_exec): code, errors = c_exec(NO_PRINT_SCOPES)[:2] - glb = {'_print_': PrintCollector, '_getattr_': None} + glb = { + '_print_': PrintCollector, + '_getattr_': None, + '_str_': str, + } exec(code, glb) ret = glb['class_scope']() @@ -273,7 +289,11 @@ def func(cond): @pytest.mark.parametrize(*c_exec) def test_print_stmt_conditional_print(c_exec): code, errors = c_exec(CONDITIONAL_PRINT)[:2] - glb = {'_print_': PrintCollector, '_getattr_': None} + glb = { + '_print_': PrintCollector, + '_getattr_': None, + '_str_': str, + } exec(code, glb) assert glb['func'](True) == '1\n' diff --git a/tests/transformer/test_attribute.py b/tests/transformer/test_attribute.py index 3159fff..62f7526 100644 --- a/tests/transformer/test_attribute.py +++ b/tests/transformer/test_attribute.py @@ -76,6 +76,7 @@ def test_RestrictingNodeTransformer__visit_Attribute__5( """It transforms writing to an attribute to `_write_`.""" glb = { '_write_': mocker.stub(), + '_str_': str, 'a': mocker.stub(), } glb['_write_'].return_value = glb['a'] diff --git a/tests/transformer/test_base_types.py b/tests/transformer/test_base_types.py index 9dc0867..2e65e96 100644 --- a/tests/transformer/test_base_types.py +++ b/tests/transformer/test_base_types.py @@ -1,6 +1,9 @@ +from RestrictedPython import safe_builtins from RestrictedPython._compat import IS_PY2 +from RestrictedPython._compat import IS_PY3 from tests import c_exec from tests import e_eval +from tests import e_exec import pytest @@ -14,7 +17,8 @@ def test_Num(e_eval): @pytest.mark.parametrize(*e_eval) def test_Bytes(e_eval): """It allows to use bytes literals.""" - assert e_eval('b"code"') == b"code" + glb = {'_str_': str} + assert e_eval('b"code"', glb) == b"code" @pytest.mark.parametrize(*e_eval) @@ -30,3 +34,76 @@ def test_Ellipsis(c_exec): """It prevents using the `ellipsis` statement.""" result = c_exec('...') assert result.errors == ('Line 1: Ellipsis statements are not allowed.',) + + +@pytest.mark.parametrize(*e_exec) +def test_Str__1(e_exec): + """It returns a str subclass for strings.""" + glb = { + '_getattr_': getattr, + '__builtins__': safe_builtins, + } + e_exec('a = "Hello world!"', glb) + assert isinstance(glb['a'], str) + + +@pytest.mark.skipif(IS_PY3, reason="In Python 3 there is no unicode.") +@pytest.mark.parametrize(*e_exec) +def test_Str__2(e_exec): + """It returns a unicode subclass for unicodes.""" + glb = { + '_getattr_': getattr, + '__builtins__': safe_builtins, + } + e_exec('a = u"Hello world!"', glb) + assert isinstance(glb['a'], unicode) + + +STRING_DOT_FORMAT_DENIED = """\ +'Hello {}'.format('world') +""" + + +@pytest.mark.parametrize(*e_eval) +def test_Str__3(e_eval): + """It prevents using the format method of a string. + + format() is considered harmful: + http://lucumr.pocoo.org/2016/12/29/careful-with-str-format/ + """ + if IS_PY2: + from RestrictedPython import RCompile + if e_eval.compile_func is RCompile.compile_restricted_eval: + pytest.skip('RCompile does not support secure strings.') + glb = { + '_getattr_': getattr, + '__builtins__': safe_builtins, + } + with pytest.raises(NotImplementedError) as err: + e_eval(STRING_DOT_FORMAT_DENIED, glb) + assert 'Using format() is not safe.' == str(err.value) + + +UNICODE_DOT_FORMAT_DENIED = """\ +u'Hello {}'.format('world') +""" + + +@pytest.mark.parametrize(*e_eval) +def test_Str__4(e_eval): + """It prevents using the format method of a unicode. + + format() is considered harmful: + http://lucumr.pocoo.org/2016/12/29/careful-with-str-format/ + """ + if IS_PY2: + from RestrictedPython import RCompile + if e_eval.compile_func is RCompile.compile_restricted_eval: + pytest.skip('RCompile does not support secure unicode.') + glb = { + '_getattr_': getattr, + '__builtins__': safe_builtins, + } + with pytest.raises(NotImplementedError) as err: + e_eval(UNICODE_DOT_FORMAT_DENIED, glb) + assert 'Using format() is not safe.' == str(err.value) diff --git a/tests/transformer/test_call.py b/tests/transformer/test_call.py index 4ab5677..3cb1ef0 100644 --- a/tests/transformer/test_call.py +++ b/tests/transformer/test_call.py @@ -57,6 +57,7 @@ def test_RestrictingNodeTransformer__visit_Call__2(e_exec, mocker): glb = { '_apply_': _apply_, + '_str_': str, 'foo': lambda *args, **kwargs: (args, kwargs) } diff --git a/tests/transformer/test_comparators.py b/tests/transformer/test_comparators.py index bb056f7..f95c27f 100644 --- a/tests/transformer/test_comparators.py +++ b/tests/transformer/test_comparators.py @@ -6,13 +6,13 @@ @pytest.mark.parametrize(*e_eval) def test_RestrictingNodeTransformer__visit_Eq__1(e_eval): """It allows == expressions.""" - assert e_eval('1 == int("1")') is True + assert e_eval('1 == int(1.0)') is True @pytest.mark.parametrize(*e_eval) def test_RestrictingNodeTransformer__visit_NotEq__1(e_eval): """It allows != expressions.""" - assert e_eval('1 != int("1")') is False + assert e_eval('1 != int(1.0)') is False @pytest.mark.parametrize(*e_eval) diff --git a/tests/transformer/test_subscript.py b/tests/transformer/test_subscript.py index 6a74fc1..30c2221 100644 --- a/tests/transformer/test_subscript.py +++ b/tests/transformer/test_subscript.py @@ -14,7 +14,10 @@ def test_read_simple_subscript(e_exec, mocker): value = None _getitem_ = mocker.stub() _getitem_.side_effect = lambda ob, index: (ob, index) - glb = {'_getitem_': _getitem_} + glb = { + '_getitem_': _getitem_, + '_str_': str, + } e_exec(SIMPLE_SUBSCRIPTS, glb) assert (value, 'b') == glb['simple_subscript'](value) @@ -146,7 +149,10 @@ def test_write_subscripts( value = {'b': None} _write_ = mocker.stub() _write_.side_effect = lambda ob: ob - glb = {'_write_': _write_} + glb = { + '_write_': _write_, + '_str_': str, + } e_exec(WRITE_SUBSCRIPTS, glb) glb['assign_subscript'](value) @@ -165,7 +171,10 @@ def test_del_subscripts( value = {'b': None} _write_ = mocker.stub() _write_.side_effect = lambda ob: ob - glb = {'_write_': _write_} + glb = { + '_write_': _write_, + '_str_': str, + } e_exec(DEL_SUBSCRIPT, glb) glb['del_subscript'](value) diff --git a/tests/transformer/test_try.py b/tests/transformer/test_try.py index 181173b..775b78a 100644 --- a/tests/transformer/test_try.py +++ b/tests/transformer/test_try.py @@ -20,8 +20,9 @@ def try_except(m): def test_RestrictingNodeTransformer__visit_Try__1( e_exec, mocker): """It allows try-except statements.""" + glb = {'_str_': str} trace = mocker.stub() - e_exec(TRY_EXCEPT)['try_except'](trace) + e_exec(TRY_EXCEPT, glb)['try_except'](trace) trace.assert_has_calls([ mocker.call('try'), @@ -44,8 +45,9 @@ def try_except_else(m): def test_RestrictingNodeTransformer__visit_Try__2( e_exec, mocker): """It allows try-except-else statements.""" + glb = {'_str_': str} trace = mocker.stub() - e_exec(TRY_EXCEPT_ELSE)['try_except_else'](trace) + e_exec(TRY_EXCEPT_ELSE, glb)['try_except_else'](trace) trace.assert_has_calls([ mocker.call('try'), @@ -68,8 +70,9 @@ def try_finally(m): def test_RestrictingNodeTransformer__visit_TryFinally__1( e_exec, mocker): """It allows try-finally statements.""" + glb = {'_str_': str} trace = mocker.stub() - e_exec(TRY_FINALLY)['try_finally'](trace) + e_exec(TRY_FINALLY, glb)['try_finally'](trace) trace.assert_has_calls([ mocker.call('try'), @@ -93,8 +96,9 @@ def try_except_finally(m): def test_RestrictingNodeTransformer__visit_TryFinally__2( e_exec, mocker): """It allows try-except-finally statements.""" + glb = {'_str_': str} trace = mocker.stub() - e_exec(TRY_EXCEPT_FINALLY)['try_except_finally'](trace) + e_exec(TRY_EXCEPT_FINALLY, glb)['try_except_finally'](trace) trace.assert_has_calls([ mocker.call('try'), @@ -120,8 +124,9 @@ def try_except_else_finally(m): def test_RestrictingNodeTransformer__visit_TryFinally__3( e_exec, mocker): """It allows try-except-else-finally statements.""" + glb = {'_str_': str} trace = mocker.stub() - e_exec(TRY_EXCEPT_ELSE_FINALLY)['try_except_else_finally'](trace) + e_exec(TRY_EXCEPT_ELSE_FINALLY, glb)['try_except_else_finally'](trace) trace.assert_has_calls([ mocker.call('try'), diff --git a/tests/transformer/test_with_stmt.py b/tests/transformer/test_with_stmt.py index 9543ce5..dd4fa58 100644 --- a/tests/transformer/test_with_stmt.py +++ b/tests/transformer/test_with_stmt.py @@ -162,7 +162,10 @@ def test_with_stmt_subscript(e_exec, mocker): _write_ = mocker.stub() _write_.side_effect = lambda ob: ob - glb = {'_write_': _write_} + glb = { + '_write_': _write_, + '_str_': str, + } e_exec(WITH_STMT_SUBSCRIPT, glb) # Test single_key