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.

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

Add file permission check for pathlib chmod #1043

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

Conversation

costaparas
Copy link
Contributor

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

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 PyCQA#1042
Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

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

Nit newline change requested.

examples/os-chmod.py Outdated Show resolved Hide resolved
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

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.

This strengthens the check to reduce false positives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account for pathlib chmod setting insecure permissions
2 participants