Replace bandit with ruff

ruff has reimplemented the bandit rules[1], so we can use that as a better-integrated tool with the rest of our stack. We enable all the bandit rules and selectively disable some across the codebase and some in just tests where they don't make sense (e.g. flagging use of `assert` or using insecure crypto). For bonus points we can get rid of the GitPython dependency, which has a history of (non-exploitable in our context) security issues. [1] Per astral-sh/ruff#1646 they've implemented nearly all of them, and the remaining ones aren't that important IMO.
freedomofpress · Oct 12, 2023 · bdc8722 · bdc8722
1 parent 8d00ba5
commit bdc8722
Show file tree

Hide file tree

Showing 17 changed files with 54 additions and 65 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -307,13 +307,6 @@ jobs:
             DOCKER_BUILD_ARGUMENTS="--cache-from securedrop-test-focal-py3:${fromtag:-latest}" securedrop/bin/dev-shell \
                   bash -c "pip3 install -U -q --upgrade safety && make -C .. safety"
 
-      - run:
-          name: Run static security testing on source code with bandit
-          command: |
-            fromtag=$(docker images |grep securedrop-test-focal-py3 |head -n1 |awk '{print $2}')
-            DOCKER_BUILD_ARGUMENTS="--cache-from securedrop-test-focal-py3:${fromtag:-latest}" securedrop/bin/dev-shell \
-                  bash -c "pip3 install -U -q --upgrade pip && pip3 install -U -q --upgrade bandit && make -C .. bandit"
-
       - run:
           name: Run static security testing on source code with semgrep
           command: |

diff --git a/Makefile b/Makefile
@@ -177,19 +177,6 @@ safety:  ## Run `safety check` to check python dependencies for vulnerabilities.
 	done
 	@echo
 
-# Bandit is a static code analysis tool to detect security vulnerabilities in Python applications
-# https://wiki.openstack.org/wiki/Security/Projects/Bandit
-.PHONY: bandit
-
-bandit: test-config ## Run bandit with medium level excluding test-related folders.
-	@command -v bandit || (echo "Please run 'pip install -U bandit'."; exit 1)
-	@echo "███ Running bandit..."
-	@bandit -ll --exclude ./admin/.tox,./admin/.venv,./admin/.eggs,./molecule,./testinfra,./securedrop/tests,./.tox,./.venv*,securedrop/config.py,./target --recursive .
-	@echo "███ Running bandit on securedrop/config.py..."
-	@bandit -ll --skip B108 securedrop/config.py
-	@echo
-
-
 # Semgrep is a static code analysis tool to detect security vulnerabilities in Python applications
 # This configuration uses the public "p/r2c-security-audit" ruleset
 .PHONY: semgrep

