From 91b21796568d42630eb6ba58ed55245e738778ab Mon Sep 17 00:00:00 2001 From: Costa Paraskevopoulos Date: Wed, 16 Aug 2023 14:37:27 +1000 Subject: [PATCH 1/3] Add file permission check for pathlib chmod This extends the existing implementation for detecting bad file permissions to account for calls to pathlib module functions in addition to those from the os module. The pathlib chmod and lchmod functions are really just wrappers around the os module equivalents. However, since they are class methods, the pre-existing logic in the code did not consider the corresponding pathlib function calls. Note that the filename is not easily parsable in the case of pathlib. Resolves #1042 --- bandit/core/utils.py | 2 +- .../plugins/general_bad_file_permissions.py | 74 ++++++++++++------- examples/os-chmod.py | 2 + examples/pathlib-chmod.py | 9 +++ tests/functional/test_functional.py | 12 ++- 5 files changed, 68 insertions(+), 31 deletions(-) create mode 100644 examples/pathlib-chmod.py diff --git a/bandit/core/utils.py b/bandit/core/utils.py index 32d9d4965..ffeeb019c 100644 --- a/bandit/core/utils.py +++ b/bandit/core/utils.py @@ -19,7 +19,7 @@ def _get_attr_qual_name(node, aliases): - """Get a the full name for the attribute node. + """Get the full name for the attribute node. This will resolve a pseudo-qualified name for the attribute rooted at node as long as all the deeper nodes are Names or diff --git a/bandit/plugins/general_bad_file_permissions.py b/bandit/plugins/general_bad_file_permissions.py index 7d3fce4df..a1c5b0ffa 100644 --- a/bandit/plugins/general_bad_file_permissions.py +++ b/bandit/plugins/general_bad_file_permissions.py @@ -21,21 +21,28 @@ .. code-block:: none - >> Issue: Probable insecure usage of temp file/directory. - Severity: Medium Confidence: Medium + >> Issue: Chmod setting a permissive mask 0o664 on file (/etc/passwd). + Severity: Medium Confidence: High CWE: CWE-732 (https://cwe.mitre.org/data/definitions/732.html) - Location: ./examples/os-chmod.py:15 - 14 os.chmod('/etc/hosts', 0o777) - 15 os.chmod('/tmp/oh_hai', 0x1ff) - 16 os.chmod('/etc/passwd', stat.S_IRWXU) + Location: ./examples/os-chmod.py:8 + 7 os.chmod('/etc/passwd', 0o7) + 8 os.chmod('/etc/passwd', 0o664) + 9 os.chmod('/etc/passwd', 0o777) - >> Issue: Chmod setting a permissive mask 0777 on file (key_file). + >> Issue: Chmod setting a permissive mask 0777 on file (keyfile). Severity: High Confidence: High CWE: CWE-732 (https://cwe.mitre.org/data/definitions/732.html) Location: ./examples/os-chmod.py:17 16 os.chmod('/etc/passwd', stat.S_IRWXU) - 17 os.chmod(key_file, 0o777) - 18 + 17 os.chmod(keyfile, 0o777) + 18 os.chmod('~/hidden_exec', stat.S_IXGRP) + + >> Issue: Chmod setting a permissive mask 0o666 on file (NOT PARSED). + Severity: High Confidence: High + CWE: CWE-732 (https://cwe.mitre.org/data/definitions/732.html) + Location: ./examples/pathlib-chmod.py:5 + 4 p1 = pathlib.Path(filename) + 5 p1.chmod(0o666) .. seealso:: @@ -52,6 +59,9 @@ .. versionchanged:: 1.7.5 Added checks for S_IWGRP and S_IXOTH +.. versionchanged:: 1.7.6 + Added check for pathlib chmod + """ # noqa: E501 import stat @@ -73,27 +83,35 @@ def _stat_is_dangerous(mode): @test.test_id("B103") def set_bad_file_permissions(context): if "chmod" in context.call_function_name: - if context.call_args_count == 2: + if ( + context.call_function_name_qual.startswith("os.") + and context.call_args_count == 2 + ): # os chmod + filename = context.get_call_arg_at_position(0) mode = context.get_call_arg_at_position(1) + elif context.call_args_count == 1: # pathlib chmod + filename = None + mode = context.get_call_arg_at_position(0) + else: + return - if ( + if ( mode is not None and isinstance(mode, int) and _stat_is_dangerous(mode) - ): - # world writable is an HIGH, group executable is a MEDIUM - if mode & stat.S_IWOTH: - sev_level = bandit.HIGH - else: - sev_level = bandit.MEDIUM - - filename = context.get_call_arg_at_position(0) - if filename is None: - filename = "NOT PARSED" - return bandit.Issue( - severity=sev_level, - confidence=bandit.HIGH, - cwe=issue.Cwe.INCORRECT_PERMISSION_ASSIGNMENT, - text="Chmod setting a permissive mask %s on file (%s)." - % (oct(mode), filename), - ) + ): + # world writable is an HIGH, group executable is a MEDIUM + if mode & stat.S_IWOTH: + sev_level = bandit.HIGH + else: + sev_level = bandit.MEDIUM + + if filename is None: + filename = "NOT PARSED" + return bandit.Issue( + severity=sev_level, + confidence=bandit.HIGH, + cwe=issue.Cwe.INCORRECT_PERMISSION_ASSIGNMENT, + text="Chmod setting a permissive mask %s on file (%s)." + % (oct(mode), filename), + ) diff --git a/examples/os-chmod.py b/examples/os-chmod.py index f7fff8517..5f3f33694 100644 --- a/examples/os-chmod.py +++ b/examples/os-chmod.py @@ -17,3 +17,5 @@ os.chmod(keyfile, 0o777) os.chmod('~/hidden_exec', stat.S_IXGRP) os.chmod('~/hidden_exec', stat.S_IXOTH) +os.fchmod(keyfile, 0o777) +os.lchmod('symlink', 0o777) \ No newline at end of file diff --git a/examples/pathlib-chmod.py b/examples/pathlib-chmod.py new file mode 100644 index 000000000..f7c014e2c --- /dev/null +++ b/examples/pathlib-chmod.py @@ -0,0 +1,9 @@ +import pathlib + +filename = 'foobar' +p1 = pathlib.Path(filename) +p1.chmod(0o666) + +symlink = 'link' +p2 = pathlib.Path(symlink) +p2.lchmod(0o777) diff --git a/tests/functional/test_functional.py b/tests/functional/test_functional.py index 7835e7488..dc5bf6bcb 100644 --- a/tests/functional/test_functional.py +++ b/tests/functional/test_functional.py @@ -297,11 +297,19 @@ def test_subdirectory_okay(self): } self.check_example("init-py-test/subdirectory-okay.py", expect) + def test_pathlib_chmod(self): + """Test setting file permissions.""" + expect = { + "SEVERITY": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 0, "HIGH": 2}, + "CONFIDENCE": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 0, "HIGH": 2}, + } + self.check_example("pathlib-chmod.py", expect) + def test_os_chmod(self): """Test setting file permissions.""" expect = { - "SEVERITY": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 4, "HIGH": 8}, - "CONFIDENCE": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 1, "HIGH": 11}, + "SEVERITY": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 4, "HIGH": 10}, + "CONFIDENCE": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 1, "HIGH": 13}, } self.check_example("os-chmod.py", expect) From 17fa75856a2dd60891d1e1f0b68a09f7e6b9f76f Mon Sep 17 00:00:00 2001 From: Costa Paraskevopoulos Date: Thu, 17 Aug 2023 09:56:55 +1000 Subject: [PATCH 2/3] Add newline --- examples/os-chmod.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/os-chmod.py b/examples/os-chmod.py index 5f3f33694..24e7e9050 100644 --- a/examples/os-chmod.py +++ b/examples/os-chmod.py @@ -18,4 +18,4 @@ os.chmod('~/hidden_exec', stat.S_IXGRP) os.chmod('~/hidden_exec', stat.S_IXOTH) os.fchmod(keyfile, 0o777) -os.lchmod('symlink', 0o777) \ No newline at end of file +os.lchmod('symlink', 0o777) From 8c0c50181987580cc66668b48cef87557325ed1b Mon Sep 17 00:00:00 2001 From: Costa Paraskevopoulos Date: Thu, 17 Aug 2023 10:11:24 +1000 Subject: [PATCH 3/3] Check if pathlib is imported This strengthens the check to reduce false positives. --- bandit/plugins/general_bad_file_permissions.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/bandit/plugins/general_bad_file_permissions.py b/bandit/plugins/general_bad_file_permissions.py index a1c5b0ffa..2b3700849 100644 --- a/bandit/plugins/general_bad_file_permissions.py +++ b/bandit/plugins/general_bad_file_permissions.py @@ -84,21 +84,24 @@ def _stat_is_dangerous(mode): def set_bad_file_permissions(context): if "chmod" in context.call_function_name: if ( - context.call_function_name_qual.startswith("os.") - and context.call_args_count == 2 + context.call_function_name_qual.startswith("os.") + and context.call_args_count == 2 ): # os chmod filename = context.get_call_arg_at_position(0) mode = context.get_call_arg_at_position(1) - elif context.call_args_count == 1: # pathlib chmod + elif ( + context.is_module_imported_like("pathlib") + and context.call_args_count == 1 # pathlib chmod + ): filename = None mode = context.get_call_arg_at_position(0) else: return if ( - mode is not None - and isinstance(mode, int) - and _stat_is_dangerous(mode) + mode is not None + and isinstance(mode, int) + and _stat_is_dangerous(mode) ): # world writable is an HIGH, group executable is a MEDIUM if mode & stat.S_IWOTH: