Incorrect result for B202:tarfile_unsafe_members

behnazh-w commented 1 year ago

Describe the bug

The B202:tarfile_unsafe_members documentation says to pass a callable as the members argument but that’s not supported in the official type signature and not implemented in CPython stdlib. members should be used as an Iterable[TarInfo] instead.

That change was introduced in v1.7.5 based on issue #207 and PR #549 cc @yilmi @ericwb @lukehinds @sigmavirus24

The following fixes are required to address this bug:

The tarfile.extractalll(members=function(tarfile)) - LOW suggestion here seems to be wrong.
The check on ast.Call node here should be fixed/removed.
The extractall function name look up here is too coarse and can easily result in inaccurate results for other libraries that have the same function names, e.g., ZipFile.extractall.

Reproduction steps

This PR addresses the B202:tarfile_unsafe_members by validating members Iterable argument but Bandit cannot detect the filtering of members used to fix the issue (hence the need to suppress the error)

Expected behavior

The check on ast.Call node here should be fixed/removed. We should not assume the members argument to have a Callable type.

Bandit version

1.7.5 (Default)

Python version

3.11 (Default)

Additional context

No response

sigmavirus24 commented 1 year ago

The tarfile.extractalll(members=function(tarfile)) - LOW suggestion here seems to be wrong.

Looking at typeshed which are user contributed and not official, it looks like we should be looking for the filter parameter, not members to be callable.

That said, either specifying a members iterable or a filter function/literal

The extractall function name look up here is too coarse and can easily result in inaccurate results for other libraries that have the same function names, e.g., ZipFile.extractall.

Briefly looking at our code it looks like we have different branches for zip and tar. Regardless if we're expecting members to be a callable there too, then that's also wrong.

This should be fixed, not removed

mattiasb commented 9 months ago

I believe this should be safe as well:

tarfile.extractall(path=some_path, filter="data")

See Extraction Filters and tarfile.data_filter from the tarfile docs.

eschalkargans commented 7 months ago

Hello,

I use bandit as a part of the continuous integration tool-chain in a project. I get also the same issue. I initially understood that I might be safe with filter="data", is it really the case?

My offending line:

tf.extractall(path=output_path, filter="data")

Test results:
>> Issue: [B202:tarfile_unsafe_members] tarfile.extractall used without any validation. Please check and discard dangerous members.
   Severity: High   Confidence: High
   CWE: CWE-22 (https://cwe.mitre.org/data/definitions/22.html)
   More Info: https://bandit.readthedocs.io/en/0.0.0/plugins/b202_tarfile_unsafe_members.html

It is unclear to me how I should fix the issue. The link https://bandit.readthedocs.io/en/0.0.0/plugins/b202_tarfile_unsafe_members.html is not working.

It seems that the source is: https://github.com/PyCQA/bandit/blob/main/bandit/plugins/tarfile_unsafe_members.py

Should filter="data" be enough?

Thanks!

mattiasb commented 7 months ago

Hello,

I use bandit as a part of the continuous integration tool-chain in a project. I get also the same issue. I initially understood that I might be safe with filter="data", is it really the case?

To the best of my knowledge it is safe (with the caveat that "safe", "security" etc is a gray scale).

Bandit will complain though because of the bug in this issue.

My offending line:

tf.extractall(path=output_path, filter="data")
Test results:
>> Issue: [B202:tarfile_unsafe_members] tarfile.extractall used without any validation. Please check and discard dangerous members.
   Severity: High   Confidence: High
   CWE: CWE-22 (https://cwe.mitre.org/data/definitions/22.html)
   More Info: https://bandit.readthedocs.io/en/0.0.0/plugins/b202_tarfile_unsafe_members.html
It is unclear to me how I should fix the issue. The link https://bandit.readthedocs.io/en/0.0.0/plugins/b202_tarfile_unsafe_members.html is not working.

The issue can only be fixed inside Bandit. The code you wrote should be safe.

The "fix" is basically to do what you did, slap a # nosec on the "offending" line and wait for someone to fix this issue (or take a stab at it yourself).

PyCQA / bandit