ruff icon indicating copy to clipboard operation
ruff copied to clipboard

Published 20 hours ago • verified publisher icon astral-sh

Implement `flake8-bandit`

Open charliermarsh opened this issue 2 years ago • 12 comments

charliermarsh avatar Jan 05 '23 00:01 charliermarsh

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

charliermarsh avatar Jan 05 '23 00:01 charliermarsh

thanks @charliermarsh!

edgarrmondragon avatar Jan 05 '23 00:01 edgarrmondragon

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:

  1. 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).
  2. 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.

charliermarsh avatar Jan 05 '23 00:01 charliermarsh

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:

  1. 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).
  2. 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.

edgarrmondragon avatar Jan 05 '23 00:01 edgarrmondragon

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

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

ahmedbilal avatar Jan 10 '23 08:01 ahmedbilal

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

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

charliermarsh avatar Jan 10 '23 13:01 charliermarsh

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

colin99d avatar Jan 12 '23 18:01 colin99d

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.

colin99d avatar Jan 12 '23 18:01 colin99d

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

charliermarsh avatar Jan 12 '23 19:01 charliermarsh

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

ngnpope avatar Feb 01 '23 20:02 ngnpope

@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 👍

edgarrmondragon avatar Feb 01 '23 20:02 edgarrmondragon

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.

ngnpope avatar Feb 01 '23 20:02 ngnpope

It looks like B109 and B111 have been deprecated.

colin99d avatar Feb 08 '23 17:02 colin99d

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

colin99d avatar Feb 26 '23 16:02 colin99d

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

charliermarsh avatar Feb 26 '23 16:02 charliermarsh

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

colin99d avatar Feb 26 '23 16:02 colin99d

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

charliermarsh avatar Feb 26 '23 16:02 charliermarsh

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.

colin99d avatar Feb 26 '23 16:02 colin99d

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.

charliermarsh avatar Feb 26 '23 17:02 charliermarsh

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

colin99d avatar Feb 26 '23 17:02 colin99d

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?

martinlehoux avatar Mar 13 '23 17:03 martinlehoux

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)

ThunderKey avatar Apr 21 '23 07:04 ThunderKey

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

charliermarsh avatar May 02 '23 06:05 charliermarsh

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: https://github.com/tylerwince/flake8-bandit/issues/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)

Daverball avatar May 02 '23 08:05 Daverball

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

charliermarsh avatar May 02 '23 15:05 charliermarsh

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?

MichaReiser avatar May 03 '23 06:05 MichaReiser

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?

ngnpope avatar May 03 '23 07:05 ngnpope

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.

Daverball avatar May 03 '23 08:05 Daverball

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

scop avatar May 19 '23 08:05 scop

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?

mkniewallner avatar Sep 23 '23 14:09 mkniewallner