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 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 diff --git a/pylint_secure_coding_standard.py b/pylint_secure_coding_standard.py index f1f46d6..78ba537 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 @@ -279,6 +281,93 @@ def _is_yaml_unsafe_call(node): return False +# ============================================================================== + +_unop = {'-': operator.neg, 'not': operator.not_, '~': operator.inv} +_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): + """ + Extract the mode constant of a 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) 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 _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 + + 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)) + + # ============================================================================== @@ -429,6 +518,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 +573,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..5f2ba94 --- /dev/null +++ b/tests/os_chmod_test.py @@ -0,0 +1,170 @@ +# -*- 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 stat + +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', 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), + ('~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', + ( + ('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', # NB: not actually a forbidden value, only for testing... + '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', ('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', + ( + ('Linux', True), + ('Darwin', True), + ('Java', True), + ('Windows', False), + ), + ) + @pytest.mark.parametrize('s', ('os.chmod("file")',)) + def test_chmod_invalid_raise(self, mocker, platform, enabled_platform, s): + mocker.patch('platform.system', lambda: platform) + + 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)