Merge pull request #6961 from freedomofpress/ruff-bandit

Replace bandit with ruff

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

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

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:

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

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:

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)
 

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:

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)

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)
 

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:

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:

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

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

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)

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

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)

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