Implement `flake8-bandit`

Question

Implement `flake8-bandit`

charliermarsh opened this issue a year ago · 38 comments

Answer 1 · 2023-01-05T00:12:12.000Z

\cc @edgarrmondragon - easier to track here than to keep incrementing the count in the README. I'll populate the checklist.

Answer 2 · 2023-01-05T00:34:58.000Z

thanks @charliermarsh!

Answer 3 · 2023-01-05T00:36:50.000Z

By the way: the scripts are pretty bad right now, but it might save you some time (and would love help improving):

python scripts/add_check.py --name ConvertLoopToAll --code SIM111 --plugin flake8-simplify

The main limitations right now are:

Doesn't keep the members sorted in registry.rs, so has to be manually resorted (else checks appear out-of-order in the docs and elsewhere).
Assumes you're using a single plugins.rs file, instead of individual files for each check or a checks.rs file or whatever else, so that also requires manual tweaks.

Answer 4 · 2023-01-05T00:40:21.000Z

By the way: the scripts are pretty bad right now, but it might save you some time (and would love help improving):
python scripts/add_check.py --name ConvertLoopToAll --code SIM111 --plugin flake8-simplify
The main limitations right now are:

Doesn't keep the members sorted in registry.rs, so has to be manually resorted (else checks appear out-of-order in the docs and elsewhere).

Assumes you're using a single plugins.rs file, instead of individual files for each check or a checks.rs file or whatever else, so that also requires manual tweaks.

Thanks, I'll take a look at those. They might be really helpful for bandit's blacklist tests.

Answer 5 · 2023-01-10T08:58:43.000Z

Please implement the configurations for it as well e.g exclude_dirs.

https://bandit.readthedocs.io/en/latest/config.html#bandit-settings

Answer 6 · 2023-01-10T13:02:49.000Z

@ahmedbilal - I think exclude_dirs can be accomplished with the per-file-ignores settings, like:

[tool.ruff.per-file-ignores]
"excluded_dir" = ["S"]

Answer 7 · 2023-01-12T18:29:16.000Z

Not sure if this is the best place to ask, but do flake8-bandit and bandit have the same linters?

Answer 8 · 2023-01-12T18:30:10.000Z

Also @charliermarsh do you think it would be helpful to pin all of the issues that are trackers for implementing a package to the top? It would make it easier for people to understand our progress, and to contribute to one.

Answer 9 · 2023-01-12T19:54:02.000Z

Not sure if this is the best place to ask, but do flake8-bandit and bandit have the same linters?

Yeah, I believe flake8-bandit is just a wrapper around bandit to make it compatible with Flake8: https://github.com/tylerwince/flake8-bandit/blob/3200f00bf34a8ff131139add500db3e62ee1e7fd/flake8_bandit.py#L9

Answer 10 · 2023-02-01T20:03:18.000Z

@charliermarsh It looks like you've only added the rules from here to the list above, but missed those from here?

Answer 11 · 2023-02-01T20:06:58.000Z

@charliermarsh It looks like you've only added the rules from here to the list above, but missed those from here?

@ngnpope I mentioned the blacklist plugins above, and yeah they wouldn't make the list much larger 👍

Answer 12 · 2023-02-01T20:13:46.000Z

Thanks. I just came across this as a # noqa: S301 was flagged as unknown by RUF100. Added to external for now.
Have checked existing issues and opened new ones as I've found gaps in coverage, bugs, etc.

Answer 13 · 2023-02-08T17:43:21.000Z

It looks like B109 and B111 have been deprecated.

Answer 14 · 2023-02-26T16:02:56.000Z

I will tackle the blacklist_calls and blacklist_imports that @ngnpope mentioned.

Answer 15 · 2023-02-26T16:05:41.000Z

Does that behavior not overlap with our existing banned-api rule?

Answer 16 · 2023-02-26T16:30:52.000Z

Yes it does look like they perform the same operations, should I just add these to the default ones called?

Answer 17 · 2023-02-26T16:47:37.000Z

My instinct is that we should just leave folks to adjust that configuration themselves rather than encode defaults (i.e. do nothing).

Answer 18 · 2023-02-26T16:53:58.000Z

It seems like we should have some way of letting people know or giving them an easy way to add it. As someone who had been using the bandit package for years, I had no idea it checked this stuff specifically, and would have never thought to configure ruff to add it. Maybe I just need to know more about the packages I use, but this seems like a big loss in the value of bandit.

Answer 19 · 2023-02-26T17:04:13.000Z

I think I misunderstood -- I thought this was something that was user-configured, but it's a list of built-in items to flag. I'm open to including those as regular rules similar to Bandit. I didn't want to have something that was yet-again user configurable for the same purpose.

Answer 20 · 2023-02-26T17:05:58.000Z

I see, yeah I was not planning on making the user configurable either. I will make it a separate rule!

Answer 21 · 2023-03-13T17:36:05.000Z

