From 4da45868e439a05b10b1da65fedc5366b9044231 Mon Sep 17 00:00:00 2001 From: Nguyen Damien Date: Wed, 28 Jul 2021 18:48:58 +0200 Subject: [PATCH 1/6] Implement handling of `os.chmod` --- pylint_secure_coding_standard.py | 75 +++++++++++++++++++++ tests/os_chmod_test.py | 112 +++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+) create mode 100644 tests/os_chmod_test.py diff --git a/pylint_secure_coding_standard.py b/pylint_secure_coding_standard.py index f1f46d6..dee07c9 100644 --- a/pylint_secure_coding_standard.py +++ b/pylint_secure_coding_standard.py @@ -282,6 +282,74 @@ def _is_yaml_unsafe_call(node): # ============================================================================== +def _chmod_is_allowed_mode(value): + """ + Test whether a single mode value is valid. + + Args: + value (str): Mode value + """ + return value in ( + 'S_ISUID', + 'S_ISGID', + 'S_ENFMT', + 'S_ISVTX', + 'S_IREAD', + 'S_IWRITE', + 'S_IEXEC', + 'S_IRWXU', + 'S_IRUSR', + 'S_IWUSR', + 'S_IXUSR', + # 'S_IRWXG', + 'S_IRGRP', + # 'S_IWGRP', + # 'S_IXGRP', + # 'S_IRWXO', + 'S_IROTH', + # 'S_IWOTH', + # 'S_IXOTH', + ) + + +def _chmod_get_mode(node): + """ + Extract the mode constant of a node. + + Args: + node (astroid.node_classes.NodeNG): an AST node + """ + if isinstance(node, astroid.Name): + return [node.name] + if isinstance(node, astroid.Attribute) and isinstance(node.expr, astroid.Name) and node.expr.name == 'stat': + return [node.attrname] + if isinstance(node, astroid.BinOp): + return _chmod_get_mode(node.left) + _chmod_get_mode(node.right) + return [] + + +def _chmod_has_wx_for_go(node): + if platform.system() == 'Windows': + return False + + modes = None + if len(node.args) > 1: + modes = _chmod_get_mode(node.args[1]) + elif node.keywords: + for keyword in node.keywords: + if keyword.arg == 'mode': + modes = _chmod_get_mode(keyword.value) + break + + if modes is None: + # NB: this would be from invalid code such as `os.chmod("file.txt")` + raise RuntimeError('Unable to extract `mode` argument from function call!') + return any(not _chmod_is_allowed_mode(mode) for mode in modes) + + +# ============================================================================== + + class SecureCodingStandardChecker(BaseChecker): # pylint: disable=too-many-instance-attributes """Plugin class.""" @@ -429,6 +497,11 @@ class SecureCodingStandardChecker(BaseChecker): # pylint: disable=too-many-inst 'os-mknod-unsafe-permissions', 'Avoid using `os.mknod` with unsafe file permissions (by default 0 <= mode <= 0o755)', ), + 'W8019': ( + 'Avoid using `os.chmod` with unsafe permissions (W ^ X for group and others)', + 'os-chmod-unsafe-permissions', + 'Avoid using `os.chmod` with unsafe file permissions (W ^ X for group and others)', + ), } def __init__(self, linter): @@ -479,6 +552,8 @@ def visit_call(self, node): # pylint: disable=too-many-branches self.add_message('avoid-marshal-load', node=node) elif _is_function_call(node, module='shelve', function='open'): self.add_message('avoid-shelve-open', node=node) + elif _is_function_call(node, module='os', function='chmod') and _chmod_has_wx_for_go(node): + self.add_message('os-chmod-unsafe-permissions', node=node) elif _is_unix(): if ( _is_function_call(node, module='os', function=('mkdir', 'makedirs')) diff --git a/tests/os_chmod_test.py b/tests/os_chmod_test.py new file mode 100644 index 0000000..0f7e0a9 --- /dev/null +++ b/tests/os_chmod_test.py @@ -0,0 +1,112 @@ +# -*- coding: utf-8 -*- +# Copyright 2021 Damien Nguyen +# +# 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 astroid +import pylint.testutils +import pytest + +import pylint_secure_coding_standard as pylint_scs + + +class TestSecureCodingStandardChecker(pylint.testutils.CheckerTestCase): + CHECKER_CLASS = pylint_scs.SecureCodingStandardChecker + + @pytest.mark.parametrize( + 's, expected', + ( + ('S_IREAD', ['S_IREAD']), + ('stat.S_IREAD', ['S_IREAD']), + ('S_IREAD | S_IWRITE', ['S_IREAD', 'S_IWRITE']), + ('stat.S_IREAD | stat.S_IWRITE', ['S_IREAD', 'S_IWRITE']), + ('stat.S_IREAD | stat.S_IWRITE | S_IXUSR', ['S_IREAD', 'S_IWRITE', 'S_IXUSR']), + ('bla.S_IREAD', []), + ), + ) + def test_chmod_get_mode(self, s, expected): + node = astroid.extract_node(s + ' #@') + assert pylint_scs._chmod_get_mode(node) == expected + + @pytest.mark.parametrize( + 'platform, enabled_platform', + ( + ('Linux', True), + ('Darwin', True), + ('Java', True), + ('Windows', False), + ), + ) + @pytest.mark.parametrize('fname', ('"file.txt"', 'fname')) + @pytest.mark.parametrize('arg_type', ('', 'mode='), ids=('arg', 'keyword')) + @pytest.mark.parametrize( + 'forbidden', + ( + 'S_IRGRP', + 'S_IRWXG', + 'S_IWGRP', + 'S_IXGRP', + 'S_IRWXO', + 'S_IWOTH', + 'S_IXOTH', + ), + ) + @pytest.mark.parametrize( + 's', + ( + '', + 'S_IREAD', + 'S_IREAD | S_IWRITE', + 'S_IRUSR | S_IWUSR | S_IXUSR', + ), + ids=lambda s: s if s else '', + ) + def test_chmod(self, mocker, platform, enabled_platform, fname, arg_type, forbidden, s): + mocker.patch('platform.system', lambda: platform) + + if s: + code = f'os.chmod({fname}, {arg_type}{s} | {forbidden}) #@' + else: + code = f'os.chmod({fname}, {arg_type} {forbidden}) #@' + + print(code) + node = astroid.extract_node(code) + if enabled_platform and forbidden != 'S_IRGRP': + with self.assertAddsMessages(pylint.testutils.Message(msg_id='os-chmod-unsafe-permissions', node=node)): + self.checker.visit_call(node) + else: + with self.assertNoMessages(): + self.checker.visit_call(node) + + @pytest.mark.parametrize( + 'platform, enabled_platform', + ( + ('Linux', True), + ('Darwin', True), + ('Java', True), + ('Windows', False), + ), + ) + @pytest.mark.parametrize('s', ('os.chmod("file")',)) + def test_chmod_invalid(self, mocker, platform, enabled_platform, s): + mocker.patch('platform.system', lambda: platform) + + print(s) + node = astroid.extract_node(s) + + if enabled_platform: + with pytest.raises(RuntimeError): + self.checker.visit_call(node) + else: + with self.assertNoMessages(): + self.checker.visit_call(node) From 8af449490e50a455a1416aa99a9e541967dfec00 Mon Sep 17 00:00:00 2001 From: Nguyen Damien Date: Wed, 28 Jul 2021 18:50:15 +0200 Subject: [PATCH 2/6] Update README --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 00063bb..226fc2c 100644 --- a/README.md +++ b/README.md @@ -32,6 +32,7 @@ pylint plugin that enforces some secure coding standards. | W8016 | Avoid using `os.mkdir` and `os.makedirs` with unsafe file permissions | | W8017 | Avoid using `os.mkfifo` with unsafe file permissions | | W8018 | Avoid using `os.mknod` with unsafe file permissions | +| W8019 | Avoid using `os.chmod` with unsafe permissions (W ^ X for group and others) | ## Plugin configuration options From 1fac1acd49cd7633baa2a0f9e8f57d1796101b19 Mon Sep 17 00:00:00 2001 From: Nguyen Damien Date: Wed, 28 Jul 2021 18:51:39 +0200 Subject: [PATCH 3/6] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c6d3cb..16ad798 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added W8016 to warn when using `os.mkdir` and `os.makedir` with unsafe permissions (UNIX-only) - Added W8017 to warn when using `os.mkfifo` with unsafe permissions (UNIX-only) - Added W8018 to warn when using `os.mknod` with unsafe permissions (UNIX-only) +- Added W8019 to warn when using `os.chmod` with unsafe permissions (all except Windows) ### Updated From 798dc63abd9caecb45fa88df757317d99b51c811 Mon Sep 17 00:00:00 2001 From: Nguyen Damien Date: Wed, 28 Jul 2021 18:57:23 +0200 Subject: [PATCH 4/6] Improve function naming --- pylint_secure_coding_standard.py | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/pylint_secure_coding_standard.py b/pylint_secure_coding_standard.py index dee07c9..30653f6 100644 --- a/pylint_secure_coding_standard.py +++ b/pylint_secure_coding_standard.py @@ -282,33 +282,20 @@ def _is_yaml_unsafe_call(node): # ============================================================================== -def _chmod_is_allowed_mode(value): +def _chmod_is_wx_for_go(value): """ - Test whether a single mode value is valid. + Test whether a single mode value is has W or X for group or others. Args: value (str): Mode value """ return value in ( - 'S_ISUID', - 'S_ISGID', - 'S_ENFMT', - 'S_ISVTX', - 'S_IREAD', - 'S_IWRITE', - 'S_IEXEC', - 'S_IRWXU', - 'S_IRUSR', - 'S_IWUSR', - 'S_IXUSR', - # 'S_IRWXG', - 'S_IRGRP', - # 'S_IWGRP', - # 'S_IXGRP', - # 'S_IRWXO', - 'S_IROTH', - # 'S_IWOTH', - # 'S_IXOTH', + 'S_IRWXG', + 'S_IWGRP', + 'S_IXGRP', + 'S_IRWXO', + 'S_IWOTH', + 'S_IXOTH', ) @@ -344,7 +331,7 @@ def _chmod_has_wx_for_go(node): if modes is None: # NB: this would be from invalid code such as `os.chmod("file.txt")` raise RuntimeError('Unable to extract `mode` argument from function call!') - return any(not _chmod_is_allowed_mode(mode) for mode in modes) + return any(_chmod_is_wx_for_go(mode) for mode in modes) # ============================================================================== From b21aeaffd0fde9b302de03c368c862b08e0daec8 Mon Sep 17 00:00:00 2001 From: Nguyen Damien Date: Wed, 28 Jul 2021 22:22:40 +0200 Subject: [PATCH 5/6] Better processing of the mode option by evaluating its expression --- pylint_secure_coding_standard.py | 104 ++++++++++++++++++++----------- tests/os_chmod_test.py | 77 ++++++++++++++++++++--- 2 files changed, 136 insertions(+), 45 deletions(-) diff --git a/pylint_secure_coding_standard.py b/pylint_secure_coding_standard.py index 30653f6..b10a3b0 100644 --- a/pylint_secure_coding_standard.py +++ b/pylint_secure_coding_standard.py @@ -15,7 +15,9 @@ """Main file for the pylint_secure_coding_standard plugin.""" +import operator import platform +import stat import astroid from pylint.checkers import BaseChecker @@ -281,22 +283,39 @@ def _is_yaml_unsafe_call(node): # ============================================================================== - -def _chmod_is_wx_for_go(value): - """ - Test whether a single mode value is has W or X for group or others. - - Args: - value (str): Mode value - """ - return value in ( - 'S_IRWXG', - 'S_IWGRP', - 'S_IXGRP', - 'S_IRWXO', - 'S_IWOTH', - 'S_IXOTH', - ) +_unop = {'-': operator.neg, 'not': operator.not_} +_binop = { + '+': operator.add, + '-': operator.sub, + '*': operator.mul, + '/': operator.truediv, + '//': operator.floordiv, + '%': operator.mod, + '^': operator.xor, + '|': operator.or_, + '&': operator.and_, +} +_chmod_known_mode_values = ( + 'S_ISUID', + 'S_ISGID', + 'S_ENFMT', + 'S_ISVTX', + 'S_IREAD', + 'S_IWRITE', + 'S_IEXEC', + 'S_IRWXU', + 'S_IRUSR', + 'S_IWUSR', + 'S_IXUSR', + 'S_IRWXG', + 'S_IRGRP', + 'S_IWGRP', + 'S_IXGRP', + 'S_IRWXO', + 'S_IROTH', + 'S_IWOTH', + 'S_IXOTH', +) def _chmod_get_mode(node): @@ -305,33 +324,48 @@ def _chmod_get_mode(node): Args: node (astroid.node_classes.NodeNG): an AST node + + Raises: + ValueError: if a node is encountered that cannot be processed """ - if isinstance(node, astroid.Name): - return [node.name] - if isinstance(node, astroid.Attribute) and isinstance(node.expr, astroid.Name) and node.expr.name == 'stat': - return [node.attrname] + if isinstance(node, astroid.Name) and node.name in _chmod_known_mode_values: + return getattr(stat, node.name) + if ( + isinstance(node, astroid.Attribute) + and isinstance(node.expr, astroid.Name) + and node.attrname in _chmod_known_mode_values + and node.expr.name == 'stat' + ): + return getattr(stat, node.attrname) + if isinstance(node, astroid.UnaryOp): + return _unop[node.op](_chmod_get_mode(node.operand)) if isinstance(node, astroid.BinOp): - return _chmod_get_mode(node.left) + _chmod_get_mode(node.right) - return [] + return _binop[node.op](_chmod_get_mode(node.left), _chmod_get_mode(node.right)) + + raise ValueError(f'Do not know how to process node: {node.repr_tree()}') def _chmod_has_wx_for_go(node): if platform.system() == 'Windows': + # On Windows, only stat.S_IREAD and stat.S_IWRITE can be used, all other bits are ignored return False - modes = None - if len(node.args) > 1: - modes = _chmod_get_mode(node.args[1]) - elif node.keywords: - for keyword in node.keywords: - if keyword.arg == 'mode': - modes = _chmod_get_mode(keyword.value) - break - - if modes is None: - # NB: this would be from invalid code such as `os.chmod("file.txt")` - raise RuntimeError('Unable to extract `mode` argument from function call!') - return any(_chmod_is_wx_for_go(mode) for mode in modes) + try: + modes = None + if len(node.args) > 1: + modes = _chmod_get_mode(node.args[1]) + elif node.keywords: + for keyword in node.keywords: + if keyword.arg == 'mode': + modes = _chmod_get_mode(keyword.value) + break + except ValueError: + return False + else: + if modes is None: + # NB: this would be from invalid code such as `os.chmod("file.txt")` + raise RuntimeError('Unable to extract `mode` argument from function call!') + return bool(modes & (stat.S_IWGRP | stat.S_IXGRP | stat.S_IWOTH | stat.S_IXOTH)) # ============================================================================== diff --git a/tests/os_chmod_test.py b/tests/os_chmod_test.py index 0f7e0a9..ad28258 100644 --- a/tests/os_chmod_test.py +++ b/tests/os_chmod_test.py @@ -13,6 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import stat + import astroid import pylint.testutils import pytest @@ -26,18 +28,58 @@ class TestSecureCodingStandardChecker(pylint.testutils.CheckerTestCase): @pytest.mark.parametrize( 's, expected', ( - ('S_IREAD', ['S_IREAD']), - ('stat.S_IREAD', ['S_IREAD']), - ('S_IREAD | S_IWRITE', ['S_IREAD', 'S_IWRITE']), - ('stat.S_IREAD | stat.S_IWRITE', ['S_IREAD', 'S_IWRITE']), - ('stat.S_IREAD | stat.S_IWRITE | S_IXUSR', ['S_IREAD', 'S_IWRITE', 'S_IXUSR']), - ('bla.S_IREAD', []), + ('S_IREAD', stat.S_IREAD), + ('stat.S_IREAD', stat.S_IREAD), + ('S_IREAD | S_IWRITE', stat.S_IREAD | stat.S_IWRITE), + ('stat.S_IREAD | stat.S_IWRITE', stat.S_IREAD | stat.S_IWRITE), + ('stat.S_IREAD | stat.S_IWRITE | S_IXUSR', stat.S_IREAD | stat.S_IWRITE | stat.S_IXUSR), ), ) def test_chmod_get_mode(self, s, expected): node = astroid.extract_node(s + ' #@') assert pylint_scs._chmod_get_mode(node) == expected + @pytest.mark.parametrize( + 's', + ( + 'stat.ST_MODE', + 'bla.S_IREAD', + ), + ) + def test_chmod_get_mode_invalid(self, s): + node = astroid.extract_node(s + ' #@') + with pytest.raises(ValueError): + pylint_scs._chmod_get_mode(node) + + @pytest.mark.parametrize( + 's, expected', + ( + ('-stat.S_IREAD', -stat.S_IREAD), + ('not stat.S_IREAD', not stat.S_IREAD), + ), + ) + def test_chmod_get_mode_unop(self, s, expected): + node = astroid.extract_node(s + ' #@') + assert pylint_scs._chmod_get_mode(node) == expected + + @pytest.mark.parametrize( + 's, expected', + ( + ('stat.S_IREAD + stat.S_IWRITE', stat.S_IREAD + stat.S_IWRITE), + ('stat.S_IREAD - stat.S_IWRITE', stat.S_IREAD - stat.S_IWRITE), + ('stat.S_IREAD * stat.S_IWRITE', stat.S_IREAD * stat.S_IWRITE), + ('stat.S_IREAD / stat.S_IWRITE', stat.S_IREAD / stat.S_IWRITE), + ('stat.S_IREAD // stat.S_IWRITE', stat.S_IREAD // stat.S_IWRITE), + ('stat.S_IREAD % stat.S_IWRITE', stat.S_IREAD % stat.S_IWRITE), + ('stat.S_IREAD ^ stat.S_IWRITE', stat.S_IREAD ^ stat.S_IWRITE), + ('stat.S_IREAD | stat.S_IWRITE', stat.S_IREAD | stat.S_IWRITE), + ('stat.S_IREAD & stat.S_IWRITE', stat.S_IREAD & stat.S_IWRITE), + ), + ) + def test_chmod_get_mode_binop(self, s, expected): + node = astroid.extract_node(s + ' #@') + assert pylint_scs._chmod_get_mode(node) == expected + @pytest.mark.parametrize( 'platform, enabled_platform', ( @@ -52,7 +94,7 @@ def test_chmod_get_mode(self, s, expected): @pytest.mark.parametrize( 'forbidden', ( - 'S_IRGRP', + 'S_IRGRP', # NB: not actually a forbidden value, only for testing... 'S_IRWXG', 'S_IWGRP', 'S_IXGRP', @@ -88,6 +130,23 @@ def test_chmod(self, mocker, platform, enabled_platform, fname, arg_type, forbid with self.assertNoMessages(): self.checker.visit_call(node) + @pytest.mark.parametrize('platform', ('Linux', 'Darwin', 'Java', 'Windows')) + @pytest.mark.parametrize( + 's', + ( + 'os.chmod("file.txt", stat.ST_MODE)', + 'os.chmod("file.txt", other.S_IRWXO)', + 'os.chmod("file.txt", mode)', + 'os.chmod("file.txt", mode=mode)', + ), + ) + def test_chmod_no_warning(self, mocker, platform, s): + mocker.patch('platform.system', lambda: platform) + + node = astroid.extract_node(s) + with self.assertNoMessages(): + self.checker.visit_call(node) + @pytest.mark.parametrize( 'platform, enabled_platform', ( @@ -98,12 +157,10 @@ def test_chmod(self, mocker, platform, enabled_platform, fname, arg_type, forbid ), ) @pytest.mark.parametrize('s', ('os.chmod("file")',)) - def test_chmod_invalid(self, mocker, platform, enabled_platform, s): + def test_chmod_invalid_raise(self, mocker, platform, enabled_platform, s): mocker.patch('platform.system', lambda: platform) - print(s) node = astroid.extract_node(s) - if enabled_platform: with pytest.raises(RuntimeError): self.checker.visit_call(node) From a31b718abb6163edd07a63de93092d0fa9d9973f Mon Sep 17 00:00:00 2001 From: Nguyen Damien Date: Wed, 28 Jul 2021 22:32:38 +0200 Subject: [PATCH 6/6] Add missing logical NOT operator (~) --- pylint_secure_coding_standard.py | 2 +- tests/os_chmod_test.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pylint_secure_coding_standard.py b/pylint_secure_coding_standard.py index b10a3b0..78ba537 100644 --- a/pylint_secure_coding_standard.py +++ b/pylint_secure_coding_standard.py @@ -283,7 +283,7 @@ def _is_yaml_unsafe_call(node): # ============================================================================== -_unop = {'-': operator.neg, 'not': operator.not_} +_unop = {'-': operator.neg, 'not': operator.not_, '~': operator.inv} _binop = { '+': operator.add, '-': operator.sub, diff --git a/tests/os_chmod_test.py b/tests/os_chmod_test.py index ad28258..5f2ba94 100644 --- a/tests/os_chmod_test.py +++ b/tests/os_chmod_test.py @@ -55,6 +55,7 @@ def test_chmod_get_mode_invalid(self, s): 's, expected', ( ('-stat.S_IREAD', -stat.S_IREAD), + ('~stat.S_IREAD', ~stat.S_IREAD), ('not stat.S_IREAD', not stat.S_IREAD), ), )