Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add file permission check for pathlib chmod #1043

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add file permission check for pathlib chmod #1043

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91b2179
Add file permission check for pathlib chmod
costaparas Aug 16, 2023
17fa758
Add newline
costaparas Aug 16, 2023
8c0c501
Check if pathlib is imported
costaparas Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bandit/core/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
74 changes: 46 additions & 28 deletions bandit/plugins/general_bad_file_permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::

Expand All @@ -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

Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this may result in some false positives. Any function named chmod with one arg will end up here. While, I can think of some other cases in other checks as well, there's not a better way to handle this at the moment. Ideally a symbol table that tracks instances of certain types would result in more accuracy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be quite a few false positives on any function containing "chmod". Not sure how prevalent this would be, but function names like "lunchmode", "chmodulus", "chmodel", etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately even the current version of this test is not restrictive enough. The following code is enough to trigger a high severity issue in the current version of bandit:

def chmod(a, b):
    pass

chmod("file.txt", 0x777)

I've fixed this by checking the fully qualified name. But as you say, its trickier to solve in the case of pathlib.

I see a few possibilities:

  1. Accept that some false positives could occur - this would already be the case for the current implementation anyway (although now it would be cases of 1 arg functions called chmod as opposed to 2 arg functions)
  2. Reduce the confidence (either to medium or low) for the new case.
  3. Put this PR on hold and do it properly, noting that the current bandit core is somewhat limiting in this regard.

I see (3) as a longer-term goal because it would be needed as more advanced tests are added.

In the meantime, I've also added a missing check that pathlib is actually in the current context, which should help reduce false positives as well.

Copy link
Contributor Author

@costaparas costaparas Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be quite a few false positives on any function containing "chmod". Not sure how prevalent this would be, but function names like "lunchmode", "chmodulus", "chmodel", etc

In addition, we could update the check so that the call_function_name is explicitly one of chmod, lchmod, fchmod. That will at least narrow down the scope for false positives ... but this has the disadvantage of not accounting new chmod functions that could be added in future.

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),
)
2 changes: 2 additions & 0 deletions examples/os-chmod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
costaparas marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 9 additions & 0 deletions examples/pathlib-chmod.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import pathlib

filename = 'foobar'
p1 = pathlib.Path(filename)
p1.chmod(0o666)

symlink = 'link'
p2 = pathlib.Path(symlink)
p2.lchmod(0o777)
12 changes: 10 additions & 2 deletions tests/functional/test_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
Loading