I'd like to try S703: django_mark_safe , but is there the infrastructure to go check the risk of a variable across modules like bandit does?

Answer 22 · 2023-04-21T07:58:27.000Z

I've compared the list of tests reported by bandit --help (version: 1.7.5) with the ones listed in this issue. The following seem to be missing:

Are these omitted intentionally?

If you want a list to copy:

* [x] `S310`: [`urllib_urlopen`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_calls.html#b310-urllib-urlopen)
* [ ] `S401`: [`import_telnetlib`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b401-import-telnetlib)
* [ ] `S402`: [`import_ftplib`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b402-import-ftplib)
* [ ] `S403`: [`import_pickle`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b403-import-pickle)
* [ ] `S404`: [`import_subprocess`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b404-import-subprocess)
* [ ] `S405`: [`import_xml_etree`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b405-import-xml-etree)
* [ ] `S406`: [`import_xml_sax`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b406-import-xml-sax)
* [ ] `S407`: [`import_xml_expat`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b407-import-xml-expat)
* [ ] `S408`: [`import_xml_minidom`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b408-import-xml-minidom)
* [ ] `S409`: [`import_xml_pulldom`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b409-import-xml-pulldom)
* [ ] `S410`: [`import_lxml`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b410-import-lxml)
* [ ] `S411`: [`import_xmlrpclib`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b411-import-xmlrpclib)
* [ ] `S412`: [`import_httpoxy`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b412-import-httpoxy)
* [ ] `S413`: [`import_pycrypto`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b413-import-pycrypto)
* [ ] `S415`: [`import_pyghmi`](https://bandit.readthedocs.io/en/latest/blacklists/blacklist_imports.html#b415-import-pyghmi)

Answer 23 · 2023-05-02T06:18:03.000Z

@ThunderKey - No, I think we just missed them since they're listed under a separate page in the docs -- adding now, thanks!

Answer 24 · 2023-05-02T08:47:25.000Z

For interoperability between the reference implementation through direct use of bandit and ruff it would be nice to support nosec comments in addition to noqa. There is also an open issue for this on flake8-bandit: tylerwince/flake8-bandit#20

Otherwise you need redundant comments to ignore the error both in ruff and bandit. (This would allow one to use ruff locally for speed but bandit on CI for correctness/completeness)

Answer 25 · 2023-05-02T15:56:22.000Z

I think you're right that we should support nosec comments. It's similar to our support for isort's comment directives.

Answer 26 · 2023-05-03T06:50:12.000Z

I think you're right that we should support nosec comments. It's similar to our support for isort's comment directives.

I do see value in supporting nosec suppression comments to ease adoption but it has the disadvantage that Ruff suddenly supports multiple ways for suppressing a violation rather than just one. Having multiple ways to achieve the same can be confusing to users and it may not be clear to them when they should use which suppression comment format, resulting in them likely using all formats in their code base.

Supporting multiple suppression comments also raises the question of what format ruff should use when running with --add-noqa or when adding suppression comments using the code actions in the IDE.

I don't have good answers to this and we'll face the very same problem if we ever decide to introduce a ruff-specific suppression format. Would ruff continue to support other suppression comment formats or would we provide an automated migration tool?

Answer 27 · 2023-05-03T07:56:25.000Z

I have the same concerns. A migration tool would make a lot of sense. I can however see the value of supporting existing suppression comments from other tools to ease migration. Maybe this support should be behind a configuration flag?

Answer 28 · 2023-05-03T08:31:16.000Z

While I do agree this is a commendable goal in most cases, in this specific case I would have to disagree, since bandit is a security linter, a lot of users would probably prefer to keep using the reference implementation on their CI, to rule out any issues in transposing the rules from Python to Rust. (I would definitely count myself among those)

The speed benefit is far more significant in local development, so you are willing to take the risk of some false negatives there, but on the CI the concerns flip and you want it to catch everything.

So if you force people to migrate to a different style of rules they either have to use them both redundantly or not use them at all locally and only use them on the CI or setup a separate flake8 config just so it can be ignored using the flake8-bandit rule names. None of which seem very appealing to me. I think it is folly to take flake8-bandit as the reference here, rather than bandit itself. I would treat bandit the same way as isort is treated.

I don't think the risk of confusion is as high if you treat the noqa comments as temporary and the nosec as solid. nosec is also honestly just better documentation than a code starting with S, that you will just skip over because you are only looking at the noqa part of it. I think it's fair to be more demanding of users in the case of potential security concerns.

Answer 29 · 2023-05-19T08:31:50.000Z

Some updates:

S310 is not in its sorted place in the list :)
Could add S325 for completeness, even though it has been removed in bandit, so probably not worth implementing in ruff either
S601 is done per #4500
S609 WIP per #4504

Answer 30 · 2023-09-23T14:32:30.000Z

I'm open to work on all the import_* rules from blacklist_imports, but I wonder if they all are that useful, given that some kind of overlap with blacklist_calls from bandit. Since the blacklist_calls rules check specific usages of modules attributes (or for ftplib and telnetlib, any attribute) known to be prone to security issues, I'm not sure that it is that useful to also report issues for the imports, which alone are not problematic, until we use specific attributes from those imports (which are checked by blacklist_calls). Note that some modules reported in blacklist_imports (like pyghmi) are not in blacklist_calls, so for those it might still make sense to implement the rules, but for the other ones, I'm not really sure.

Since Ruff does not handle the severity on reported issues, it probably makes even less sense right now, as in bandit, one could deactivate some specifics severity levels (for instance, low ones, ending up with only blacklist_calls and not blacklist_imports).

For reference, for the given file:

# foo.py
from xml.etree import ElementTree

ElementTree.parse("")

In bandit, you would get:

$ pipx run bandit==1.7.5 foo.py
[...]
Test results:
>> Issue: [B405:blacklist] Using ElementTree to parse untrusted XML data is known to be vulnerable to XML attacks. Replace ElementTree with the equivalent defusedxml package, or make sure defusedxml.defuse_stdlib() is called.
   Severity: Low   Confidence: High
   CWE: CWE-20 (https://cwe.mitre.org/data/definitions/20.html)
   More Info: https://bandit.readthedocs.io/en/1.7.5/blacklists/blacklist_imports.html#b405-import-xml-etree
   Location: foo.py:1:0
1	from xml.etree import ElementTree
2
3	ElementTree.parse("")

--------------------------------------------------
>> Issue: [B314:blacklist] Using xml.etree.ElementTree.parse to parse untrusted XML data is known to be vulnerable to XML attacks. Replace xml.etree.ElementTree.parse with its defusedxml equivalent function or make sure defusedxml.defuse_stdlib() is called
   Severity: Medium   Confidence: High
   CWE: CWE-20 (https://cwe.mitre.org/data/definitions/20.html)
   More Info: https://bandit.readthedocs.io/en/1.7.5/blacklists/blacklist_calls.html#b313-b320-xml-bad-elementtree
   Location: foo.py:3:0
2
3	ElementTree.parse("")

By ignoring low severity issues, bandit users would only get reports for blacklist_calls. In Ruff, users would have to deactivate all B3 rules.

What is the general stance on rules implementation? Do we prefer to implement rules from plugins as-is, and let users decide if the rules are useful (and disable them if they don't), or are some rules open to discussion depending on how useful or how close to existing rules they are?

Answer 31 · 2023-11-06T14:44:12.000Z

I am also interested in picking up some of the leftover rules. Has any decision been made on the S4XX import rules yet? Also how does Ruff deal with Bandit's severities? E.g. if I wanted to implement S202 (source docs) is there a default severity level that Ruff ports? Or do we split it into 3 separate smaller rules?

Clicked through some ported rules and it looks like S103 has a med and high version and Ruff just ports both in the same rule but for something like S202 that might be a bit aggressive.

Answer 32 · 2023-11-26T08:10:51.000Z

These rules have already been implemented:
S201: flask_debug_true
S601: paramiko_calls
S609: linux_commands_wildcard_injection

Answer 33 · 2023-12-14T23:18:54.000Z

@charliermarsh I think is more a generic question rather than specific to this rule set, but are you implementing them with the exact same behaviour of the originals?

I am getting the same (odd) behaviour in ruff and flake8

❯ ruff ruff_test.py
ruff_test.py:4:1: S311 Standard pseudo-random generators are not suitable for security/cryptographic purposes.

With this sample code:

import random
import time

time.sleep(random.choice(range(1, 5)))

I was wondering if this behaviour is going to be reviewed or fixed in Ruff? Thanks!

Answer 34 · 2023-12-15T07:24:39.000Z

@pygaiwan This is the intended behavior of that rule and not odd at all. The random module is not safe, if you care about security, since the random sequences are predictable. Whether or not you use random in a place where security matters is not the linters' job to figure out and also quite impossible to do reliably in a dynamic language like Python. There's a secrets module which contains some of the same functions as random but implemented in a more secure way.

Bandit is a security linter so its goal is to have essentially zero false negatives, at the cost of some false positives, so it's quite common that you will have to review bandit errors and decide whether or not it's an actual issue and put a noqa/nosec comment, when you decide it isn't a problem.

You can also globally disable rules you don't care about.

Answer 35 · 2024-01-03T18:19:30.000Z

Five rules left :)

Answer 36 · 2024-01-13T00:20:08.000Z

S503 was addressed by #9391 so there's only S610 and S703 left!

Answer 37 · 2024-01-16T15:15:10.000Z

How is S703 different from S308, by the way? They seem redundant to me.

Answer 38 · 2024-01-23T21:30:55.000Z

How is S703 different from S308, by the way? They seem redundant to me.

Per PyCQA/bandit#294 (comment), it would seem so. Would seem like S308 was first and then S703 covered S308 but did not want to have a breaking change.