Add file permission check for pathlib chmod #1043
Conversation
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
costaparas
requested review from
ericwb,
lukehinds and
sigmavirus24
as code owners
| mode = context.get_call_arg_at_position(1) | ||
| elif context.call_args_count == 1: # pathlib chmod |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- 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)
- Reduce the confidence (either to medium or low) for the new case.
- 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.
There was a problem hiding this comment.
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.
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