From ea8566ef8942d3df7e641e51acd77da7b39c10c1 Mon Sep 17 00:00:00 2001 From: Andreas Wagner Date: Thu, 30 Mar 2017 15:43:18 +0200 Subject: [PATCH] Disallow ctypes --- tests/test_js_signing.py | 14 ++-- validator/testcases/javascript/actions.py | 67 +++++++++++++------ validator/testcases/javascript/jstypes.py | 6 +- .../javascript/predefinedentities.py | 25 ++----- validator/testcases/javascript/traverser.py | 21 ++++++ 5 files changed, 86 insertions(+), 47 deletions(-) diff --git a/tests/test_js_signing.py b/tests/test_js_signing.py index 911ca44ef..bd47e1b7d 100644 --- a/tests/test_js_signing.py +++ b/tests/test_js_signing.py @@ -335,15 +335,19 @@ def test(prop): yield test, prop def test_ctypes(self): - """Tests that usage of `ctypes` generates warnings.""" + """Tests that usage of `ctypes` generates errors.""" self.run_script(""" ctypes.open("libc.so.6"); """) - self.assert_failed(with_warnings=[ - {'id': ('js', 'traverser', 'dangerous_global'), - 'editors_only': True, - 'signing_severity': 'high'}]) + self.assert_failed(with_errors=[ + {'id': ('js', 'traverser', 'forbidden_global')}]) + + self.run_script(""" + Components.utils.import("resource://gre/modules/ctypes.jsm") + """) + self.assert_failed(with_errors=[ + {'id': ('js', 'traverser', 'forbidden_global')}]) def test_nsIProcess(self): """Tests that usage of `nsIProcess` generates warnings.""" diff --git a/validator/testcases/javascript/actions.py b/validator/testcases/javascript/actions.py index c492c9c90..94130723d 100644 --- a/validator/testcases/javascript/actions.py +++ b/validator/testcases/javascript/actions.py @@ -439,28 +439,51 @@ def _call_expression(traverser, node): column=traverser.position, context=traverser.context) - if member.is_global and callable(member.value.get('dangerous', None)): - result = member.value['dangerous'](a=args, t=traverser._traverse_node, - e=traverser.err) - name = member.value.get('name', '') - - if result and name: - kwargs = { - 'err_id': ('testcases_javascript_actions', '_call_expression', - 'called_dangerous_global'), - 'warning': '`%s` called in potentially dangerous manner' % - member.value['name'], - 'description': - 'The global `%s` function was called using a set ' - 'of dangerous parameters. Calls of this nature ' - 'are deprecated.' % member.value['name']} - - if isinstance(result, DESCRIPTION_TYPES): - kwargs['description'] = result - elif isinstance(result, dict): - kwargs.update(result) - - traverser.warning(**kwargs) + if member.is_global: + if callable(member.value.get('dangerous', None)): + result = member.value['dangerous'](a=args, t=traverser._traverse_node, + e=traverser.err) + name = member.value.get('name', '') + + if result and name: + kwargs = { + 'err_id': ('testcases_javascript_actions', '_call_expression', + 'called_dangerous_global'), + 'warning': '`%s` called in potentially dangerous manner' % + member.value['name'], + 'description': + 'The global `%s` function was called using a set ' + 'of dangerous parameters. Calls of this nature ' + 'are deprecated.' % member.value['name']} + + if isinstance(result, DESCRIPTION_TYPES): + kwargs['description'] = result + elif isinstance(result, dict): + kwargs.update(result) + + traverser.warning(**kwargs) + + if callable(member.value.get('forbidden', None)): + result = member.value['forbidden'](a=args, t=traverser._traverse_node, + e=traverser.err) + name = member.value.get('name', '') + + if result and name: + kwargs = { + 'err_id': ('testcases_javascript_actions', '_call_expression', + 'called_forbidden_global'), + 'warning': ' Forbidden `%s` called' % + member.value['name'], + 'description': + 'The forbidden global `%s` function was called.' + % member.value['name']} + + if isinstance(result, DESCRIPTION_TYPES): + kwargs['description'] = result + elif isinstance(result, dict): + kwargs.update(result) + + traverser.error(**kwargs) elif (node['callee']['type'] == 'MemberExpression' and node['callee']['property']['type'] == 'Identifier'): diff --git a/validator/testcases/javascript/jstypes.py b/validator/testcases/javascript/jstypes.py index 2db3dd590..752bf1ab4 100644 --- a/validator/testcases/javascript/jstypes.py +++ b/validator/testcases/javascript/jstypes.py @@ -256,7 +256,11 @@ def apply_value(name): if name in self.value: output.value[name] = self.value[name] - map(apply_value, ('dangerous', 'readonly', 'context', 'name')) + map(apply_value, ('forbidden', + 'dangerous', + 'readonly', + 'context', + 'name')) output.is_global = True output.context = self.context return output diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index fd28abb13..db78f5e54 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -717,25 +717,12 @@ def interface_obj(iface): u'getInstallForURL': ADDON_INSTALL_METHOD, u'installAddonsFromWebpage': ADDON_INSTALL_METHOD}}, - u'ctypes': {'dangerous': { + u'ctypes': {'forbidden': { 'description': ( - 'Insufficiently meticulous use of ctypes can lead to serious, ' - 'and often exploitable, errors. The use of bundled binary code, ' - 'or access to system libraries, may allow for add-ons to ' - 'perform unsafe operations. All ctypes use must be carefully ' - 'reviewed by a qualified reviewer.'), - 'editors_only': True, - 'signing_help': ('Please try to avoid interacting with or bundling ' - 'native binaries whenever possible. If you are ' - 'bundling binaries for performance reasons, please ' - 'consider alternatives such as Emscripten ' - '(http://mzl.la/1KrSUh2), JavaScript typed arrays ' - '(http://mzl.la/1Iw02sr), and Worker threads ' - '(http://mzl.la/1OGfAcc).', - 'Any code which makes use of the `ctypes` API ' - 'must undergo manual code review for at least one ' - 'submission.'), - 'signing_severity': 'high'}}, + 'Starting with Firefox 53, add-ons loading binary libraries are ' + 'not allowed. Native Messaging is the only allowed method to ' + 'communicate with external binaries. For more information, please ' + 'refer to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Native_messaging')}}, u'document': {'value': @@ -925,7 +912,7 @@ def interface_obj(iface): 'Care should be taken to ensure that ' 'this is done safely.')}}, u'import': - {'dangerous': + {'forbidden': lambda a, t, e: a and 'ctypes.jsm' in _get_as_str(t(a[0]))}, diff --git a/validator/testcases/javascript/traverser.py b/validator/testcases/javascript/traverser.py index 0397969ee..e3973fdca 100644 --- a/validator/testcases/javascript/traverser.py +++ b/validator/testcases/javascript/traverser.py @@ -325,6 +325,27 @@ def _build_global(self, name, entity): self._debug('DANGEROUS') self.warning(**kwargs) + if not callable(entity.get('forbidden')): + dang = entity.get('forbidden') + + if callable(dang): + dang = dang(self._traverse_node, self.err) + + if dang: + kwargs = dict( + err_id=('js', 'traverser', 'forbidden_global'), + error='Access to the `%s` global' % name, + description='Access to the `%s` property is ' + 'forbidden.' % name) + + if isinstance(dang, DESCRIPTION_TYPES): + kwargs['description'] = dang + elif isinstance(dang, dict): + kwargs.update(dang) + + self._debug('FORBIDDEN') + self.error(**kwargs) + entity.setdefault('name', name) # Build out the wrapper object from the global definition.