diff --git a/devops/scripts/verify-mo.py b/devops/scripts/verify-mo.py
@@ -113,11 +113,11 @@ def diffoscope_call(
         # because we want to inherit the Python virtual environment
         # in which we're invoked.
         # nosemgrep: python.lang.security.audit.subprocess-shell-true.subprocess-shell-true
-        return subprocess.run(  # nosec B602
+        return subprocess.run(
             cmd,
             capture_output=True,
             env=os.environ,
-            shell=True,
+            shell=True,  # noqa: S602
         )
 
     def reproduce(self) -> None:

diff --git a/pyproject.toml b/pyproject.toml
@@ -28,6 +28,8 @@ select = [
     "PYI",
     # flake8-return
     "RET",
+    # flake8-bandit
+    "S",
     # flake8-simplify
     "SIM",
     # pyupgrade
@@ -58,6 +60,16 @@ ignore = [
     "RET503",
     # superflous-else- rules, find they hurt readability
     "RET505", "RET506", "RET507", "RET508",
+    # yes, we bind to 0.0.0.0
+    "S104",
+    # hardcoded passwords, lots of false positives
+    "S105", "S106",
+    # we intentionally don't log stuff sometimes
+    "S110",
+    # flags every instance of subprocess
+    "S603",
+    # we trust $PATH isn't hijacked
+    "S607",
     # Find contextlib.suppress() is harder to read
     "SIM105",
     # Find ternary statements harder to read
@@ -74,6 +86,27 @@ extend-include = ["securedrop/scripts/*"]
 # them as third-party to match O.G. isort until we fix our package layout
 known-third-party = ["journalist_app", "management", "source_app", "tests"]
 
+[tool.ruff.per-file-ignores]
+"**/test**.py" = [
+    # use of `assert`
+    "S101",
+    # weak crypto
+    "S311",
+
+    # insecure temporary file/directory
+    "S108",
+    # HTTP request without timeout
+    "S113",
+]
+"securedrop/loaddata.py" = [
+    # ok to use weak crypto here
+    "S311",
+]
+"securedrop/pretty_bad_protocol/*.py" = [
+    # legacy code that still uses `assert`
+    "S101",
+]
+
 [tool.mypy]
 ignore_missing_imports = true
 no_implicit_optional = true

diff --git a/securedrop/alembic/versions/2e24fc7536e8_make_journalist_id_non_nullable.py b/securedrop/alembic/versions/2e24fc7536e8_make_journalist_id_non_nullable.py
@@ -63,7 +63,7 @@ def migrate_nulls() -> None:
     conn = op.get_bind()
     for table in tables:
         result = conn.execute(
-            f"SELECT 1 FROM {table} WHERE journalist_id IS NULL;"  # nosec
+            f"SELECT 1 FROM {table} WHERE journalist_id IS NULL;"  # noqa: S608
         ).first()
         if result is not None:
             needs_migration.append(table)
@@ -80,12 +80,12 @@ def migrate_nulls() -> None:
         # unique key violations.
         op.execute(
             sa.text(
-                f"UPDATE OR IGNORE {table} SET journalist_id=:journalist_id "  # nosec
+                f"UPDATE OR IGNORE {table} SET journalist_id=:journalist_id "  # noqa: S608
                 "WHERE journalist_id IS NULL;"
             ).bindparams(journalist_id=deleted_id)
         )
         # Then we delete any leftovers which had been ignored earlier.
-        op.execute(f"DELETE FROM {table} WHERE journalist_id IS NULL")  # nosec
+        op.execute(f"DELETE FROM {table} WHERE journalist_id IS NULL")  # noqa: S608
 
 
 def upgrade() -> None:

diff --git a/securedrop/alembic/versions/3da3fcab826a_delete_orphaned_submissions.py b/securedrop/alembic/versions/3da3fcab826a_delete_orphaned_submissions.py
@@ -24,10 +24,10 @@
 def raw_sql_grab_orphaned_objects(table_name: str) -> str:
     """Objects that have a source ID that doesn't exist in the
     sources table OR a NULL source ID should be deleted."""
-    return (
-        "SELECT id, filename, source_id FROM {table} "  # nosec
+    return (  # noqa: S608
+        "SELECT id, filename, source_id FROM {table} "
         "WHERE source_id NOT IN (SELECT id FROM sources) "
-        "UNION SELECT id, filename, source_id FROM {table} "  # nosec
+        "UNION SELECT id, filename, source_id FROM {table} "
         "WHERE source_id IS NULL"
     ).format(table=table_name)
 

diff --git a/securedrop/alembic/versions/b7f98cfd6a70_make_filesystem_id_non_nullable.py b/securedrop/alembic/versions/b7f98cfd6a70_make_filesystem_id_non_nullable.py
@@ -42,9 +42,9 @@ def upgrade() -> None:
     # Now things that directly refer to sources
     for table in ("source_stars", "submissions", "replies"):
         op.execute(
-            f"DELETE FROM {table} WHERE source_id IN "  # nosec
+            f"DELETE FROM {table} WHERE source_id IN "  # noqa: S608
             f"(SELECT id FROM sources WHERE filesystem_id IS NULL)"
-        )  # nosec
+        )
     # And now the sources
     op.execute("DELETE FROM sources WHERE filesystem_id IS NULL")
     with op.batch_alter_table("sources", schema=None) as batch_op:

diff --git a/securedrop/journalist.py b/securedrop/journalist.py
@@ -27,4 +27,4 @@ def prime_keycache() -> None:
 if __name__ == "__main__":  # pragma: no cover
     debug = getattr(config, "env", "prod") != "prod"
     # nosemgrep: python.flask.security.audit.app-run-param-config.avoid_app_run_with_bad_host
-    app.run(debug=debug, host="0.0.0.0", port=8081)  # nosec
+    app.run(debug=debug, host="0.0.0.0", port=8081)
diff --git a/securedrop/models.py b/securedrop/models.py
@@ -559,7 +559,8 @@ def valid_password(self, passphrase: "Optional[str]") -> bool:
                 )
 
             # For type checking
-            assert isinstance(self.pw_hash, bytes)
+            if not isinstance(self.pw_hash, bytes):
+                raise RuntimeError("self.pw_hash isn't bytes")
 
             is_valid = compare_digest(self._scrypt_hash(passphrase, self.pw_salt), self.pw_hash)
 

diff --git a/securedrop/pretty_bad_protocol/_util.py b/securedrop/pretty_bad_protocol/_util.py
@@ -44,7 +44,7 @@
 # environs. https://github.com/isislovecruft/python-gnupg/issues/74
 if not _user:
     # FIXME: Shouldn't we use a randomized tmp directory? (Do we even hit this code path?)
-    _user = "/tmp/python-gnupg"  # nosec B108
+    _user = "/tmp/python-gnupg"  # noqa: S108
     try:
         os.makedirs(_user)
     except OSError:

diff --git a/securedrop/request_that_secures_file_uploads.py b/securedrop/request_that_secures_file_uploads.py
@@ -29,7 +29,7 @@ def _secure_file_stream(
             # note in `config.py` for more info. Instead, we just use
             # `/tmp`, which has the additional benefit of being
             # automatically cleared on reboot.
-            return SecureTemporaryFile("/tmp")  # nosec
+            return SecureTemporaryFile("/tmp")  # noqa: S108
         return BytesIO()
 
     def make_form_data_parser(self) -> FormDataParser:

diff --git a/securedrop/requirements/python3/develop-requirements.in b/securedrop/requirements/python3/develop-requirements.in
@@ -2,7 +2,6 @@ Jinja2>=3.0.0
 ansible-lint>=4.2.0
 ansible>=2.9.23,<2.10.0
 argon2_cffi>=20.1.0
-bandit
 black
 cffi>=1.14.2
 cryptography>=41.0.3

diff --git a/securedrop/requirements/python3/develop-requirements.txt b/securedrop/requirements/python3/develop-requirements.txt
@@ -66,10 +66,6 @@ babel==2.12.1 \
 babelgladeextractor==0.7.0 \
     --hash=sha256:bcf805e28b4bb18c8b6909a65a7cf5c7c2bcbf4ae50b164878c9682d22271798
     # via -r requirements/python3/translation-requirements.in
-bandit==1.7.0 \
-    --hash=sha256:216be4d044209fa06cf2a3e51b319769a51be8318140659719aa7a115c35ed07 \
-    --hash=sha256:8a4c7415254d75df8ff3c3b15cfe9042ecee628a1e40b44c15a98890fbfc2608
-    # via -r requirements/python3/develop-requirements.in
 bcrypt==3.1.3 \
     --hash=sha256:05b35b9842b009b44496fa5433ce462f69966291e50fbd471dbb427f399f748f \
     --hash=sha256:06280fe19ffbcf6cf904de25190dd6fcd313e30bc79da305f5642a8295d1616e \
@@ -341,14 +337,6 @@ git-url-parse==1.0.2 \
     --hash=sha256:75d1cf1e19534678711e1a9293e6fb978461a5f734adf941cd468a802d08bbb2 \
     --hash=sha256:bf0a20c48e8552fea1218b52714be84fa31b0d73c58897e70b765d5e7a26f4cb
     # via python-gilt
-gitdb2==2.0.3 \
-    --hash=sha256:b60e29d4533e5e25bb50b7678bbc187c8f6bcff1344b4f293b2ba55c85795f09 \
-    --hash=sha256:cf9a4b68e8c4da8d42e48728c944ff7af2d8c9db303ac1ab32eac37aa4194b0e
-    # via gitpython
-gitpython==2.1.8 \
-    --hash=sha256:ad61bc25deadb535b047684d06f3654c001d9415e1971e51c9c20f5b510076e9 \
-    --hash=sha256:b8367c432de995dc330b5b146c5bfdc0926b8496e100fda6692134e00c0dcdc5
-    # via bandit
 glom==22.1.0 \
     --hash=sha256:1510c6587a8f9c64a246641b70033cbc5ebde99f02ad245693678038e821aeb5 \
     --hash=sha256:5339da206bf3532e01a83a35aca202960ea885156986d190574b779598e9e772
@@ -654,7 +642,6 @@ pbr==5.1.1 \
     # via
     #   git-url-parse
     #   python-gilt
-    #   stevedore
 peewee==3.15.0 \
     --hash=sha256:48eac70be812ac84daa5400fb8e7b545e0c83adcfa05c8e2a8612f9ced4da495
     # via semgrep
@@ -850,7 +837,6 @@ pyyaml==5.4.1 \
     #   ansible
     #   ansible-lint
     #   aspy.yaml
-    #   bandit
     #   molecule
     #   molecule-vagrant
     #   pre-commit
@@ -968,7 +954,6 @@ six==1.15.0 \
     #   -r ../admin/requirements.in
     #   ansible-lint
     #   argon2-cffi
-    #   bandit
     #   bcrypt
     #   cfgv
     #   click-completion
@@ -981,16 +966,7 @@ six==1.15.0 \
     #   prompt-toolkit
     #   pynacl
     #   python-dateutil
-    #   stevedore
     #   websocket-client
-smmap2==2.0.3 \
-    --hash=sha256:b78ee0f1f5772d69ff50b1cbdb01b8c6647a8354f02f23b488cf4b2cfc923956 \
-    --hash=sha256:c7530db63f15f09f8251094b22091298e82bf6c699a6b8344aaaef3f2e1276c3
-    # via gitdb2
-stevedore==1.28.0 \
-    --hash=sha256:e3d96b2c4e882ec0c1ff95eaebf7b575a779fd0ccb4c741b9832bed410d58b3d \
-    --hash=sha256:f1c7518e7b160336040fee272174f1f7b29a46febb3632502a8f2055f973d60b
-    # via bandit
 tabulate==0.8.7 \
     --hash=sha256:ac64cb76d53b1231d364babcd72abbb16855adac7de6665122f97b593f1eb2ba \
     --hash=sha256:db2723a20d04bcda8522165c73eea7c300eda74e0ce852d9022e0159d7895007

diff --git a/securedrop/source.py b/securedrop/source.py
@@ -8,4 +8,4 @@
 if __name__ == "__main__":  # pragma: no cover
     debug = getattr(config, "env", "prod") != "prod"
     # nosemgrep: python.flask.security.audit.app-run-param-config.avoid_app_run_with_bad_host
-    app.run(debug=debug, host="0.0.0.0", port=8080)  # nosec
+    app.run(debug=debug, host="0.0.0.0", port=8080)
diff --git a/securedrop/store.py b/securedrop/store.py
@@ -324,7 +324,7 @@ def save_file_submission(
 
         encrypted_file_name = f"{count}-{journalist_filename}-doc.gz.gpg"
         encrypted_file_path = self.path(filesystem_id, encrypted_file_name)
-        with SecureTemporaryFile("/tmp") as stf:  # nosec
+        with SecureTemporaryFile("/tmp") as stf:  # noqa: S108
             with gzip.GzipFile(filename=sanitized_filename, mode="wb", fileobj=stf, mtime=0) as gzf:
                 # Buffer the stream into the gzip file to avoid excessive
                 # memory consumption

diff --git a/securedrop/tests/migrations/migration_de00920916bf.py b/securedrop/tests/migrations/migration_de00920916bf.py
@@ -14,15 +14,15 @@ class Helper:
     def __init__(self):
         self.journalist_id = None
 
-    def create_journalist(self, otp_secret="ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"):
+    def create_journalist(self):
         if self.journalist_id is not None:
             raise RuntimeError("Journalist already created")
 
         params = {
             "uuid": str(uuid.uuid4()),
             "username": random_chars(50),
             "session_nonce": 0,
-            "otp_secret": otp_secret,
+            "otp_secret": "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567",
         }
         sql = """INSERT INTO journalists (uuid, username, otp_secret, session_nonce)
                  VALUES (:uuid, :username, :otp_secret, :session_nonce)

diff --git a/securedrop/two_factor.py b/securedrop/two_factor.py
@@ -30,7 +30,7 @@ class HOTP:
     _LENGTH = 6
 
     # nosemgrep: python.cryptography.security.insecure-hash-algorithms.insecure-hash-algorithm-sha1
-    _ALGORITHM = SHA1()  # nosec B303
+    _ALGORITHM = SHA1()  # noqa: S303
 
     _LOOK_AHEAD_WINDOW_SIZE = 20
 
@@ -84,7 +84,7 @@ class TOTP:
     _TIME_STEP = 30
 
     # nosemgrep: python.cryptography.security.insecure-hash-algorithms.insecure-hash-algorithm-sha1
-    _ALGORITHM = SHA1()  # nosec B303
+    _ALGORITHM = SHA1()  # noqa: S303
 
     # Minimum length for ascii-encoded OTP secrets - by default, secrets are now 160-bit (32 chars)
     # but existing Journalist users may still have 80-bit (16-char) secrets