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

Implement flake8-bandit #1646

Closed
74 of 75 tasks
charliermarsh opened this issue Jan 5, 2023 · 38 comments
Closed
74 of 75 tasks

Implement flake8-bandit #1646

charliermarsh opened this issue Jan 5, 2023 · 38 comments
Labels
plugin Implementing a known but unsupported plugin

Comments

@charliermarsh
Copy link
Member

charliermarsh commented Jan 5, 2023

@charliermarsh
Copy link
Member Author

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

@charliermarsh charliermarsh added the plugin Implementing a known but unsupported plugin label Jan 5, 2023
@edgarrmondragon
Copy link
Contributor

thanks @charliermarsh!

@charliermarsh
Copy link
Member Author

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.

@edgarrmondragon
Copy link
Contributor

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.

@ahmedbilal
Copy link

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

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

@charliermarsh
Copy link
Member Author

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

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

@colin99d
Copy link
Contributor

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

@colin99d
Copy link
Contributor

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.

@charliermarsh
Copy link
Member Author

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

@qdegraaf
Copy link
Contributor

qdegraaf commented Nov 6, 2023

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.

@ischaojie
Copy link
Contributor

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

@pygaiwan
Copy link

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

@Daverball
Copy link
Contributor

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

@charliermarsh
Copy link
Member Author

Five rules left :)

charliermarsh pushed a commit that referenced this issue Jan 3, 2024
## Summary

Adds all `S4XX` rules to the
[flake8-bandit](https://github.com/tylerwince/flake8-bandit) plugin
port.

There is a lot of documentation to write, some tests can be expanded and
implementation can probably be refactored to be more compact. As there
is some discussion on whether this is actually useful. (See:
#1646 (comment)),
wanted to check which rules we want to have before I go through the
process of polishing this up.

## Test Plan

Fixtures for all rules based on `flake8-bandit`
[tests](https://github.com/tylerwince/flake8-bandit/tree/main/tests)

## Issue link

Refers: #1646
charliermarsh pushed a commit that referenced this issue Jan 3, 2024
## Summary
Adds `S504` rule for the
[flake8-bandit](https://github.com/tylerwince/flake8-bandit) plugin
port.

Checks for calls to `ssl.wrap_socket` which have no `ssl_version`
argument set. See also
https://bandit.readthedocs.io/en/latest/_modules/bandit/plugins/insecure_ssl_tls.html#ssl_with_no_version

## Test Plan

Fixture added 

## Issue Link

Refers: #1646
charliermarsh pushed a commit that referenced this issue Jan 5, 2024
## Summary

Adds S502 rule for the
[flake8-bandit](https://github.com/tylerwince/flake8-bandit) plugin
port.

Checks for calls to any function with keywords arguments `ssl_version`
or `method` or for kwargs `method` in calls to `OpenSSL.SSL.Context` and
`ssl_version` in calls to `ssl.wrap_socket` which have an insecure
ssl_version valu. See also
https://bandit.readthedocs.io/en/latest/_modules/bandit/plugins/insecure_ssl_tls.html#ssl_with_bad_version

## Test Plan

Fixture added

## Issue Link

Refers: #1646
charliermarsh pushed a commit that referenced this issue Jan 5, 2024
## Summary

Adds S503 rule for the
[flake8-bandit](https://github.com/tylerwince/flake8-bandit) plugin
port.

Checks for function defs argument defaults which have an insecure
ssl_version value. See also
https://bandit.readthedocs.io/en/latest/_modules/bandit/plugins/insecure_ssl_tls.html#ssl_with_bad_defaults

Some logic and the `const` can be shared with
#9390. When one of the two is
merged.

## Test Plan

Fixture added

## Issue Link

Refers: #1646
@edgarrmondragon
Copy link
Contributor

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

@glennmatthews
Copy link

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

@itdependsnetworks
Copy link

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.

charliermarsh pushed a commit that referenced this issue Mar 12, 2024
Part of #1646.

## Summary

Implement `S610` rule from `flake8-bandit`. 

Upstream references:
- Implementation:
https://github.com/PyCQA/bandit/blob/1.7.8/bandit/plugins/django_sql_injection.py#L20-L97
- Test cases:
https://github.com/PyCQA/bandit/blob/1.7.8/examples/django_sql_injection_extra.py
- Test assertion:
https://github.com/PyCQA/bandit/blob/1.7.8/tests/functional/test_functional.py#L517-L524

The implementation in `bandit` targets additional arguments (`params`,
`order_by` and `select_params`) but doesn't seem to do anything with
them in the end, so I did not include them in the implementation.

Note that this rule could be prone to false positives, as ideally we
would want to check if `extra()` is tied to a [Django
queryset](https://docs.djangoproject.com/en/5.0/ref/models/querysets/),
but AFAIK Ruff is not able to resolve classes outside of the current
module.

## Test Plan

Snapshot tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Implementing a known but unsupported plugin
Projects
None yet
Development

No branches or pull requests