From bdc87222a6b4a4bb4cc736a06a655064305596b6 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Fri, 29 Sep 2023 18:49:06 -0400 Subject: [PATCH] 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 https://github.com/astral-sh/ruff/issues/1646 they've implemented nearly all of them, and the remaining ones aren't that important IMO. --- .circleci/config.yml | 7 ---- Makefile | 13 -------- devops/scripts/verify-mo.py | 4 +-- pyproject.toml | 33 +++++++++++++++++++ ...c7536e8_make_journalist_id_non_nullable.py | 6 ++-- ...da3fcab826a_delete_orphaned_submissions.py | 6 ++-- ...cfd6a70_make_filesystem_id_non_nullable.py | 4 +-- securedrop/journalist.py | 2 +- securedrop/models.py | 3 +- securedrop/pretty_bad_protocol/_util.py | 2 +- .../request_that_secures_file_uploads.py | 2 +- .../python3/develop-requirements.in | 1 - .../python3/develop-requirements.txt | 24 -------------- securedrop/source.py | 2 +- securedrop/store.py | 2 +- .../migrations/migration_de00920916bf.py | 4 +-- securedrop/two_factor.py | 4 +-- 17 files changed, 54 insertions(+), 65 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index eff3f852ea..d8e4f8ab22 100644 --- 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 index b528fcf257..271e6b11e2 100644 --- 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 index cc1d7d595e..52ad7e546e 100755 --- 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 index 7e8def0d94..799c8aa95a 100644 --- 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 index afa053708c..5237c0dfe5 100644 --- 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 index aaa5e32ae7..19887fa014 100644 --- 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 index 5e60276568..2109666a20 100644 --- 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 index 07757a2976..edb43e535f 100644 --- 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 index a07b8b74bf..e70e492fcb 100644 --- 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 index f8e49438a8..d90b83082a 100644 --- 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 index 9d58928c09..e76e31e77d 100644 --- 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 index e888974bb7..05e2ed2d43 100644 --- 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 index 6d073073fc..6ea9b8bf5f 100644 --- 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 index 176f416b8a..6e380f4a6c 100644 --- 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 index 9d8e514c3b..a9b4987c3c 100644 --- 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 index 48ebb2f68f..13336c5a6c 100644 --- a/securedrop/tests/migrations/migration_de00920916bf.py +++ b/securedrop/tests/migrations/migration_de00920916bf.py @@ -14,7 +14,7 @@ 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") @@ -22,7 +22,7 @@ def create_journalist(self, otp_secret="ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"): "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 index d3f61d2be7..5542c381e4 100644 --- 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