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