From 3ddc876fb5ee119e5dc311644dbdfbcf4fd689fc Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Wed, 7 Aug 2024 19:57:09 -0400 Subject: [PATCH 01/10] Set password for redis access Redis can require basic password authentication to further limit who can access it. We need to set a password in `/etc/redis/redis.conf` and then ensure all clients pass it when authenticating. For most of our code, this is straightforward since we can set the password in config.py and access it through `SecureDropConfig`. However the `rqworker` has its own config file that we haven't needed until now, so we also create a `rq_config.py` with just the redis password. The ansible playbook now generates a 32-byte password for redis, sets it in config.py, rq_config.py, and /etc/redis/redis.conf, and then restarts the redis-server systemd service. The postinst generates a 32-byte password in the same manner, writes it to the same set of files, and then restarts the redis-server service. This ended up requiring the most changes, since I removed the inline Python code from the Makefile into a separate `dev-config` script, which now also handles creation of rq_config.py. We set the password in /etc/redis/redis.conf and then start the redis-server service. It's also written to `/tmp/redispasswd` for easy fetching in tests. testinfra tests verify that that unathenticated access fails and that the configured `rq_config.py` password works. This addresses SEC-01-008 WP3: "Read+Write Access to Redis via UnAuth Telnet (Low)". Fixes . --- Makefile | 14 +------ .../app/tasks/initialize_securedrop_app.yml | 42 +++++++++++++++++++ .../app-code/test_securedrop_rqworker.py | 2 +- molecule/testinfra/app/test_redis.py | 38 +++++++++++++++++ securedrop/.gitignore | 1 + securedrop/bin/dev-config | 33 +++++++++++++++ securedrop/bin/dev-deps | 7 +++- securedrop/bin/generate-docs-screenshots | 2 +- securedrop/bin/run | 4 +- securedrop/bin/run-test | 2 +- securedrop/bin/translation-test | 2 +- securedrop/config.py.example | 2 + .../app-code/etc/apparmor.d/usr.sbin.apache2 | 1 + .../system/securedrop_rqworker.service | 2 +- .../debian/securedrop-app-code.postinst | 19 +++++++++ securedrop/encryption.py | 5 ++- securedrop/journalist_app/__init__.py | 2 +- securedrop/journalist_app/sessions.py | 13 +++--- securedrop/sdconfig.py | 11 +++++ securedrop/tests/config_from_2014.py | 4 ++ securedrop/tests/conftest.py | 2 + securedrop/tests/factories.py | 3 ++ securedrop/tests/test_encryption.py | 2 + securedrop/tests/test_journalist_session.py | 34 ++++++++------- securedrop/tests/test_worker.py | 2 + securedrop/worker.py | 7 +++- 26 files changed, 209 insertions(+), 47 deletions(-) create mode 100644 molecule/testinfra/app/test_redis.py create mode 100755 securedrop/bin/dev-config diff --git a/Makefile b/Makefile index e6613187ce..c22c3adf3d 100644 --- a/Makefile +++ b/Makefile @@ -239,19 +239,7 @@ rust-audit: securedrop/config.py: ## Generate the test SecureDrop application config. @echo "███ Generating securedrop/config.py..." - @cd securedrop && source_secret_key=$(shell head -c 32 /dev/urandom | base64) \ - journalist_secret_key=$(shell head -c 32 /dev/urandom | base64) \ - scrypt_id_pepper=$(shell head -c 32 /dev/urandom | base64) \ - scrypt_gpg_pepper=$(shell head -c 32 /dev/urandom | base64) \ - python -c 'import os; from jinja2 import Environment, FileSystemLoader; \ - env = Environment(loader=FileSystemLoader(".")); \ - ctx = {"securedrop_app_gpg_fingerprint": "65A1B5FF195B56353CC63DFFCC40EF1228271441"}; \ - ctx.update(dict((k, {"stdout":v}) for k,v in os.environ.items())); \ - ctx = open("config.py", "w").write(env.get_template("config.py.example").render(ctx))' - @echo >> securedrop/config.py - @echo "SUPPORTED_LOCALES = $$(make --quiet supported-locales)" >> securedrop/config.py - @echo "SUPPORTED_LOCALES.append('en_US')" >> securedrop/config.py - @echo + @./securedrop/bin/dev-config HOOKS_DIR=.githooks diff --git a/install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml b/install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml index d4c4f37cfe..4417fcfb1d 100644 --- a/install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml +++ b/install_files/ansible-base/roles/app/tasks/initialize_securedrop_app.yml @@ -126,6 +126,48 @@ tags: - securedrop_config +- name: Generate 32-byte value for "redis password". + shell: "head -c 32 /dev/urandom | base64" + register: redis_password + when: not config.stat.exists + tags: + - securedrop_config + +- name: Add 32-byte value for "redis password" to config.py. + lineinfile: + dest: "{{ securedrop_code }}/config.py" + regexp: "redis_password" + line: "REDIS_PASSWORD = '{{ redis_password.stdout }}'" + when: not config.stat.exists + tags: + - securedrop_config + +- name: Create rq_config.py with the redis password + copy: + content: "REDIS_PASSWORD = \"{{ redis_password.stdout }}\"" + dest: "{{ securedrop_code }}/rq_config.py" + owner: "root" + group: "www-data" + mode: "0640" + when: not config.stat.exists + tags: + - securedrop_config + +- name: Add 32-byte value for "redis password" to /etc/redis/redis.conf. + lineinfile: + dest: "/etc/redis/redis.conf" + regexp: "^requirepass" + line: "requirepass {{ redis_password.stdout }}" + insertafter: EOF + when: not config.stat.exists + register: redis_conf + +- name: Restart redis + service: + name: redis-server + state: restarted + when: redis_conf.changed + - name: Declare Application GPG fingerprint in config.py. lineinfile: dest: "{{ securedrop_code }}/config.py" diff --git a/molecule/testinfra/app-code/test_securedrop_rqworker.py b/molecule/testinfra/app-code/test_securedrop_rqworker.py index 737a4a9728..1afdb20c47 100644 --- a/molecule/testinfra/app-code/test_securedrop_rqworker.py +++ b/molecule/testinfra/app-code/test_securedrop_rqworker.py @@ -23,7 +23,7 @@ def test_securedrop_rqworker_service(host): securedrop_test_vars.securedrop_code, securedrop_test_vars.securedrop_venv_site_packages, ), - f"ExecStart={securedrop_test_vars.securedrop_venv_bin}/rqworker", + f"ExecStart={securedrop_test_vars.securedrop_venv_bin}/rqworker -c rq_config", "PrivateDevices=yes", "PrivateTmp=yes", "ProtectSystem=full", diff --git a/molecule/testinfra/app/test_redis.py b/molecule/testinfra/app/test_redis.py new file mode 100644 index 0000000000..75af29f6ab --- /dev/null +++ b/molecule/testinfra/app/test_redis.py @@ -0,0 +1,38 @@ +""" +Test redis is configured as desired +""" + +import re + +import testutils + +sdvars = testutils.securedrop_test_vars +testinfra_hosts = [sdvars.app_hostname] + + +def test_auth_required(host): + """ + Verify the redis server requires authentication + """ + response = host.run("bash -c 'echo \"PING\" | redis-cli'").stdout.strip() + assert response == "NOAUTH Authentication required." + + +def test_password_works(host): + """ + Verify the redis password works + """ + f = host.file("/var/www/securedrop/rq_config.py") + with host.sudo(): + # First let's check file permissions + assert f.is_file + assert f.user == "root" + assert f.group == "www-data" + assert f.mode == 0o640 + contents = f.content_string + password = re.search('"(.*?)"', contents).group(1) + # Now run an authenticated PING + response = host.run( + f'bash -c \'echo "PING" | REDISCLI_AUTH="{password}" redis-cli\'' + ).stdout.strip() + assert response == "PONG" diff --git a/securedrop/.gitignore b/securedrop/.gitignore index 2c48a57145..ffba15c6e4 100644 --- a/securedrop/.gitignore +++ b/securedrop/.gitignore @@ -1,5 +1,6 @@ # Don't version control config.py because it contains secret values config.py +rq_config.py # compiled file securedrop-adminc diff --git a/securedrop/bin/dev-config b/securedrop/bin/dev-config new file mode 100755 index 0000000000..74633c3b22 --- /dev/null +++ b/securedrop/bin/dev-config @@ -0,0 +1,33 @@ +#!/usr/bin/env python3 +import os +import base64 +import subprocess + +from jinja2 import Environment, FileSystemLoader + + +def generate_random(length): + return {'stdout': base64.b64encode(os.urandom(length)).decode()} + + +env = Environment(loader=FileSystemLoader(".")) + +ctx = { + "securedrop_app_gpg_fingerprint": "65A1B5FF195B56353CC63DFFCC40EF1228271441", + 'source_secret_key': generate_random(32), + 'journalist_secret_key': generate_random(32), + 'scrypt_id_pepper': generate_random(32), + 'scrypt_gpg_pepper': generate_random(32), + 'redis_password': generate_random(32), +} + +with open('securedrop/config.py', 'w') as f: + text = env.get_template("securedrop/config.py.example").render(ctx) + text += '\n' + supported_locales = subprocess.check_output(['make', '--quiet', 'supported-locales']).decode().strip() + text += f'SUPPORTED_LOCALES = {supported_locales}\n' + text += 'SUPPORTED_LOCALES.append("en_US")\n' + f.write(text) + +with open('securedrop/rq_config.py', 'w') as f: + f.write('REDIS_PASSWORD = "{}"\n'.format(ctx['redis_password']['stdout'])) diff --git a/securedrop/bin/dev-deps b/securedrop/bin/dev-deps index f91a61bf58..ac7cf88265 100755 --- a/securedrop/bin/dev-deps +++ b/securedrop/bin/dev-deps @@ -22,7 +22,11 @@ function run_xvfb() { function run_redis() { rm -f "${REPOROOT}/securedrop/dump.rdb" - setsid redis-server >& /tmp/redis.out || cat /tmp/redis.out + redis_password=$(cd "${REPOROOT}/securedrop" && python -c "import rq_config; print(rq_config.REDIS_PASSWORD)") + echo "$redis_password" > /tmp/redispasswd + echo "requirepass ${redis_password}" | sudo -u redis tee -a /etc/redis/redis.conf + echo "Starting redis..." + sudo service redis-server start } function setup_vncauth { @@ -47,6 +51,7 @@ function append_to_exit() { function maybe_create_config_py() { if ! test -f "${REPOROOT}/securedrop/config.py" ; then append_to_exit "rm ${REPOROOT}/securedrop/config.py" + append_to_exit "rm ${REPOROOT}/securedrop/rq_config.py" (cd "$REPOROOT" && make test-config) fi } diff --git a/securedrop/bin/generate-docs-screenshots b/securedrop/bin/generate-docs-screenshots index 40c859a2fc..930896e081 100755 --- a/securedrop/bin/generate-docs-screenshots +++ b/securedrop/bin/generate-docs-screenshots @@ -7,11 +7,11 @@ source "${BASH_SOURCE%/*}/dev-deps" run_xvfb & run_tor & -run_redis & run_x11vnc & urandom build_redwood maybe_create_config_py +run_redis pybabel compile --directory translations/ pytest -v --page-layout "${@:-tests/functional/pageslayout}" diff --git a/securedrop/bin/run b/securedrop/bin/run index cd32d4fe4f..cbf68572fb 100755 --- a/securedrop/bin/run +++ b/securedrop/bin/run @@ -12,15 +12,15 @@ cd "${REPOROOT}/securedrop" source /opt/venvs/securedrop-app-code/bin/activate source "${BASH_SOURCE%/*}/dev-deps" -run_redis & urandom build_redwood maybe_create_config_py +run_redis reset_demo maybe_use_tor # run the batch processing services normally managed by systemd -/opt/venvs/securedrop-app-code/bin/rqworker & +PYTHONPATH="${REPOROOT}/securedrop" /opt/venvs/securedrop-app-code/bin/rqworker -c rq_config & PYTHONPATH="${REPOROOT}/securedrop" /opt/venvs/securedrop-app-code/bin/python "${REPOROOT}/securedrop/scripts/rqrequeue" --interval 60 & PYTHONPATH="${REPOROOT}/securedrop" /opt/venvs/securedrop-app-code/bin/python "${REPOROOT}/securedrop/scripts/shredder" --interval 60 & PYTHONPATH="${REPOROOT}/securedrop" /opt/venvs/securedrop-app-code/bin/python "${REPOROOT}/securedrop/scripts/source_deleter" --interval 10 & diff --git a/securedrop/bin/run-test b/securedrop/bin/run-test index ff11dc33f5..8b1e6ab956 100755 --- a/securedrop/bin/run-test +++ b/securedrop/bin/run-test @@ -20,12 +20,12 @@ source "${BASH_SOURCE%/*}/dev-deps" run_xvfb run_tor & -run_redis & setup_vncauth run_x11vnc & urandom build_redwood maybe_create_config_py +run_redis if [ -n "${CIRCLE_BRANCH:-}" ] ; then touch tests/log/firefox.log diff --git a/securedrop/bin/translation-test b/securedrop/bin/translation-test index 0cb95be12f..a3c6cb99a1 100755 --- a/securedrop/bin/translation-test +++ b/securedrop/bin/translation-test @@ -7,12 +7,12 @@ source "${BASH_SOURCE%/*}/dev-deps" run_xvfb run_tor & -run_redis & setup_vncauth run_x11vnc & urandom build_redwood maybe_create_config_py +run_redis pybabel compile --directory translations/ mkdir -p "/tmp/test-results/logs" diff --git a/securedrop/config.py.example b/securedrop/config.py.example index 92544087ab..2da049cf8a 100644 --- a/securedrop/config.py.example +++ b/securedrop/config.py.example @@ -86,3 +86,5 @@ DEFAULT_LOCALE = 'en_US' # How long a session is valid before it expires and logs a user out SESSION_EXPIRATION_MINUTES = 120 + +REDIS_PASSWORD = '{{ redis_password.stdout }}' diff --git a/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 b/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 index f7a3ebdeaa..119856197e 100644 --- a/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 +++ b/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 @@ -213,6 +213,7 @@ /var/www/securedrop/pretty_bad_protocol/_meta.py r, /var/www/securedrop/request_that_secures_file_uploads.py r, /var/www/securedrop/rm.py r, + /var/www/securedrop/rq_config.py r, /var/www/securedrop/sdconfig.py r, /var/www/securedrop/secure_tempfile.py r, /var/www/securedrop/source.py r, diff --git a/securedrop/debian/app-code/lib/systemd/system/securedrop_rqworker.service b/securedrop/debian/app-code/lib/systemd/system/securedrop_rqworker.service index 8dc59a84f0..0f62e2d61f 100644 --- a/securedrop/debian/app-code/lib/systemd/system/securedrop_rqworker.service +++ b/securedrop/debian/app-code/lib/systemd/system/securedrop_rqworker.service @@ -5,7 +5,7 @@ Wants=redis-server.service [Service] Environment=PYTHONPATH="/var/www/securedrop:/opt/venvs/securedrop-app-code/lib/python3.8/site-packages" -ExecStart=/opt/venvs/securedrop-app-code/bin/rqworker +ExecStart=/opt/venvs/securedrop-app-code/bin/rqworker -c rq_config PrivateDevices=yes PrivateTmp=yes ProtectSystem=full diff --git a/securedrop/debian/securedrop-app-code.postinst b/securedrop/debian/securedrop-app-code.postinst index 90ff93a23d..0858cb40ed 100644 --- a/securedrop/debian/securedrop-app-code.postinst +++ b/securedrop/debian/securedrop-app-code.postinst @@ -192,6 +192,22 @@ export_journalist_public_key() { } +# Password-protect access to Redis +set_redis_password() { + # Only run when upgrading, which means config.py exists and rq_config.py does not. + if ! test -f "/var/www/securedrop/rq_config.py" && test -f "/var/www/securedrop/config.py"; then + password=$(head -c 32 /dev/urandom | base64) + echo "requirepass $password" | sudo -u redis tee -a /etc/redis/redis.conf + # Set in config.py for web apps + echo "REDIS_PASSWORD = \"$password\"" >> /var/www/securedrop/config.py + # Create separate rq_config for rqworker + touch /var/www/securedrop/rq_config.py + chown root:www-data /var/www/securedrop/rq_config.py + chmod 640 /var/www/securedrop/rq_config.py + echo "REDIS_PASSWORD = \"$password\"" > /var/www/securedrop/rq_config.py + service redis-server restart + fi +} case "$1" in configure) @@ -291,6 +307,9 @@ case "$1" in # GPG -> Sequoia migration export_journalist_public_key + # Set redis password + set_redis_password + # Version migrations database_migration diff --git a/securedrop/encryption.py b/securedrop/encryption.py index 672de291e0..9638617bb6 100644 --- a/securedrop/encryption.py +++ b/securedrop/encryption.py @@ -42,14 +42,14 @@ class EncryptionManager: SOURCE_KEY_UID_RE = re.compile(r"(Source|Autogenerated) Key <([-A-Za-z0-9+/=_]+)>") - def __init__(self, gpg_key_dir: Path, journalist_pub_key: Path) -> None: + def __init__(self, gpg_key_dir: Path, journalist_pub_key: Path, redis: Redis) -> None: self._gpg_key_dir = gpg_key_dir self.journalist_pub_key = journalist_pub_key if not self.journalist_pub_key.exists(): raise RuntimeError( f"The journalist public key does not exist at {self.journalist_pub_key}" ) - self._redis = Redis(decode_responses=True) + self._redis = redis # Instantiate the "main" GPG binary self._gpg = None @@ -88,6 +88,7 @@ def get_default(cls) -> "EncryptionManager": _default_encryption_mgr = cls( gpg_key_dir=config.GPG_KEY_DIR, journalist_pub_key=(config.SECUREDROP_DATA_ROOT / "journalist.pub"), + redis=Redis(decode_responses=True, **config.REDIS_KWARGS), ) return _default_encryption_mgr diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index 5e3a87ed07..a64b3902e3 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -48,7 +48,7 @@ def create_app(config: SecureDropConfig) -> Flask: app.config.from_object(config.JOURNALIST_APP_FLASK_CONFIG_CLS) - Session(app) + Session(app, config) csrf = CSRFProtect(app) app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False diff --git a/securedrop/journalist_app/sessions.py b/securedrop/journalist_app/sessions.py index 856bd333e8..6fdcb19ef0 100644 --- a/securedrop/journalist_app/sessions.py +++ b/securedrop/journalist_app/sessions.py @@ -12,6 +12,7 @@ from itsdangerous import BadSignature, URLSafeTimedSerializer from models import Journalist from redis import Redis +from sdconfig import SecureDropConfig from werkzeug.datastructures import CallbackDict @@ -231,20 +232,20 @@ def logout_user(self, uid: int) -> None: class Session: - def __init__(self, app: Flask) -> None: + def __init__(self, app: Flask, sdconfig: SecureDropConfig) -> None: self.app = app if app is not None: - self.init_app(app) + self.init_app(app, sdconfig) - def init_app(self, app: Flask) -> "None": + def init_app(self, app: Flask, sdconfig: SecureDropConfig) -> "None": """This is used to set up session for your app object. :param app: the Flask app object with proper configuration. """ - app.session_interface = self._get_interface(app) # type: ignore + app.session_interface = self._get_interface(app, sdconfig) # type: ignore - def _get_interface(self, app: Flask) -> SessionInterface: + def _get_interface(self, app: Flask, sdconfig: SecureDropConfig) -> SessionInterface: config = app.config.copy() - config.setdefault("SESSION_REDIS", Redis()) + config.setdefault("SESSION_REDIS", Redis(**sdconfig.REDIS_KWARGS)) config.setdefault("SESSION_LIFETIME", 2 * 60 * 60) config.setdefault("SESSION_RENEW_COUNT", 5) config.setdefault("SESSION_SIGNER_SALT", "session") diff --git a/securedrop/sdconfig.py b/securedrop/sdconfig.py index df07089d03..7810bfc23d 100644 --- a/securedrop/sdconfig.py +++ b/securedrop/sdconfig.py @@ -71,6 +71,8 @@ class SecureDropConfig: RQ_WORKER_NAME: str + REDIS_PASSWORD: str + env: str = "prod" @property @@ -88,6 +90,11 @@ def STORE_DIR(self) -> Path: def DATABASE_URI(self) -> str: return f"sqlite:///{self.DATABASE_FILE}" + @property + def REDIS_KWARGS(self) -> Dict[str, str]: + """kwargs to pass to `redis.Redis` constructor""" + return {"password": self.REDIS_PASSWORD} + @classmethod def get_current(cls) -> "SecureDropConfig": global _current_config @@ -114,6 +121,9 @@ def _parse_config_from_file(config_module_name: str) -> SecureDropConfig: final_worker_name = getattr(config_from_local_file, "RQ_WORKER_NAME", "default") final_scrypt_params = getattr(config_from_local_file, "SCRYPT_PARAMS", dict(N=2**14, r=8, p=1)) + final_redis_password = getattr(config_from_local_file, "REDIS_PASSWORD", None) + if final_redis_password is None: + raise RuntimeError("REDIS_PASSWORD must be set in config.py") env = getattr(config_from_local_file, "env", "prod") @@ -211,4 +221,5 @@ def _parse_config_from_file(config_module_name: str) -> SecureDropConfig: SUPPORTED_LOCALES=final_supp_locales, SESSION_EXPIRATION_MINUTES=final_sess_expiration_mins, RQ_WORKER_NAME=final_worker_name, + REDIS_PASSWORD=final_redis_password, ) diff --git a/securedrop/tests/config_from_2014.py b/securedrop/tests/config_from_2014.py index 59cf0776e6..7f2040be12 100644 --- a/securedrop/tests/config_from_2014.py +++ b/securedrop/tests/config_from_2014.py @@ -83,3 +83,7 @@ class JournalistInterfaceFlaskConfig(FlaskConfig): # depending on the environment DATABASE_ENGINE = "sqlite" DATABASE_FILE = os.path.join(SECUREDROP_ROOT, "db.sqlite") + +# Note: This was added in 2024, but it is generated in postinst in addition to +# ansible, so 2014-era SecureDrops will have it. +REDIS_PASSWORD = "1234567890" diff --git a/securedrop/tests/conftest.py b/securedrop/tests/conftest.py index 667f1a5860..4822ad8e9a 100644 --- a/securedrop/tests/conftest.py +++ b/securedrop/tests/conftest.py @@ -332,6 +332,8 @@ def _start_test_rqworker(worker_name: str, worker_pid_file: Path, securedrop_roo worker_name, "-P", securedrop_root, + "-c", + "rq_config", "--pid", worker_pid_file, "--logging_level", diff --git a/securedrop/tests/factories.py b/securedrop/tests/factories.py index a4b0443ae9..3ea832b65a 100644 --- a/securedrop/tests/factories.py +++ b/securedrop/tests/factories.py @@ -12,6 +12,8 @@ from tests.functional.db_session import _get_fake_db_module from tests.utils.db_helper import reset_database +REDIS_PASSWORD = Path("/tmp/redispasswd").read_text().strip() + def _generate_random_token() -> str: return secrets.token_hex(32) @@ -95,6 +97,7 @@ def create( SOURCE_TEMPLATES_DIR=DEFAULT_SECUREDROP_ROOT / "source_templates", JOURNALIST_TEMPLATES_DIR=DEFAULT_SECUREDROP_ROOT / "journalist_templates", DEFAULT_LOCALE=DEFAULT_LOCALE, + REDIS_PASSWORD=REDIS_PASSWORD, ) # Delete any previous/existing DB and initialize a new one diff --git a/securedrop/tests/test_encryption.py b/securedrop/tests/test_encryption.py index ffd577e027..1cb9b5e10e 100644 --- a/securedrop/tests/test_encryption.py +++ b/securedrop/tests/test_encryption.py @@ -5,6 +5,7 @@ from db import db from encryption import EncryptionManager, GpgDecryptError, GpgKeyNotFoundError from passphrases import PassphraseGenerator +from redis import Redis from source_user import create_source_user from tests import utils @@ -262,6 +263,7 @@ def test_get_source_secret_key_from_gpg(self, test_source, tmp_path, config): encryption_mgr = EncryptionManager( gpg_key_dir=tmp_path, journalist_pub_key=(config.SECUREDROP_DATA_ROOT / "journalist.pub"), + redis=Redis(decode_responses=True, **config.REDIS_KWARGS), ) new_fingerprint = utils.create_legacy_gpg_key(encryption_mgr, source_user, source) secret_key = encryption_mgr.get_source_secret_key_from_gpg( diff --git a/securedrop/tests/test_journalist_session.py b/securedrop/tests/test_journalist_session.py index 147b85b910..b23aef3b81 100644 --- a/securedrop/tests/test_journalist_session.py +++ b/securedrop/tests/test_journalist_session.py @@ -1,6 +1,7 @@ import json from datetime import datetime, timedelta, timezone +import pytest from flask import Response, url_for from flask.sessions import session_json_serializer from itsdangerous import URLSafeTimedSerializer @@ -9,11 +10,14 @@ from tests.utils.api_helper import get_api_headers from two_factor import TOTP -redis = Redis() - NEW_PASSWORD = "another correct horse battery staple generic passphrase" +@pytest.fixture() +def redis(config): + return Redis(**config.REDIS_KWARGS) + + # Helper function to check if session cookie are properly signed # Returns just the session id without signature def _check_sig(session_cookie, journalist_app, api=False): @@ -28,7 +32,7 @@ def _check_sig(session_cookie, journalist_app, api=False): # Helper function to get a session payload from redis # Returns the unserialized session payload -def _get_session(sid, journalist_app, api=False): +def _get_session(sid, journalist_app, redis, api=False): if api: key = "api_" + journalist_app.config["SESSION_KEY_PREFIX"] + sid else: @@ -51,7 +55,7 @@ def _session_from_cookiejar(cookie_jar, journalist_app): # Test a standard login sequence -def test_session_login(journalist_app, test_journo): +def test_session_login(journalist_app, test_journo, redis): # Given a test client and a valid journalist user with journalist_app.test_client() as app: # When sending a correct login request @@ -67,7 +71,7 @@ def test_session_login(journalist_app, test_journo): # Then such cookie is properly signed sid = _check_sig(session_cookie.value, journalist_app) # Then such session cookie has a corresponding payload in redis - redis_session = _get_session(sid, journalist_app) + redis_session = _get_session(sid, journalist_app, redis) ttl = redis.ttl(journalist_app.config["SESSION_KEY_PREFIX"] + sid) # Then the TTL of such key in redis conforms to the lifetime configuration @@ -87,7 +91,7 @@ def test_session_login(journalist_app, test_journo): # Test a standard session renewal sequence -def test_session_renew(journalist_app, test_journo): +def test_session_renew(journalist_app, test_journo, redis): # Given a test client and a valid journalist user with journalist_app.test_client() as app: # When sending a correct login request @@ -99,7 +103,7 @@ def test_session_renew(journalist_app, test_journo): assert session_cookie is not None sid = _check_sig(session_cookie.value, journalist_app) - redis_session = _get_session(sid, journalist_app) + redis_session = _get_session(sid, journalist_app, redis) # The `renew_count` must exists in the session payload and must be equal to the app config assert redis_session["renew_count"] == journalist_app.config["SESSION_RENEW_COUNT"] @@ -116,7 +120,7 @@ def test_session_renew(journalist_app, test_journo): assert resp.status_code == 200 # Then the corresponding renew_count in redis must have been decreased - redis_session = _get_session(sid, journalist_app) + redis_session = _get_session(sid, journalist_app, redis) assert redis_session["renew_count"] == (journalist_app.config["SESSION_RENEW_COUNT"] - 1) # Then the ttl must have been updated and the new lifetime must be > of app confing lifetime @@ -126,7 +130,7 @@ def test_session_renew(journalist_app, test_journo): # Test a standard login then logout sequence -def test_session_logout(journalist_app, test_journo): +def test_session_logout(journalist_app, test_journo, redis): # Given a test client and a valid journalist user with journalist_app.test_client() as app: # When sending a correct login request @@ -153,7 +157,7 @@ def test_session_logout(journalist_app, test_journo): # Test the user forced logout when an admin changes the user password -def test_session_admin_change_password_logout(journalist_app, test_journo, test_admin): +def test_session_admin_change_password_logout(journalist_app, test_journo, test_admin, redis): # Given a test client and a valid journalist user with journalist_app.test_client() as app: # When sending a correct login request @@ -204,7 +208,7 @@ def test_session_admin_change_password_logout(journalist_app, test_journo, test_ # Test the forced logout when the user changes its password -def test_session_change_password_logout(journalist_app, test_journo): +def test_session_change_password_logout(journalist_app, test_journo, redis): # Given a test client and a valid journalist user with journalist_app.test_client() as app: # When sending a correct login request @@ -260,7 +264,7 @@ def test_session_login_regenerate_sid(journalist_app, test_journo): # Test a standard `get_token` API login -def test_session_api_login(journalist_app, test_journo): +def test_session_api_login(journalist_app, test_journo, redis): # Given a test client and a valid journalist user with journalist_app.test_client() as app: # When sending a `get_token` request to the API with valid creds @@ -282,7 +286,7 @@ def test_session_api_login(journalist_app, test_journo): # Then such token is properly signed sid = _check_sig(resp.json["token"], journalist_app, api=True) - redis_session = _get_session(sid, journalist_app, api=True) + redis_session = _get_session(sid, journalist_app, redis, api=True) # Then the session id in redis match that of the credentials assert redis_session["uid"] == test_journo["id"] @@ -314,7 +318,7 @@ def test_session_api_login(journalist_app, test_journo): # test a standard login then logout from API -def test_session_api_logout(journalist_app, test_journo): +def test_session_api_logout(journalist_app, test_journo, redis): # Given a test client and a valid journalist user with journalist_app.test_client() as app: # When sending a valid login request and asking an API token @@ -428,7 +432,7 @@ def test_session_bad_signature(journalist_app, test_journo): # To avoid this, save_session() uses a setxx call, which writes in redis only if the key exists. # This does not apply when the session is new or is being regenerated. # Test that a deleted session cannot be rewritten by a race condition -def test_session_race_condition(mocker, journalist_app, test_journo): +def test_session_race_condition(mocker, journalist_app, test_journo, redis): # Given a test client and a valid journalist user with journalist_app.test_request_context() as app: # When manually creating a session in the context diff --git a/securedrop/tests/test_worker.py b/securedrop/tests/test_worker.py index 1cdc9a2d81..285e7dcded 100644 --- a/securedrop/tests/test_worker.py +++ b/securedrop/tests/test_worker.py @@ -24,6 +24,8 @@ def start_rq_worker(config, queue_name): "/opt/venvs/securedrop-app-code/bin/rqworker", "--path", config.SECUREDROP_ROOT, + "-c", + "rq_config", queue_name, ], preexec_fn=os.setsid, diff --git a/securedrop/worker.py b/securedrop/worker.py index 21a434b6b5..b60b9b7cec 100644 --- a/securedrop/worker.py +++ b/securedrop/worker.py @@ -7,6 +7,7 @@ from rq.queue import Queue from rq.registry import StartedJobRegistry from rq.worker import Worker, WorkerStatus +from sdconfig import SecureDropConfig def create_queue(name: str, timeout: int = 3600) -> Queue: @@ -15,7 +16,8 @@ def create_queue(name: str, timeout: int = 3600) -> Queue: If ``name`` is omitted, ``config.RQ_WORKER_NAME`` is used. """ - return Queue(name=name, connection=Redis(), default_timeout=timeout) + config = SecureDropConfig.get_current() + return Queue(name=name, connection=Redis(**config.REDIS_KWARGS), default_timeout=timeout) def rq_workers(queue: Queue = None) -> List[Worker]: @@ -23,7 +25,8 @@ def rq_workers(queue: Queue = None) -> List[Worker]: Returns the list of current rq ``Worker``s. """ - return Worker.all(connection=Redis(), queue=queue) + config = SecureDropConfig.get_current() + return Worker.all(connection=Redis(**config.REDIS_KWARGS), queue=queue) def worker_for_job(job_id: str) -> Optional[Worker]: From e59cafee539ad0f4370c7b4d86ba0adbfa17466c Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Wed, 21 Aug 2024 08:26:26 -0700 Subject: [PATCH 02/10] Document stdout dictionary structures in dev-config Co-authored-by: Cory Francis Myers --- securedrop/bin/dev-config | 1 + 1 file changed, 1 insertion(+) diff --git a/securedrop/bin/dev-config b/securedrop/bin/dev-config index 74633c3b22..244db80af3 100755 --- a/securedrop/bin/dev-config +++ b/securedrop/bin/dev-config @@ -7,6 +7,7 @@ from jinja2 import Environment, FileSystemLoader def generate_random(length): + # Emulate the {"stdout": "..."} dictionaries Ansible produces when configuring non-development environments. return {'stdout': base64.b64encode(os.urandom(length)).decode()} From 2855f4b1c54ca90a492fa311ac81eed7bbe1446b Mon Sep 17 00:00:00 2001 From: Cory Francis Myers Date: Mon, 12 Aug 2024 13:03:10 -0700 Subject: [PATCH 03/10] fix(/logout): require POST for CSRF protection Collateral changes here address cases where tests have: 1. done app.get("/logout") without checking either the response code or the result of the actual logout operation; or 2. assumed there will be exactly one button, belonging to exactly one form, on any given page. Co-authored-by: Kunal Mehta --- securedrop/journalist_app/__init__.py | 2 +- securedrop/journalist_app/main.py | 2 +- .../account_edit_hotp_secret.html | 2 +- .../journalist_templates/admin_add_user.html | 2 +- .../admin_edit_hotp_secret.html | 2 +- securedrop/journalist_templates/base.html | 5 +++- .../journalist_templates/edit_account.html | 2 +- securedrop/journalist_templates/login.html | 2 +- securedrop/source_app/main.py | 5 ++-- securedrop/source_app/utils.py | 9 ++++---- securedrop/source_templates/base.html | 6 +++-- securedrop/static/css/journalist.css | 4 ++++ .../app_navigators/journalist_app_nav.py | 10 ++++---- .../app_navigators/source_app_nav.py | 2 +- .../pageslayout/test_journalist_account.py | 4 +++- .../pageslayout/test_source_session_layout.py | 4 +++- .../tests/functional/test_admin_interface.py | 6 +++-- securedrop/tests/test_integration.py | 18 ++++++++++----- securedrop/tests/test_journalist.py | 21 +++++++++++++---- securedrop/tests/test_journalist_session.py | 2 +- securedrop/tests/test_source.py | 23 ++++++++++++++----- securedrop/tests/test_two_factor_in_apps.py | 5 ++-- securedrop/translations/messages.pot | 13 +++++++---- 23 files changed, 100 insertions(+), 51 deletions(-) diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index 5e3a87ed07..ad3381ea68 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -68,7 +68,7 @@ def default(self, obj: "Any") -> "Any": @app.errorhandler(CSRFError) def handle_csrf_error(e: CSRFError) -> "Response": app.logger.error("The CSRF token is invalid.") - msg = gettext("You have been logged out due to inactivity.") + msg = gettext("You have been logged out due to inactivity or a problem with your session.") session.destroy(("error", msg), session.get("locale")) return redirect(url_for("main.login")) diff --git a/securedrop/journalist_app/main.py b/securedrop/journalist_app/main.py index 4b6ccb9ac6..5a95feb241 100644 --- a/securedrop/journalist_app/main.py +++ b/securedrop/journalist_app/main.py @@ -57,7 +57,7 @@ def login() -> Union[str, werkzeug.Response]: return render_template("login.html") - @view.route("/logout") + @view.route("/logout", methods=("POST",)) def logout() -> werkzeug.Response: session.destroy() return redirect(url_for("main.index")) diff --git a/securedrop/journalist_templates/account_edit_hotp_secret.html b/securedrop/journalist_templates/account_edit_hotp_secret.html index 2a68c9e959..647323ab8f 100644 --- a/securedrop/journalist_templates/account_edit_hotp_secret.html +++ b/securedrop/journalist_templates/account_edit_hotp_secret.html @@ -4,7 +4,7 @@ {% block body %}

{{ gettext('Change Secret') }}

-
+
diff --git a/securedrop/journalist_templates/admin_add_user.html b/securedrop/journalist_templates/admin_add_user.html index 89356527ba..25228135ad 100644 --- a/securedrop/journalist_templates/admin_add_user.html +++ b/securedrop/journalist_templates/admin_add_user.html @@ -8,7 +8,7 @@

Add User

- + {{ form.password(value=password, id=False) }}
diff --git a/securedrop/journalist_templates/admin_edit_hotp_secret.html b/securedrop/journalist_templates/admin_edit_hotp_secret.html index 18b144a47e..ecbceabdf5 100644 --- a/securedrop/journalist_templates/admin_edit_hotp_secret.html +++ b/securedrop/journalist_templates/admin_edit_hotp_secret.html @@ -5,7 +5,7 @@ {% block body %}

{{ gettext('Change HOTP Secret')}}

{{ gettext("Enter a new HOTP secret formatted as a 40-digit hexadecimal string. Spaces will be ignored:") }}

- +
diff --git a/securedrop/journalist_templates/base.html b/securedrop/journalist_templates/base.html index bc6a836a90..af8e1b8ad3 100644 --- a/securedrop/journalist_templates/base.html +++ b/securedrop/journalist_templates/base.html @@ -25,7 +25,10 @@ {% if session.get_user() and session.get_user().is_admin %} {{ gettext('Admin') }} | {% endif %} - {{ gettext('Log Out') }} + + + + {% endif %} diff --git a/securedrop/journalist_templates/edit_account.html b/securedrop/journalist_templates/edit_account.html index bc7c833fab..d5fe25b3d1 100644 --- a/securedrop/journalist_templates/edit_account.html +++ b/securedrop/journalist_templates/edit_account.html @@ -18,7 +18,7 @@

{{ gettext('Edit user "{user}"').format(user=user.username) }}

{{ gettext('Change Name and Admin Status') }}

-
+
diff --git a/securedrop/journalist_templates/login.html b/securedrop/journalist_templates/login.html index 9e801a470b..157894b953 100644 --- a/securedrop/journalist_templates/login.html +++ b/securedrop/journalist_templates/login.html @@ -6,7 +6,7 @@

{{ gettext('Log in to access the journalist interface') }}

- +
diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index 2fd44e40df..55be14ef38 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -406,8 +406,9 @@ def login() -> Union[str, werkzeug.Response]: return redirect(url_for(".lookup", from_login="1")) - @view.route("/logout") - def logout() -> Union[str, werkzeug.Response]: + @view.route("/logout", methods=("POST",)) + @login_required + def logout(logged_in_source: SourceUser) -> Union[str, werkzeug.Response]: """ If a user is logged in, show them a logout page that prompts them to click the New Identity button in Tor Browser to complete their session. diff --git a/securedrop/source_app/utils.py b/securedrop/source_app/utils.py index 88ddc56e78..a349ce663f 100644 --- a/securedrop/source_app/utils.py +++ b/securedrop/source_app/utils.py @@ -50,10 +50,11 @@ def clear_session_and_redirect_to_logged_out_page(flask_session: SessionMixin) - declarative=gettext("Important"), msg_contents=Markup( gettext( - 'You were logged out due to inactivity. Click the  New Identity button in your Tor Browser\'s ' - "toolbar before moving on. This will clear your Tor Browser activity data on " - "this device." + "You have been logged out due to inactivity or a problem with " + 'your session. Click the  New Identity button in your Tor ' + "Browser's toolbar before moving on. This will clear your Tor " + "Browser activity data on this device." ).format(icon=url_for("static", filename="i/torbroom.png")) ), ) diff --git a/securedrop/source_templates/base.html b/securedrop/source_templates/base.html index 659b7c2b92..d08fe1ee78 100644 --- a/securedrop/source_templates/base.html +++ b/securedrop/source_templates/base.html @@ -41,8 +41,10 @@

{{ gettext("We're sorry, our SecureDrop is currently offline.") }}

{% if is_user_logged_in %} {% endif %} diff --git a/securedrop/static/css/journalist.css b/securedrop/static/css/journalist.css index 4a3e5c40ff..be1fa8e241 100644 --- a/securedrop/static/css/journalist.css +++ b/securedrop/static/css/journalist.css @@ -1884,6 +1884,10 @@ body > nav { margin: 1em 1em 0 1em; } +body > nav form { + display: inline; +} + label { cursor: pointer; } diff --git a/securedrop/tests/functional/app_navigators/journalist_app_nav.py b/securedrop/tests/functional/app_navigators/journalist_app_nav.py index ecd192e16c..1280a0000a 100644 --- a/securedrop/tests/functional/app_navigators/journalist_app_nav.py +++ b/securedrop/tests/functional/app_navigators/journalist_app_nav.py @@ -55,7 +55,7 @@ def journalist_goes_to_login_page_and_enters_credentials( self.nav_helper.safe_send_keys_by_css_selector('input[name="token"]', otp.now()) if should_submit_login_form: - self.nav_helper.safe_click_by_css_selector('button[type="submit"]') + self.nav_helper.safe_click_by_css_selector('form#login button[type="submit"]') def journalist_logs_in( self, @@ -71,7 +71,7 @@ def journalist_logs_in( ) # Successful login should redirect to the index - self.nav_helper.wait_for(lambda: self.driver.find_element(By.ID, "link-logout")) + self.nav_helper.wait_for(lambda: self.driver.find_element(By.ID, "btn-logout")) assert self.is_on_journalist_homepage() def journalist_checks_messages(self) -> None: @@ -259,7 +259,7 @@ def admin_creates_a_user( callback_before_submitting_add_user_step() # Submit the form - self.nav_helper.safe_click_by_css_selector("button[type=submit]") + self.nav_helper.safe_click_by_css_selector("form#admin-add-user button[type=submit]") # Submitting the add user form should redirect to the 2FA page self.nav_helper.wait_for(lambda: self.driver.find_element(By.ID, "check-token")) @@ -290,7 +290,7 @@ def admin_creates_a_user( if callback_before_submitting_2fa_step: callback_before_submitting_2fa_step() - self.nav_helper.safe_click_by_css_selector("button[type=submit]") + self.nav_helper.safe_click_by_css_selector("form#check-token button[type=submit]") # Verify the two-factor authentication def user_token_added(): @@ -311,7 +311,7 @@ def user_token_added(): def journalist_logs_out(self) -> None: # Click the logout link - self.nav_helper.safe_click_by_id("link-logout") + self.nav_helper.safe_click_by_id("logout") self.nav_helper.wait_for(lambda: self.driver.find_element(By.CSS_SELECTOR, ".login-form")) # Logging out should redirect back to the login page diff --git a/securedrop/tests/functional/app_navigators/source_app_nav.py b/securedrop/tests/functional/app_navigators/source_app_nav.py index 55e11ab067..ddf5aa189a 100644 --- a/securedrop/tests/functional/app_navigators/source_app_nav.py +++ b/securedrop/tests/functional/app_navigators/source_app_nav.py @@ -102,7 +102,7 @@ def source_chooses_to_login(self) -> None: self.nav_helper.wait_for(lambda: self.driver.find_elements(By.ID, "source-login")) def _is_logged_in(self) -> WebElement: - return self.nav_helper.wait_for(lambda: self.driver.find_element(By.ID, "logout")) + return self.nav_helper.wait_for(lambda: self.driver.find_element(By.ID, "btn-logout")) def source_proceeds_to_login(self, codename: str) -> None: self.nav_helper.safe_send_keys_by_id("codename", codename) diff --git a/securedrop/tests/functional/pageslayout/test_journalist_account.py b/securedrop/tests/functional/pageslayout/test_journalist_account.py index faab09d7ee..376bade7d1 100644 --- a/securedrop/tests/functional/pageslayout/test_journalist_account.py +++ b/securedrop/tests/functional/pageslayout/test_journalist_account.py @@ -57,7 +57,9 @@ def test_account_edit_and_set_hotp_secret( journ_app_nav.nav_helper.safe_send_keys_by_css_selector( 'input[name="otp_secret"]', "123456" ) - journ_app_nav.nav_helper.safe_click_by_css_selector("button[type=submit]") + journ_app_nav.nav_helper.safe_click_by_css_selector( + "form#account-edit-hotp-secret button[type=submit]" + ) save_static_data(journ_app_nav.driver, locale, "journalist-account_new_two_factor_hotp") @staticmethod diff --git a/securedrop/tests/functional/pageslayout/test_source_session_layout.py b/securedrop/tests/functional/pageslayout/test_source_session_layout.py index 304abc399f..e234ef25c6 100644 --- a/securedrop/tests/functional/pageslayout/test_source_session_layout.py +++ b/securedrop/tests/functional/pageslayout/test_source_session_layout.py @@ -61,7 +61,9 @@ def test_source_session_timeout(self, locale, sd_servers_with_short_timeout): notification = navigator.driver.find_element(By.CLASS_NAME, "error") assert notification.text if locale == "en_US": - expected_text = "You were logged out due to inactivity." + expected_text = ( + "You have been logged out due to inactivity or a problem with your session." + ) assert expected_text in notification.text save_static_data(navigator.driver, locale, "source-session_timeout") diff --git a/securedrop/tests/functional/test_admin_interface.py b/securedrop/tests/functional/test_admin_interface.py index 82c5152475..4a017ce8fe 100644 --- a/securedrop/tests/functional/test_admin_interface.py +++ b/securedrop/tests/functional/test_admin_interface.py @@ -126,7 +126,9 @@ class StopAfterFormSubmitted(Exception): pass def submit_form_and_stop(): - journ_app_nav.nav_helper.safe_click_by_css_selector("button[type=submit]") + journ_app_nav.nav_helper.safe_click_by_css_selector( + "form#admin-add-user button[type=submit]" + ) raise StopAfterFormSubmitted() try: @@ -277,7 +279,7 @@ def _admin_edits_username_and_submits_form( journ_app_nav.nav_helper.safe_send_keys_by_css_selector( 'input[name="username"]', new_username ) - journ_app_nav.nav_helper.safe_click_by_css_selector("button[type=submit]") + journ_app_nav.nav_helper.safe_click_by_css_selector("form#edit-account button[type=submit]") def test_admin_resets_password(self, sd_servers_with_second_journalist, firefox_web_driver): # Given an SD server with a second journalist created diff --git a/securedrop/tests/test_integration.py b/securedrop/tests/test_integration.py index a5016fef23..ceafe350dd 100644 --- a/securedrop/tests/test_integration.py +++ b/securedrop/tests/test_integration.py @@ -45,7 +45,8 @@ def test_submit_message(journalist_app, source_app, test_journo, app_storage): follow_redirects=True, ) assert resp.status_code == 200 - app.get("/logout") + resp = app.post("/logout") + assert resp.status_code == 200 # Request the Journalist Interface index with journalist_app.test_client() as app: @@ -140,7 +141,8 @@ def test_submit_file(journalist_app, source_app, test_journo, app_storage): follow_redirects=True, ) assert resp.status_code == 200 - app.get("/logout") + resp = app.post("/logout") + assert resp.status_code == 200 with journalist_app.test_client() as app: login_journalist( @@ -236,7 +238,8 @@ def _helper_test_reply(journalist_app, source_app, test_journo, test_reply): assert resp.status_code == 200 source_user = SessionManager.get_logged_in_user(db_session=db.session) filesystem_id = source_user.filesystem_id - app.get("/logout") + resp = app.post("/logout") + assert resp.status_code == 200 with journalist_app.test_client() as app: login_journalist( @@ -320,7 +323,8 @@ def _helper_test_reply(journalist_app, source_app, test_journo, test_reply): text = resp.data.decode("utf-8") assert "Reply deleted" in text - app.get("/logout") + resp = app.post("/logout") + assert resp.status_code == 200 def _helper_filenames_delete(journalist_app, soup, i): @@ -456,7 +460,8 @@ def test_delete_collections(mocker, journalist_app, source_app, test_journo): ), follow_redirects=True, ) - app.get("/logout") + resp = app.post("/logout") + assert resp.status_code == 200 with journalist_app.test_client() as app: login_journalist( @@ -615,7 +620,8 @@ def test_user_change_password(journalist_app, test_journo): ), ) # logout - app.get("/logout") + resp = app.post("/logout") + assert resp.status_code == 302 # start a new client/context to be sure we've cleared the session with journalist_app.test_client() as app: diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 02ddd9e91d..0f2fab40cf 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -472,7 +472,7 @@ def test_admin_logout_redirects_to_index(config, journalist_app, test_admin): test_admin["password"], test_admin["otp_secret"], ) - resp = app.get(url_for("main.logout")) + resp = app.post(url_for("main.logout")) ins.assert_redirects(resp, url_for("main.index")) @@ -485,7 +485,7 @@ def test_user_logout_redirects_to_index(config, journalist_app, test_journo): test_journo["password"], test_journo["otp_secret"], ) - resp = app.get(url_for("main.logout")) + resp = app.post(url_for("main.logout")) ins.assert_redirects(resp, url_for("main.index")) @@ -3577,16 +3577,27 @@ def test_csrf_error_page(config, journalist_app, locale): journalist_app.config["WTF_CSRF_ENABLED"] = True with journalist_app.test_client() as app: + # /login without a CSRF token should redirect... with InstrumentedApp(journalist_app) as ins: resp = app.post(url_for("main.login")) ins.assert_redirects(resp, url_for("main.login")) resp = app.post(url_for("main.login"), follow_redirects=True) - # because the session is being cleared when it expires, the - # response should always be in English. + # ...and show an error. (Because the session is being cleared when it + # expires, the response should always be in English.) assert page_language(resp.data) == "en-US" - assert "You have been logged out due to inactivity." in resp.data.decode("utf-8") + assert ( + "You have been logged out due to inactivity or a problem with your session." + in resp.data.decode("utf-8") + ) + + # /logout without a CSRF token should also error. + resp = app.post(url_for("main.logout"), follow_redirects=True) + assert ( + "You have been logged out due to inactivity or a problem with your session." + in resp.data.decode("utf-8") + ) def test_col_process_aborts_with_bad_action(journalist_app, test_journo): diff --git a/securedrop/tests/test_journalist_session.py b/securedrop/tests/test_journalist_session.py index 147b85b910..90302fc517 100644 --- a/securedrop/tests/test_journalist_session.py +++ b/securedrop/tests/test_journalist_session.py @@ -141,7 +141,7 @@ def test_session_logout(journalist_app, test_journo): assert (redis.get(journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is not None # When sending a logout request from a logged in journalist - resp = app.get(url_for("main.logout"), follow_redirects=False) + resp = app.post(url_for("main.logout"), follow_redirects=False) # Then it redirects to login assert resp.status_code == 302 # Then the session no longer exists in redis diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 4c92918cba..2104750c50 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -272,7 +272,8 @@ def test_login_and_logout(source_app): assert resp.status_code == 200 assert SessionManager.is_user_logged_in(db_session=db.session) - resp = app.get(url_for("main.logout"), follow_redirects=True) + resp = app.post(url_for("main.logout"), follow_redirects=True) + assert resp.status_code == 200 assert not SessionManager.is_user_logged_in(db_session=db.session) text = resp.data.decode("utf-8") @@ -600,7 +601,8 @@ def test_submit_codename_second_login(source_app): text = resp.data.decode("utf-8") assert "Please do not submit your codename!" in text - resp = app.get(url_for("main.logout"), follow_redirects=True) + resp = app.post(url_for("main.logout"), follow_redirects=True) + assert resp.status_code == 200 assert not SessionManager.is_user_logged_in(db_session=db.session) text = resp.data.decode("utf-8") assert "This will clear your Tor Browser activity data" in text @@ -948,7 +950,7 @@ def test_source_session_expiration(source_app): # They get redirected to the index page with the "logged out" message text = resp.data.decode("utf-8") - assert "You were logged out due to inactivity" in text + assert "You have been logged out due to inactivity or a problem with your session." in text def test_source_session_expiration_create(source_app): @@ -967,7 +969,7 @@ def test_source_session_expiration_create(source_app): # They get redirected to the index page with the "logged out" message text = resp.data.decode("utf-8") - assert "You were logged out due to inactivity" in text + assert "You have been logged out due to inactivity or a problem with your session." in text def test_source_no_session_expiration_message_when_not_logged_in(source_app): @@ -986,19 +988,28 @@ def test_source_no_session_expiration_message_when_not_logged_in(source_app): # The session expiration message is NOT displayed text = refreshed_resp.data.decode("utf-8") - assert "You were logged out due to inactivity" not in text + assert ( + "You have been logged out due to inactivity or a problem with your session." not in text + ) def test_csrf_error_page(source_app): source_app.config["WTF_CSRF_ENABLED"] = True with source_app.test_client() as app: + # /create without a CSRF token should redirect... with InstrumentedApp(source_app) as ins: resp = app.post(url_for("main.create")) ins.assert_redirects(resp, url_for("main.index")) + # ...and show an error message. resp = app.post(url_for("main.create"), follow_redirects=True) text = resp.data.decode("utf-8") - assert "You were logged out due to inactivity" in text + assert "You have been logged out due to inactivity or a problem with your session." in text + + # /logout without a CSRF token should also error. + resp = app.post(url_for("main.logout"), follow_redirects=True) + text = resp.data.decode("utf-8") + assert "You have been logged out due to inactivity or a problem with your session." in text def test_source_can_only_delete_own_replies(source_app, app_storage): diff --git a/securedrop/tests/test_two_factor_in_apps.py b/securedrop/tests/test_two_factor_in_apps.py index b3ee28ec6a..184ffe832b 100644 --- a/securedrop/tests/test_two_factor_in_apps.py +++ b/securedrop/tests/test_two_factor_in_apps.py @@ -27,7 +27,7 @@ def test_rejects_already_used_totp_token(self, journalist_app, test_journo): ) assert resp1.status_code == 200 - resp2 = app.get(url_for("main.logout"), follow_redirects=True) + resp2 = app.post(url_for("main.logout"), follow_redirects=True) assert resp2.status_code == 200 with journalist_app.app_context(): @@ -140,7 +140,8 @@ def test_can_login_after_regenerating_hotp(self, journalist_app, test_journo): ) # And they then log out - app.get("/logout") + resp = app.post("/logout") + assert resp.status_code == 302 # When they later try to login using a 2fa token based on their new HOTP secret with journalist_app.test_client() as app, InstrumentedApp(journalist_app) as ins: diff --git a/securedrop/translations/messages.pot b/securedrop/translations/messages.pot index 9c197321b7..22da780b54 100644 --- a/securedrop/translations/messages.pot +++ b/securedrop/translations/messages.pot @@ -33,7 +33,7 @@ msgstr "" msgid "{time} ago" msgstr "" -msgid "You have been logged out due to inactivity." +msgid "You have been logged out due to inactivity or a problem with your session." msgstr "" msgid "SecureDrop" @@ -164,6 +164,9 @@ msgstr "" msgid "No unread submissions for this source." msgstr "" +msgid "You have been logged out due to inactivity." +msgstr "" + msgid "Account updated." msgstr "" @@ -431,6 +434,9 @@ msgstr "" msgid "Log Out" msgstr "" +msgid "LOG OUT" +msgstr "" + msgid "Return to All Sources" msgstr "" @@ -821,7 +827,7 @@ msgstr "" msgid "Important" msgstr "" -msgid "You were logged out due to inactivity. Click the \"\" New Identity button in your Tor Browser's toolbar before moving on. This will clear your Tor Browser activity data on this device." +msgid "You have been logged out due to inactivity or a problem with your session. Click the \"\" New Identity button in your Tor Browser's toolbar before moving on. This will clear your Tor Browser activity data on this device." msgstr "" msgid "Protecting Journalists and Sources" @@ -842,9 +848,6 @@ msgstr "" msgid "Please try again later. Check our website for more information." msgstr "" -msgid "LOG OUT" -msgstr "" - msgid "Server error" msgstr "" From b037afe8dae163463e9d3748bbf87be8c542390e Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Mon, 12 Aug 2024 18:10:18 -0400 Subject: [PATCH 04/10] Validate user provided same password back to server We require that all users have a strong diceware password by generating it ourselves for them to use. However, we accept any diceware-looking sent in the POST request, meaning that a user can deliberately set a weak or fixed password. (It would be difficult for an attacker to do so since they'd need to both bypass the onion service authentication and CSRF.) To fix this, upon generating a new password for users, we save a hash of it in the user's session. When they save the password, we validate it against the hash and if matches, allow it to be saved. This ends up being mostly straightforward (outside of tests), requiring modifications to utils.set_diceware_password() and the creation of utils.set_pending_password() and utils.verify_pending_password(). Tests are a bit more complicated because we rely on being able to send arbitrary POST strings to the correct endpoint, so it needs to manually be set in the session. New tests for this functionality are added as well. This addresses "SEC-01-002 WP4: Possible Account Takeover via Weak Password Policy (Low)" Fixes . --- securedrop/journalist_app/account.py | 3 + securedrop/journalist_app/admin.py | 13 +- securedrop/journalist_app/utils.py | 43 +++- securedrop/tests/test_integration.py | 2 + securedrop/tests/test_journalist.py | 262 +++++++++++++++++++- securedrop/tests/test_journalist_session.py | 4 +- securedrop/tests/utils/__init__.py | 19 ++ 7 files changed, 339 insertions(+), 7 deletions(-) diff --git a/securedrop/journalist_app/account.py b/securedrop/journalist_app/account.py index d99821cd32..a516c4c617 100644 --- a/securedrop/journalist_app/account.py +++ b/securedrop/journalist_app/account.py @@ -8,6 +8,7 @@ from journalist_app.utils import ( set_diceware_password, set_name, + set_pending_password, validate_hotp_secret, validate_user, ) @@ -23,6 +24,8 @@ def edit() -> str: password = PassphraseGenerator.get_default().generate_passphrase( preferred_language=g.localeinfo.language ) + # Store password in session for future verification + set_pending_password(session.get_user(), password) return render_template("edit_account.html", password=password) @view.route("/change-name", methods=("POST",)) diff --git a/securedrop/journalist_app/admin.py b/securedrop/journalist_app/admin.py index 1a0723e343..699c4dff5a 100644 --- a/securedrop/journalist_app/admin.py +++ b/securedrop/journalist_app/admin.py @@ -19,7 +19,13 @@ from journalist_app.decorators import admin_required from journalist_app.forms import LogoForm, NewUserForm, OrgNameForm, SubmissionPreferencesForm from journalist_app.sessions import session -from journalist_app.utils import commit_account_changes, set_diceware_password, validate_hotp_secret +from journalist_app.utils import ( + commit_account_changes, + set_diceware_password, + set_pending_password, + validate_hotp_secret, + verify_pending_password, +) from models import ( FirstOrLastNameError, InstanceConfig, @@ -152,6 +158,7 @@ def add_user() -> Union[str, werkzeug.Response]: otp_secret = None if request.form.get("is_hotp", False): otp_secret = request.form.get("otp_secret", "") + verify_pending_password(for_="new", passphrase=password) new_user = Journalist( username=username, password=password, @@ -215,6 +222,8 @@ def add_user() -> Union[str, werkzeug.Response]: password = PassphraseGenerator.get_default().generate_passphrase( preferred_language=g.localeinfo.language ) + # Store password in session for future verification + set_pending_password("new", password) return render_template("admin_add_user.html", password=password, form=form) @view.route("/2fa", methods=("GET", "POST")) @@ -321,6 +330,8 @@ def edit_user(user_id: int) -> Union[str, werkzeug.Response]: password = PassphraseGenerator.get_default().generate_passphrase( preferred_language=g.localeinfo.language ) + # Store password in session for future verification + set_pending_password(user, password) return render_template("edit_account.html", user=user, password=password) @view.route("/delete/", methods=("POST",)) diff --git a/securedrop/journalist_app/utils.py b/securedrop/journalist_app/utils.py index 0260e021b0..a543221f63 100644 --- a/securedrop/journalist_app/utils.py +++ b/securedrop/journalist_app/utils.py @@ -1,8 +1,9 @@ import binascii import os from datetime import datetime, timezone -from typing import List, Optional, Union +from typing import List, Literal, Optional, Union +import argon2 import flask import werkzeug from db import db @@ -12,6 +13,7 @@ from journalist_app.sessions import session from markupsafe import Markup, escape from models import ( + ARGON2_PARAMS, FirstOrLastNameError, InvalidPasswordLength, InvalidUsernameException, @@ -427,10 +429,49 @@ def set_name(user: Journalist, first_name: Optional[str], last_name: Optional[st flash(gettext("Name not updated: {message}").format(message=e), "error") +def set_pending_password(for_: Union[Journalist, Literal["new"]], passphrase: str) -> None: + """ + The user has requested a password change, but hasn't confirmed it yet. + + NOTE: This mutates the current session and not the database. + + We keep track of the hash so we can verify they are using the password + we provided for them. It is expected they hit /new-password → + utils.set_diceware_password() + """ + hasher = argon2.PasswordHasher(**ARGON2_PARAMS) + # Include the user's id in the hash to avoid possible collisions in case we're + # resetting someone else's password. + if isinstance(for_, Journalist): + id = str(for_.id) + else: # "new" + id = for_ + session[f"pending_password_{id}"] = hasher.hash(passphrase) + + +def verify_pending_password(for_: Union[Journalist, Literal["new"]], passphrase: str) -> None: + if isinstance(for_, Journalist): + id = str(for_.id) + else: # "new" + id = for_ + pending_password_hash = session.get(f"pending_password_{id}") + if pending_password_hash is None: + raise PasswordError() + hasher = argon2.PasswordHasher(**ARGON2_PARAMS) + try: + hasher.verify(pending_password_hash, passphrase) + except argon2.exceptions.VerificationError: + raise PasswordError() + + def set_diceware_password( user: Journalist, password: Optional[str], admin: Optional[bool] = False ) -> bool: try: + if password is not None: + # FIXME: password being None will trigger an error in set_password(), we + # should turn it into a type error + verify_pending_password(user, password) # nosemgrep: python.django.security.audit.unvalidated-password.unvalidated-password user.set_password(password) except PasswordError: diff --git a/securedrop/tests/test_integration.py b/securedrop/tests/test_integration.py index a5016fef23..9caaaa1db7 100644 --- a/securedrop/tests/test_integration.py +++ b/securedrop/tests/test_integration.py @@ -606,6 +606,8 @@ def test_user_change_password(journalist_app, test_journo): # change password new_pw = "another correct horse battery staply long password" assert new_pw != test_journo["password"] # precondition + utils.prepare_password_change(app, test_journo["id"], new_pw) + app.post( "/account/new-password", data=dict( diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 02ddd9e91d..be332f2eb9 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -600,6 +600,7 @@ def test_admin_cannot_edit_own_password_without_validation( login_journalist( app, test_admin["username"], test_admin["password"], test_admin["otp_secret"] ) + utils.prepare_password_change(app, test_admin["id"], VALID_PASSWORD) resp = app.post( url_for("admin.new_password", user_id=test_admin["id"], l=locale), @@ -618,6 +619,7 @@ def test_admin_cannot_edit_own_password_without_validation( def test_admin_edits_user_password_success_response( config, journalist_app, test_admin, test_journo, locale ): + """Verify the flow of an admin resetting another journalist's password""" with journalist_app.test_client() as app: login_journalist( app, @@ -625,10 +627,14 @@ def test_admin_edits_user_password_success_response( test_admin["password"], test_admin["otp_secret"], ) + # First obtain a randomly generated password from the server + first = app.get(url_for("admin.edit_user", user_id=test_journo["id"], l=locale)) + password = utils.extract_password(first.data) + # Then send it back resp = app.post( url_for("admin.new_password", user_id=test_journo["id"], l=locale), - data=dict(password=VALID_PASSWORD_2), + data=dict(password=password), follow_redirects=True, ) assert resp.status_code == 200 @@ -639,7 +645,7 @@ def test_admin_edits_user_password_success_response( with xfail_untranslated_messages(config, locale, msgids): resp_text = resp.data.decode("utf-8") assert escape(gettext(msgids[0])) in resp_text - assert VALID_PASSWORD_2 in resp_text + assert password in resp_text @flaky(rerun_filter=utils.flaky_filter_xfail) @@ -664,6 +670,7 @@ def test_admin_edits_user_password_session_invalidate( test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(admin_app, test_journo["id"], VALID_PASSWORD_2) resp = admin_app.post( url_for("admin.new_password", user_id=test_journo["id"], l=locale), @@ -681,6 +688,68 @@ def test_admin_edits_user_password_session_invalidate( assert VALID_PASSWORD_2 in resp_text +@flaky(rerun_filter=utils.flaky_filter_xfail) +@pytest.mark.parametrize("locale", get_test_locales()) +def test_admin_edits_user_password_invalid_different( + config, journalist_app, test_admin, test_journo, locale +): + """Test if the admin submits a different password than the one they received""" + with journalist_app.test_client() as app: + login_journalist( + app, + test_admin["username"], + test_admin["password"], + test_admin["otp_secret"], + ) + # First obtain a randomly generated password from the server + first = app.get(url_for("admin.edit_user", user_id=test_journo["id"], l=locale)) + password = utils.extract_password(first.data) + + # No freak random collisions + assert password != VALID_PASSWORD_2 + + # Then send back a different password + resp = app.post( + url_for("admin.new_password", user_id=test_journo["id"], l=locale), + data=dict(password=VALID_PASSWORD_2), + follow_redirects=True, + ) + assert resp.status_code == 200 + assert page_language(resp.data) == language_tag(locale) + msgids = ["The password you submitted is invalid. Password not changed."] + with xfail_untranslated_messages(config, locale, msgids): + resp_text = resp.data.decode("utf-8") + assert escape(gettext(msgids[0])) in resp_text + + +@flaky(rerun_filter=utils.flaky_filter_xfail) +@pytest.mark.parametrize("locale", get_test_locales()) +def test_admin_edits_user_password_invalid_nopending( + config, journalist_app, test_admin, test_journo, locale +): + """Test if the user submits a password without receiving one from the server first""" + with journalist_app.test_client() as app: + login_journalist( + app, + test_admin["username"], + test_admin["password"], + test_admin["otp_secret"], + ) + + # We explicitly do not fetch a password from the server nor set a pending one + resp = app.post( + url_for("admin.new_password", user_id=test_journo["id"], l=locale), + data=dict(password=VALID_PASSWORD_2), + follow_redirects=True, + ) + assert resp.status_code == 200 + assert page_language(resp.data) == language_tag(locale) + msgids = ["The password you submitted is invalid. Password not changed."] + with xfail_untranslated_messages(config, locale, msgids): + resp_text = resp.data.decode("utf-8") + assert escape(gettext(msgids[0])) in resp_text + + def test_admin_deletes_invalid_user_404(journalist_app, test_admin): with journalist_app.app_context(): invalid_id = db.session.query(func.max(Journalist.id)).scalar() + 1 @@ -725,6 +794,7 @@ def test_admin_edits_user_password_error_response( test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, test_journo["id"], VALID_PASSWORD_2) with patch("sqlalchemy.orm.scoping.scoped_session.commit", side_effect=Exception()): with InstrumentedApp(journalist_app) as ins: @@ -750,6 +820,7 @@ def test_user_edits_password_success_response(config, journalist_app, test_journ login_journalist( app, test_journo["username"], test_journo["password"], test_journo["otp_secret"] ) + utils.prepare_password_change(app, test_journo["id"], VALID_PASSWORD_2) token = TOTP(test_journo["otp_secret"]).now() resp = app.post( url_for("account.new_password", l=locale), @@ -776,6 +847,7 @@ def test_user_edits_password_expires_session(journalist_app, test_journo): app, test_journo["username"], test_journo["password"], test_journo["otp_secret"] ) assert "uid" in session + utils.prepare_password_change(app, test_journo["id"], VALID_PASSWORD_2) with InstrumentedApp(journalist_app) as ins: token = TOTP(test_journo["otp_secret"]).now() @@ -802,6 +874,7 @@ def test_user_edits_password_error_response(config, journalist_app, test_journo, login_journalist( app, test_journo["username"], test_journo["password"], test_journo["otp_secret"] ) + utils.prepare_password_change(app, test_journo["id"], VALID_PASSWORD_2) # patch token verification because there are multiple commits # to the database and this isolates the one we want to fail @@ -840,6 +913,7 @@ def test_admin_add_user_when_username_already_taken(config, journalist_app, test test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(client, "new", VALID_PASSWORD) with InstrumentedApp(journalist_app) as ins: resp = client.post( url_for("admin.add_user", l=locale), @@ -914,6 +988,8 @@ def test_admin_edits_user_password_too_long_warning(journalist_app, test_admin, test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, test_journo["id"], overly_long_password) + with InstrumentedApp(journalist_app) as ins: app.post( url_for("admin.new_password", user_id=test_journo["id"]), @@ -945,6 +1021,7 @@ def test_user_edits_password_too_long_warning(config, journalist_app, test_journ test_journo["password"], test_journo["otp_secret"], ) + utils.prepare_password_change(app, test_journo["id"], overly_long_password) with InstrumentedApp(journalist_app) as ins: app.post( @@ -980,6 +1057,7 @@ def test_admin_add_user_password_too_long_warning(config, journalist_app, test_a test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", overly_long_password) with InstrumentedApp(journalist_app) as ins: resp = app.post( @@ -1014,6 +1092,7 @@ def test_admin_add_user_first_name_too_long_warning(config, journalist_app, test test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1047,6 +1126,7 @@ def test_admin_add_user_last_name_too_long_warning(config, journalist_app, test_ test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1477,6 +1557,7 @@ def test_http_get_on_admin_add_user_page(config, journalist_app, test_admin, loc def test_admin_add_user(journalist_app, test_admin): + """Test the workflow of adding a new journalist""" username = "dellsberg" with journalist_app.test_client() as app: @@ -1487,6 +1568,11 @@ def test_admin_add_user(journalist_app, test_admin): test_admin["otp_secret"], ) + # Get the autogenerated password + first = app.get(url_for("admin.add_user")) + assert first.status_code == 200 + password = utils.extract_password(first.data) + with InstrumentedApp(journalist_app) as ins: resp = app.post( url_for("admin.add_user"), @@ -1494,7 +1580,7 @@ def test_admin_add_user(journalist_app, test_admin): username=username, first_name="", last_name="", - password=VALID_PASSWORD, + password=password, otp_secret="", is_admin=None, ), @@ -1504,6 +1590,83 @@ def test_admin_add_user(journalist_app, test_admin): ins.assert_redirects(resp, url_for("admin.new_user_two_factor", uid=new_user.id)) +@flaky(rerun_filter=utils.flaky_filter_xfail) +@pytest.mark.parametrize("locale", get_test_locales()) +def test_admin_add_user_invalid_different_password(config, journalist_app, test_admin, locale): + """The admin submits a different password than the one given""" + username = "dellsberg" + + with journalist_app.test_client() as app: + login_journalist( + app, + test_admin["username"], + test_admin["password"], + test_admin["otp_secret"], + ) + + # Get the autogenerated password + first = app.get(url_for("admin.add_user", l=locale)) + assert first.status_code == 200 + password = utils.extract_password(first.data) + + # No freak random collisions + assert password != VALID_PASSWORD + + resp = app.post( + url_for("admin.add_user", l=locale), + data=dict( + username=username, + first_name="", + last_name="", + password=VALID_PASSWORD, + otp_secret="", + is_admin=None, + ), + ) + assert page_language(resp.data) == language_tag(locale) + msgids = [ + "There was an error with the autogenerated password. " + "User not created. Please try again." + ] + with xfail_untranslated_messages(config, locale, msgids): + assert escape(gettext(msgids[0])) in resp.data.decode("utf-8") + + +@flaky(rerun_filter=utils.flaky_filter_xfail) +@pytest.mark.parametrize("locale", get_test_locales()) +def test_admin_add_user_invalid_nopending(config, journalist_app, test_admin, locale): + """The admin does not get a random password from the server""" + username = "dellsberg" + + with journalist_app.test_client() as app: + login_journalist( + app, + test_admin["username"], + test_admin["password"], + test_admin["otp_secret"], + ) + + # We explicitly do not fetch a password from the server nor set a pending one + resp = app.post( + url_for("admin.add_user", l=locale), + data=dict( + username=username, + first_name="", + last_name="", + password=VALID_PASSWORD, + otp_secret="", + is_admin=None, + ), + ) + assert page_language(resp.data) == language_tag(locale) + msgids = [ + "There was an error with the autogenerated password. " + "User not created. Please try again." + ] + with xfail_untranslated_messages(config, locale, msgids): + assert escape(gettext(msgids[0])) in resp.data.decode("utf-8") + + @flaky(rerun_filter=utils.flaky_filter_xfail) @pytest.mark.parametrize("locale", get_test_locales()) def test_admin_add_user_with_invalid_username(config, journalist_app, test_admin, locale): @@ -1516,6 +1679,7 @@ def test_admin_add_user_with_invalid_username(config, journalist_app, test_admin test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1596,6 +1760,7 @@ def test_admin_add_user_without_username(config, journalist_app, test_admin, loc test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1627,6 +1792,9 @@ def test_admin_add_user_too_short_username(config, journalist_app, test_admin, l test_admin["password"], test_admin["otp_secret"], ) + # this will never be possible in real circumstances, but verify that later + # checks would reject such a password + utils.prepare_password_change(app, "new", "pentagonpapers") resp = app.post( url_for("admin.add_user", l=locale), @@ -1668,6 +1836,7 @@ def test_admin_add_user_yubikey_odd_length(config, journalist_app, test_admin, l test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1706,6 +1875,7 @@ def test_admin_add_user_yubikey_blank_secret(config, journalist_app, test_admin, test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1740,6 +1910,7 @@ def test_admin_add_user_yubikey_valid_length(config, journalist_app, test_admin, test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1776,6 +1947,7 @@ def test_admin_add_user_yubikey_correct_length_with_whitespace( test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user", l=locale), @@ -1808,6 +1980,7 @@ def test_admin_sets_user_to_admin(journalist_app, test_admin): test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user"), @@ -1820,6 +1993,7 @@ def test_admin_sets_user_to_admin(journalist_app, test_admin): is_admin=None, ), ) + print(resp.data.decode("utf-8")) assert resp.status_code in (200, 302) journo = Journalist.query.filter_by(username=new_user).one() @@ -1846,6 +2020,7 @@ def test_admin_renames_user(journalist_app, test_admin): test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user"), @@ -1883,6 +2058,7 @@ def test_admin_adds_first_name_last_name_to_user(journalist_app, test_admin): test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) resp = app.post( url_for("admin.add_user"), @@ -1921,6 +2097,7 @@ def test_admin_adds_invalid_first_last_name_to_user(config, journalist_app, test test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(client, "new", VALID_PASSWORD) resp = client.post( url_for("admin.add_user"), @@ -1974,6 +2151,7 @@ def test_admin_add_user_integrity_error(config, journalist_app, test_admin, mock test_admin["password"], test_admin["otp_secret"], ) + utils.prepare_password_change(app, "new", VALID_PASSWORD) with InstrumentedApp(journalist_app) as ins: resp = app.post( @@ -2514,6 +2692,7 @@ def test_incorrect_current_password_change(config, journalist_app, test_journo, test_journo["password"], test_journo["otp_secret"], ) + utils.prepare_password_change(app, test_journo["id"], VALID_PASSWORD) with InstrumentedApp(journalist_app) as ins: resp = app.post( url_for("account.new_password", l=locale), @@ -2629,6 +2808,7 @@ def test_too_long_user_password_change(config, journalist_app, test_journo, loca test_journo["password"], test_journo["otp_secret"], ) + utils.prepare_password_change(app, test_journo["id"], overly_long_password) with InstrumentedApp(journalist_app) as ins: resp = app.post( @@ -2650,6 +2830,7 @@ def test_too_long_user_password_change(config, journalist_app, test_journo, loca @flaky(rerun_filter=utils.flaky_filter_xfail) @pytest.mark.parametrize("locale", get_test_locales()) def test_valid_user_password_change(config, journalist_app, test_journo, locale): + """Test a successful password change flow""" with journalist_app.test_client() as app: login_journalist( app, @@ -2658,10 +2839,16 @@ def test_valid_user_password_change(config, journalist_app, test_journo, locale) test_journo["otp_secret"], ) + # First obtain a randomly generated password from the server + first = app.get(url_for("account.edit", l=locale)) + assert first.status_code == 200 + password = utils.extract_password(first.data) + + # Then send it back resp = app.post( url_for("account.new_password", l=locale), data=dict( - password=VALID_PASSWORD_2, + password=password, token=TOTP(test_journo["otp_secret"]).now(), current_password=test_journo["password"], ), @@ -2674,6 +2861,73 @@ def test_valid_user_password_change(config, journalist_app, test_journo, locale) ] with xfail_untranslated_messages(config, locale, msgids): assert escape(gettext(msgids[0])) in resp.data.decode("utf-8") + assert password in resp.data.decode() + + +@flaky(rerun_filter=utils.flaky_filter_xfail) +@pytest.mark.parametrize("locale", get_test_locales()) +def test_invalid_user_password_change_different(config, journalist_app, test_journo, locale): + """Test if the user submits a different password than the one they received""" + with journalist_app.test_client() as app: + login_journalist( + app, + test_journo["username"], + test_journo["password"], + test_journo["otp_secret"], + ) + + # First obtain a randomly generated password from the server + first = app.get(url_for("account.edit", l=locale)) + assert first.status_code == 200 + password = utils.extract_password(first.data) + + # No freak random collisions + assert password != VALID_PASSWORD + + # Then send back a different password + resp = app.post( + url_for("account.new_password", l=locale), + data=dict( + password=VALID_PASSWORD, + token=TOTP(test_journo["otp_secret"]).now(), + current_password=test_journo["password"], + ), + follow_redirects=True, + ) + + assert page_language(resp.data) == language_tag(locale) + msgids = ["The password you submitted is invalid. Password not changed."] + with xfail_untranslated_messages(config, locale, msgids): + assert escape(gettext(msgids[0])) in resp.data.decode("utf-8") + + +@flaky(rerun_filter=utils.flaky_filter_xfail) +@pytest.mark.parametrize("locale", get_test_locales()) +def test_invalid_user_password_change_nopending(config, journalist_app, test_journo, locale): + """Test if the user submits a password without receiving one from the server first""" + with journalist_app.test_client() as app: + login_journalist( + app, + test_journo["username"], + test_journo["password"], + test_journo["otp_secret"], + ) + + # We explicitly do not fetch a password from the server nor set a pending one + resp = app.post( + url_for("account.new_password", l=locale), + data=dict( + password=VALID_PASSWORD, + token=TOTP(test_journo["otp_secret"]).now(), + current_password=test_journo["password"], + ), + follow_redirects=True, + ) + + assert page_language(resp.data) == language_tag(locale) + msgids = ["The password you submitted is invalid. Password not changed."] + with xfail_untranslated_messages(config, locale, msgids): + assert escape(gettext(msgids[0])) in resp.data.decode("utf-8") @flaky(rerun_filter=utils.flaky_filter_xfail) diff --git a/securedrop/tests/test_journalist_session.py b/securedrop/tests/test_journalist_session.py index 147b85b910..49671a752e 100644 --- a/securedrop/tests/test_journalist_session.py +++ b/securedrop/tests/test_journalist_session.py @@ -5,7 +5,7 @@ from flask.sessions import session_json_serializer from itsdangerous import URLSafeTimedSerializer from redis import Redis -from tests.utils import login_journalist +from tests.utils import login_journalist, prepare_password_change from tests.utils.api_helper import get_api_headers from two_factor import TOTP @@ -178,6 +178,7 @@ def test_session_admin_change_password_logout(journalist_app, test_journo, test_ test_admin["otp_secret"], ) # When changing password of the journalist (non-admin) user + prepare_password_change(admin_app, test_journo["id"], NEW_PASSWORD) resp = admin_app.post( url_for("admin.new_password", user_id=test_journo["id"]), data=dict(password=NEW_PASSWORD), @@ -219,6 +220,7 @@ def test_session_change_password_logout(journalist_app, test_journo): assert (redis.get(journalist_app.config["SESSION_KEY_PREFIX"] + sid)) is not None # When sending a self change password request + prepare_password_change(app, test_journo["id"], NEW_PASSWORD) resp = app.post( url_for("account.new_password"), data=dict( diff --git a/securedrop/tests/utils/__init__.py b/securedrop/tests/utils/__init__.py index 70436d8102..22f86680f5 100644 --- a/securedrop/tests/utils/__init__.py +++ b/securedrop/tests/utils/__init__.py @@ -1,11 +1,14 @@ import datetime +import re from pathlib import Path +import argon2 import flask import models from db import db from encryption import EncryptionManager from flask.testing import FlaskClient +from models import ARGON2_PARAMS from source_user import SourceUser from tests.utils import asynchronous, db_helper # noqa: F401 from two_factor import TOTP @@ -68,6 +71,22 @@ def login_journalist( _reset_journalist_last_token(username) +def extract_password(html: bytes) -> str: + search = re.search(r'', html.decode()) + if search: + return search.group(1) + else: + raise ValueError("Could not find password in HTML") + + +def prepare_password_change(app_test_client: FlaskClient, id: int, new_password: str) -> None: + """A reimplementation of utils.set_pending_password() for tests""" + with app_test_client.session_transaction() as session: + session[f"pending_password_{id}"] = argon2.PasswordHasher(**ARGON2_PARAMS).hash( + new_password + ) + + def decrypt_as_journalist(ciphertext: bytes) -> bytes: return redwood.decrypt( ciphertext=ciphertext, From 8f3e387eba2e16247956bbd986dbb12a13187641 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Thu, 22 Aug 2024 16:19:12 -0400 Subject: [PATCH 05/10] Set SameSite="Strict" on all cookies for more CSRF protection See for an explanation of how SameSite="Strict" works. We just need to set the correct Flask config value and it automatically takes care of the rest. The journalist session code has some conditional code to support older versions of Flask that we no longer care about. Tests verify that `SameSite=Strict` is set on the session cookie in both the JI and SI. Note that the `http.cookie_jar` module doesn't support the SameSite option directly () so we need to use the `get_nonstandard_attr()` method to access the value. Refs . --- securedrop/journalist_app/__init__.py | 1 + securedrop/journalist_app/sessions.py | 7 ++----- securedrop/source_app/__init__.py | 1 + securedrop/tests/test_journalist_session.py | 17 +++-------------- securedrop/tests/test_source.py | 8 ++++++++ securedrop/tests/utils/__init__.py | 9 +++++++++ 6 files changed, 24 insertions(+), 19 deletions(-) diff --git a/securedrop/journalist_app/__init__.py b/securedrop/journalist_app/__init__.py index a64b3902e3..b0d0d898ac 100644 --- a/securedrop/journalist_app/__init__.py +++ b/securedrop/journalist_app/__init__.py @@ -50,6 +50,7 @@ def create_app(config: SecureDropConfig) -> Flask: Session(app, config) csrf = CSRFProtect(app) + app.config["SESSION_COOKIE_SAMESITE"] = "Strict" app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False app.config["SQLALCHEMY_DATABASE_URI"] = config.DATABASE_URI diff --git a/securedrop/journalist_app/sessions.py b/securedrop/journalist_app/sessions.py index 6fdcb19ef0..3d03d334bc 100644 --- a/securedrop/journalist_app/sessions.py +++ b/securedrop/journalist_app/sessions.py @@ -117,7 +117,6 @@ def __init__( self.api_salt = "api_" + salt self.header_name = header_name self.new = False - self.has_same_site_capability = hasattr(self, "get_cookie_samesite") def _new_session(self, is_api: bool = False, initial: Any = None) -> ServerSideSession: sid = self._generate_sid() @@ -192,11 +191,9 @@ def save_session( # type: ignore[override] session["renew_count"] -= 1 expires += self.lifetime session.modified = True - conditional_cookie_kwargs = {} httponly = self.get_cookie_httponly(app) secure = self.get_cookie_secure(app) - if self.has_same_site_capability: - conditional_cookie_kwargs["samesite"] = self.get_cookie_samesite(app) + samesite = self.get_cookie_samesite(app) val = self.serializer.dumps(dict(session)) if session.to_regenerate: self.redis.delete(self.key_prefix + session.sid) @@ -217,7 +214,7 @@ def save_session( # type: ignore[override] domain=domain, path=path, secure=secure, - **conditional_cookie_kwargs, # type: ignore + samesite=samesite, ) def logout_user(self, uid: int) -> None: diff --git a/securedrop/source_app/__init__.py b/securedrop/source_app/__init__.py index 79a5e772a7..49dee7adfa 100644 --- a/securedrop/source_app/__init__.py +++ b/securedrop/source_app/__init__.py @@ -57,6 +57,7 @@ def setup_i18n() -> None: # take longer than an hour over Tor, we increase the valid window to 24h. app.config["WTF_CSRF_TIME_LIMIT"] = 60 * 60 * 24 CSRFProtect(app) + app.config["SESSION_COOKIE_SAMESITE"] = "Strict" app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False app.config["SQLALCHEMY_DATABASE_URI"] = config.DATABASE_URI diff --git a/securedrop/tests/test_journalist_session.py b/securedrop/tests/test_journalist_session.py index 0d53b4c1d8..7f30faf89a 100644 --- a/securedrop/tests/test_journalist_session.py +++ b/securedrop/tests/test_journalist_session.py @@ -6,7 +6,7 @@ from flask.sessions import session_json_serializer from itsdangerous import URLSafeTimedSerializer from redis import Redis -from tests.utils import login_journalist, prepare_password_change +from tests.utils import _session_from_cookiejar, login_journalist, prepare_password_change from tests.utils.api_helper import get_api_headers from two_factor import TOTP @@ -41,19 +41,6 @@ def _get_session(sid, journalist_app, redis, api=False): return session_json_serializer.loads(redis.get(key)) -# Helper function to extract a the session cookie from the cookiejar when testing as client -# Returns the session cookie value -def _session_from_cookiejar(cookie_jar, journalist_app): - return next( - ( - cookie - for cookie in cookie_jar - if cookie.name == journalist_app.config["SESSION_COOKIE_NAME"] - ), - None, - ) - - # Test a standard login sequence def test_session_login(journalist_app, test_journo, redis): # Given a test client and a valid journalist user @@ -67,6 +54,8 @@ def test_session_login(journalist_app, test_journo, redis): session_cookie = _session_from_cookiejar(app.cookie_jar, journalist_app) # Then there is a session cookie in it assert session_cookie is not None + # Verify correct `SameSite` value was set on the cookie + assert session_cookie.get_nonstandard_attr("SameSite") == "Strict" # Then such cookie is properly signed sid = _check_sig(session_cookie.value, journalist_app) diff --git a/securedrop/tests/test_source.py b/securedrop/tests/test_source.py index 4c92918cba..c57c874639 100644 --- a/securedrop/tests/test_source.py +++ b/securedrop/tests/test_source.py @@ -124,6 +124,10 @@ def test_create_new_source(source_app): tab_id = next(iter(session["codenames"].keys())) resp = app.post(url_for("main.create"), data={"tab_id": tab_id}, follow_redirects=True) assert SessionManager.is_user_logged_in(db_session=db.session) + session_cookie = utils._session_from_cookiejar(app.cookie_jar, source_app) + # Verify correct `SameSite` value was set on the cookie + assert session_cookie.get_nonstandard_attr("SameSite") == "Strict" + # should be redirected to /lookup text = resp.data.decode("utf-8") assert "Submit Files" in text @@ -256,6 +260,10 @@ def test_login_and_logout(source_app): assert "Submit Files" in text assert SessionManager.is_user_logged_in(db_session=db.session) + session_cookie = utils._session_from_cookiejar(app.cookie_jar, source_app) + # Verify correct `SameSite` value was set on the cookie + assert session_cookie.get_nonstandard_attr("SameSite") == "Strict" + with source_app.test_client() as app: resp = app.post(url_for("main.login"), data=dict(codename="invalid"), follow_redirects=True) assert resp.status_code == 200 diff --git a/securedrop/tests/utils/__init__.py b/securedrop/tests/utils/__init__.py index 22f86680f5..774488297a 100644 --- a/securedrop/tests/utils/__init__.py +++ b/securedrop/tests/utils/__init__.py @@ -123,3 +123,12 @@ def create_legacy_gpg_key( db.session.add(source) db.session.commit() return result.fingerprint + + +# Helper function to extract a the session cookie from the cookiejar when testing as client +# Returns the session cookie value +def _session_from_cookiejar(cookie_jar, flask_app): + return next( + (cookie for cookie in cookie_jar if cookie.name == flask_app.config["SESSION_COOKIE_NAME"]), + None, + ) From 379e111aefdedde36efd227e25b34feac8b4f84b Mon Sep 17 00:00:00 2001 From: Cory Francis Myers Date: Thu, 15 Aug 2024 10:12:22 -0700 Subject: [PATCH 06/10] feat: style button.link like a hyperlink thread: https://github.com/freedomofpress/securedrop-server-security/pull/9#discussion_r1717619803 thread: https://github.com/freedomofpress/securedrop-server-security/pull/9#discussion_r1727661795 Co-authored-by: Kunal Mehta --- securedrop/journalist_templates/base.html | 2 +- securedrop/static/css/journalist.css | 14 ++++++++++++-- securedrop/translations/messages.pot | 6 +++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/securedrop/journalist_templates/base.html b/securedrop/journalist_templates/base.html index af8e1b8ad3..ef8e7834b0 100644 --- a/securedrop/journalist_templates/base.html +++ b/securedrop/journalist_templates/base.html @@ -27,7 +27,7 @@ {% endif %}
- +
{% endif %} diff --git a/securedrop/static/css/journalist.css b/securedrop/static/css/journalist.css index be1fa8e241..4b569cd0a5 100644 --- a/securedrop/static/css/journalist.css +++ b/securedrop/static/css/journalist.css @@ -977,14 +977,16 @@ button.icon[data-tooltip] > span.label:before { margin-left: 5px; } -a { +a, button.link { text-decoration: none; color: #0065db; + border: none; border-bottom: 1px solid rgba(0, 117, 247, 0.4); } -a:hover { +a:hover, button.link:hover { color: #2a319d; + border: none; border-bottom: 1px solid #2a319d; } @@ -5588,6 +5590,14 @@ footer img#footer-logo { font-style: italic; } +/* Adapted from . */ +button.link { + background-color: #fff; + letter-spacing: normal; + font: inherit; + padding: 0; +} + #replies .reply { border: 1px solid #9e9e9e; text-align: left; diff --git a/securedrop/translations/messages.pot b/securedrop/translations/messages.pot index 22da780b54..e2e2a2cbfa 100644 --- a/securedrop/translations/messages.pot +++ b/securedrop/translations/messages.pot @@ -434,9 +434,6 @@ msgstr "" msgid "Log Out" msgstr "" -msgid "LOG OUT" -msgstr "" - msgid "Return to All Sources" msgstr "" @@ -848,6 +845,9 @@ msgstr "" msgid "Please try again later. Check our website for more information." msgstr "" +msgid "LOG OUT" +msgstr "" + msgid "Server error" msgstr "" From 2e5466e7bd4ba705b2914d81b773c2593c51d13a Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Wed, 28 Aug 2024 15:34:12 -0400 Subject: [PATCH 07/10] Don't allow admins to look up arbitrary users' TOTP secrets via the web The `/admin/2fa` endpoint has an IDOR vulnerability in which changing the value of the `uid` parameter reveals that user's TOTP secret. The similar `/account/2fa` endpoint allows a user to see their own TOTP secret, which we'd like to avoid as well. Address that by requiring the TOTP secret to be checked to be passed as a form parameter. This allows to keep the existing workflow while removing the abilty to get arbitrary users' TOTP secrets. Practically the `/2fa` endpoint is split into `/verify-2fa-totp` and `/verify-2fa-hotp` endpoints that now each have their own template and function. Both contain documentation that explains what is provided by the user and what is looked up in the database. Tests needed an update because various endpoints no longer redirect. Note that no functional test changes were needed because there are no user-facing behavioral changes. This addresses "SEC-01-001 WP4: Arbitrary 2FA Enrollment via IDOR (medium)". Fixes . --- .../app-code/etc/apparmor.d/usr.sbin.apache2 | 6 +- securedrop/journalist_app/account.py | 101 ++++++++-- securedrop/journalist_app/admin.py | 124 ++++++++++--- .../account_new_two_factor_hotp.html | 14 ++ ....html => account_new_two_factor_totp.html} | 13 +- .../admin_new_user_two_factor_hotp.html | 15 ++ ...ml => admin_new_user_two_factor_totp.html} | 13 +- securedrop/models.py | 23 +-- securedrop/tests/test_journalist.py | 173 +++++++++--------- securedrop/tests/test_two_factor_in_apps.py | 27 ++- securedrop/translations/messages.pot | 32 ++-- securedrop/two_factor.py | 24 +++ 12 files changed, 370 insertions(+), 195 deletions(-) create mode 100644 securedrop/journalist_templates/account_new_two_factor_hotp.html rename securedrop/journalist_templates/{account_new_two_factor.html => account_new_two_factor_totp.html} (79%) create mode 100644 securedrop/journalist_templates/admin_new_user_two_factor_hotp.html rename securedrop/journalist_templates/{admin_new_user_two_factor.html => admin_new_user_two_factor_totp.html} (79%) diff --git a/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 b/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 index 119856197e..2a588c577a 100644 --- a/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 +++ b/securedrop/debian/app-code/etc/apparmor.d/usr.sbin.apache2 @@ -185,11 +185,13 @@ /var/www/securedrop/journalist_templates/_sources_confirmation_final_modal.html r, /var/www/securedrop/journalist_templates/_sources_confirmation_modal.html r, /var/www/securedrop/journalist_templates/account_edit_hotp_secret.html r, - /var/www/securedrop/journalist_templates/account_new_two_factor.html r, + /var/www/securedrop/journalist_templates/account_new_two_factor_totp.html r, + /var/www/securedrop/journalist_templates/account_new_two_factor_hotp.html r, /var/www/securedrop/journalist_templates/admin.html r, /var/www/securedrop/journalist_templates/admin_add_user.html r, /var/www/securedrop/journalist_templates/admin_edit_hotp_secret.html r, - /var/www/securedrop/journalist_templates/admin_new_user_two_factor.html r, + /var/www/securedrop/journalist_templates/admin_new_user_two_factor_totp.html r, + /var/www/securedrop/journalist_templates/admin_new_user_two_factor_hotp.html r, /var/www/securedrop/journalist_templates/base.html r, /var/www/securedrop/journalist_templates/col.html r, /var/www/securedrop/journalist_templates/config.html r, diff --git a/securedrop/journalist_app/account.py b/securedrop/journalist_app/account.py index a516c4c617..b8a0c3861a 100644 --- a/securedrop/journalist_app/account.py +++ b/securedrop/journalist_app/account.py @@ -1,5 +1,7 @@ +from datetime import datetime from typing import Union +import two_factor import werkzeug from db import db from flask import Blueprint, current_app, flash, g, redirect, render_template, request, url_for @@ -12,8 +14,8 @@ validate_hotp_secret, validate_user, ) +from markupsafe import Markup from passphrases import PassphraseGenerator -from two_factor import OtpTokenInvalid def make_blueprint() -> Blueprint: @@ -49,32 +51,93 @@ def new_password() -> werkzeug.Response: return redirect(url_for("main.login")) return redirect(url_for("account.edit")) - @view.route("/2fa", methods=("GET", "POST")) - def new_two_factor() -> Union[str, werkzeug.Response]: - if request.method == "POST": - token = request.form["token"] + @view.route("/verify-2fa-totp", methods=("POST",)) + def new_two_factor_totp() -> Union[str, werkzeug.Response]: + """ + After (re)setting a user's 2FA TOTP, allow them to verify the newly generated code. + + We don't want users to be able to see their TOTP secret after generation, so it must + be supplied in the form body, generated by another endpoint. The provided token is + then verified against the supplied secret. + """ + token = request.form["token"] + # NOTE: We only use the session for getting the user's name for the QR code + # and don't fetch any secrets from it. + username = session.get_user().username + otp_secret = request.form["otp_secret"] + totp = two_factor.TOTP(otp_secret) + try: + # Note: this intentionally doesn't prevent replay attacks, since we just want + # to make sure they have the right token + totp.verify(token, datetime.utcnow()) + flash( + gettext("Your two-factor credentials have been reset successfully."), + "notification", + ) + return redirect(url_for("account.edit")) + + except two_factor.OtpTokenInvalid: + flash( + gettext("There was a problem verifying the two-factor code. Please try again."), + "error", + ) + + return render_template( + "account_new_two_factor_totp.html", + qrcode=Markup(totp.qrcode_svg(username).decode()), + otp_secret=otp_secret, + formatted_otp_secret=two_factor.format_secret(otp_secret), + ) + + @view.route("/reset-2fa-totp", methods=["POST"]) + def reset_two_factor_totp() -> str: + session.get_user().is_totp = True + session.get_user().regenerate_totp_shared_secret() + db.session.commit() + new_otp_secret = session.get_user().otp_secret + return render_template( + "account_new_two_factor_totp.html", + qrcode=Markup(session.get_user().totp.qrcode_svg(session.get_user().username).decode()), + otp_secret=new_otp_secret, + formatted_otp_secret=two_factor.format_secret(new_otp_secret), + ) + + @view.route("/verify-2fa-hotp", methods=("POST",)) + def new_two_factor_hotp() -> Union[str, werkzeug.Response]: + """ + After (re)setting a user's 2FA HOTP, allow them to verify the newly generated code. + + This works differently than the analogous TOTP endpoint, as here we do verify against + the database secret because we need to compare with and increment the counter. + """ + user = session.get_user() + token = request.form["token"] + + error = False + + if not user.is_totp: try: - session.get_user().verify_2fa_token(token) + user.verify_2fa_token(token) flash( gettext("Your two-factor credentials have been reset successfully."), "notification", ) return redirect(url_for("account.edit")) - except OtpTokenInvalid: - flash( - gettext("There was a problem verifying the two-factor code. Please try again."), - "error", - ) + except two_factor.OtpTokenInvalid: + error = True + else: + # XXX: Consider using a different error message here, or do we not want to reveal + # if the user is using HOTP vs TOTP + error = True - return render_template("account_new_two_factor.html", user=session.get_user()) + if error: + flash( + gettext("There was a problem verifying the two-factor code. Please try again."), + "error", + ) - @view.route("/reset-2fa-totp", methods=["POST"]) - def reset_two_factor_totp() -> werkzeug.Response: - session.get_user().is_totp = True - session.get_user().regenerate_totp_shared_secret() - db.session.commit() - return redirect(url_for("account.new_two_factor")) + return render_template("account_new_two_factor_hotp.html", user=user) @view.route("/reset-2fa-hotp", methods=["POST"]) def reset_two_factor_hotp() -> Union[str, werkzeug.Response]: @@ -84,7 +147,7 @@ def reset_two_factor_hotp() -> Union[str, werkzeug.Response]: return render_template("account_edit_hotp_secret.html") session.get_user().set_hotp_secret(otp_secret) db.session.commit() - return redirect(url_for("account.new_two_factor")) + return render_template("account_new_two_factor_hotp.html", user=session.get_user()) else: return render_template("account_edit_hotp_secret.html") diff --git a/securedrop/journalist_app/admin.py b/securedrop/journalist_app/admin.py index 699c4dff5a..b823fb0cda 100644 --- a/securedrop/journalist_app/admin.py +++ b/securedrop/journalist_app/admin.py @@ -1,7 +1,9 @@ import binascii import os +from datetime import datetime from typing import Optional, Union +import two_factor import werkzeug from db import db from flask import ( @@ -26,6 +28,7 @@ validate_hotp_secret, verify_pending_password, ) +from markupsafe import Markup from models import ( FirstOrLastNameError, InstanceConfig, @@ -37,7 +40,6 @@ from passphrases import PassphraseGenerator from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound -from two_factor import OtpTokenInvalid def make_blueprint() -> Blueprint: @@ -217,8 +219,20 @@ def add_user() -> Union[str, werkzeug.Response]: current_app.logger.error("Adding user " "'{}' failed: {}".format(username, e)) if form_valid: - return redirect(url_for("admin.new_user_two_factor", uid=new_user.id)) + if new_user.is_totp: + return render_template( + "admin_new_user_two_factor_totp.html", + qrcode=Markup(new_user.totp.qrcode_svg(new_user.username).decode()), + otp_secret=new_user.otp_secret, + formatted_otp_secret=new_user.formatted_otp_secret, + userid=str(new_user.id), + ) + else: + return render_template( + "admin_new_user_two_factor_hotp.html", + user=new_user, + ) password = PassphraseGenerator.get_default().generate_passphrase( preferred_language=g.localeinfo.language ) @@ -226,13 +240,80 @@ def add_user() -> Union[str, werkzeug.Response]: set_pending_password("new", password) return render_template("admin_add_user.html", password=password, form=form) - @view.route("/2fa", methods=("GET", "POST")) + @view.route("/verify-2fa-totp", methods=("POST",)) @admin_required - def new_user_two_factor() -> Union[str, werkzeug.Response]: - user = Journalist.query.get(request.args["uid"]) + def new_user_two_factor_totp() -> Union[str, werkzeug.Response]: + """ + After (re)setting a user's 2FA TOTP, allow the admin to verify the newly generated code. + + We don't want admins to be able to look up arbitrary users' TOTP secrets, so it must + be supplied in the form body, generated by another endpoint. The provided token is + then verified against the supplied secret. + """ + token = request.form["token"] + # NOTE: This ID comes from the user and should be only used to look up the username + # for embedding in the QR code and success messages. We don't load any other state + # from the database to prevent IDOR attacks. + username = Journalist.query.get(request.form["userid"]).username + otp_secret = request.form["otp_secret"] + totp = two_factor.TOTP(otp_secret) + try: + # Note: this intentionally doesn't prevent replay attacks, since we just want + # to make sure they have the right token + totp.verify(token, datetime.utcnow()) + flash( + gettext( + 'The two-factor code for user "{user}" was verified ' "successfully." + ).format(user=username), + "notification", + ) + return redirect(url_for("admin.index")) - if request.method == "POST": - token = request.form["token"] + except two_factor.OtpTokenInvalid: + flash( + gettext("There was a problem verifying the two-factor code. Please try again."), + "error", + ) + + return render_template( + "admin_new_user_two_factor_totp.html", + qrcode=Markup(totp.qrcode_svg(username).decode()), + otp_secret=otp_secret, + formatted_otp_secret=two_factor.format_secret(otp_secret), + userid=request.form["userid"], + ) + + @view.route("/reset-2fa-totp", methods=["POST"]) + @admin_required + def reset_two_factor_totp() -> str: + uid = request.form["uid"] + user = Journalist.query.get(uid) + user.is_totp = True + user.regenerate_totp_shared_secret() + db.session.commit() + return render_template( + "admin_new_user_two_factor_totp.html", + qrcode=Markup(user.totp.qrcode_svg(user.username).decode()), + otp_secret=user.otp_secret, + formatted_otp_secret=user.formatted_otp_secret, + userid=str(user.id), + ) + + @view.route("/verify-2fa-hotp", methods=("POST",)) + @admin_required + def new_user_two_factor_hotp() -> Union[str, werkzeug.Response]: + """ + After (re)setting a user's 2FA HOTP, allow the admin to verify the newly generated code. + + This works differently than the analogous TOTP endpoint, as here we do verify against + the database secret because we need to compare with and increment the counter. + """ + user = Journalist.query.get(request.form["uid"]) + token = request.form["token"] + + error = False + + if not user.is_totp: try: user.verify_2fa_token(token) flash( @@ -243,23 +324,20 @@ def new_user_two_factor() -> Union[str, werkzeug.Response]: ) return redirect(url_for("admin.index")) - except OtpTokenInvalid: - flash( - gettext("There was a problem verifying the two-factor code. Please try again."), - "error", - ) + except two_factor.OtpTokenInvalid: + error = True + else: + # XXX: Consider using a different error message here, or do we not want to reveal + # if the user is using HOTP vs TOTP + error = True - return render_template("admin_new_user_two_factor.html", user=user) + if error: + flash( + gettext("There was a problem verifying the two-factor code. Please try again."), + "error", + ) - @view.route("/reset-2fa-totp", methods=["POST"]) - @admin_required - def reset_two_factor_totp() -> werkzeug.Response: - uid = request.form["uid"] - user = Journalist.query.get(uid) - user.is_totp = True - user.regenerate_totp_shared_secret() - db.session.commit() - return redirect(url_for("admin.new_user_two_factor", uid=user.id)) + return render_template("admin_new_user_two_factor_hotp.html", user=user) @view.route("/reset-2fa-hotp", methods=["POST"]) @admin_required @@ -271,7 +349,7 @@ def reset_two_factor_hotp() -> Union[str, werkzeug.Response]: if not validate_hotp_secret(user, otp_secret): return render_template("admin_edit_hotp_secret.html", uid=user.id) db.session.commit() - return redirect(url_for("admin.new_user_two_factor", uid=user.id)) + return render_template("admin_new_user_two_factor_hotp.html", user=user) else: return render_template("admin_edit_hotp_secret.html", uid=user.id) diff --git a/securedrop/journalist_templates/account_new_two_factor_hotp.html b/securedrop/journalist_templates/account_new_two_factor_hotp.html new file mode 100644 index 0000000000..c861215e73 --- /dev/null +++ b/securedrop/journalist_templates/account_new_two_factor_hotp.html @@ -0,0 +1,14 @@ +{% extends "base.html" %} + +{% block tab_title %}{{ gettext('Enable YubiKey (OATH-HOTP)') }}{% endblock %} + +{% block body %} +

{{ gettext('Enable YubiKey (OATH-HOTP)') }}

+

{{ gettext('Once you have configured your YubiKey, enter the 6-digit verification code below:') }}

+
+ + + + +
+{% endblock %} diff --git a/securedrop/journalist_templates/account_new_two_factor.html b/securedrop/journalist_templates/account_new_two_factor_totp.html similarity index 79% rename from securedrop/journalist_templates/account_new_two_factor.html rename to securedrop/journalist_templates/account_new_two_factor_totp.html index 508aedeb36..6e69023c69 100644 --- a/securedrop/journalist_templates/account_new_two_factor.html +++ b/securedrop/journalist_templates/account_new_two_factor_totp.html @@ -3,7 +3,6 @@ {% block tab_title %}{{ gettext('Enable FreeOTP') }}{% endblock %} {% block body %} -{% if user.is_totp %}

{{ gettext('Enable FreeOTP') }}

{{ gettext("You're almost done! To finish resetting your two-factor authentication, follow the instructions below to set up FreeOTP. Once you've added the entry for your account in the app, enter one of the 6-digit codes from the app to confirm that two-factor authentication is set up correctly.") }} @@ -16,18 +15,16 @@

{{ gettext('Enable FreeOTP') }}

  • {{ gettext('Your phone will now be in "scanning" mode. When you are in this mode, scan the barcode below:') }}
    • -
    {{ user.shared_secret_qrcode }}
    +
    {{ qrcode }}

    {{ gettext("Can't scan the barcode? You can manually pair FreeOTP with your SecureDrop account by entering the following two-factor secret into the app:") }}

    -{{ user.formatted_otp_secret }} +{{ formatted_otp_secret }} +

    {{ gettext("Once you have paired FreeOTP with this account, enter the 6-digit verification code below:") }}

    -{% else %} -

    {{ gettext('Enable YubiKey (OATH-HOTP)') }}

    -

    {{ gettext('Once you have configured your YubiKey, enter the 6-digit verification code below:') }}

    -{% endif %} -
    + + diff --git a/securedrop/journalist_templates/admin_new_user_two_factor_hotp.html b/securedrop/journalist_templates/admin_new_user_two_factor_hotp.html new file mode 100644 index 0000000000..179f187806 --- /dev/null +++ b/securedrop/journalist_templates/admin_new_user_two_factor_hotp.html @@ -0,0 +1,15 @@ +{% extends "base.html" %} + +{% block tab_title %}{{ gettext('Enable YubiKey (OATH-HOTP)') }}{% endblock %} + +{% block body %} +

    {{ gettext('Enable YubiKey (OATH-HOTP)') }}

    +

    {{ gettext('Once you have configured your YubiKey, enter the 6-digit code below:') }}

    + + + + + + +
    +{% endblock %} diff --git a/securedrop/journalist_templates/admin_new_user_two_factor.html b/securedrop/journalist_templates/admin_new_user_two_factor_totp.html similarity index 79% rename from securedrop/journalist_templates/admin_new_user_two_factor.html rename to securedrop/journalist_templates/admin_new_user_two_factor_totp.html index 14112cb3d1..03d1d11c8e 100644 --- a/securedrop/journalist_templates/admin_new_user_two_factor.html +++ b/securedrop/journalist_templates/admin_new_user_two_factor_totp.html @@ -3,7 +3,6 @@ {% block tab_title %}{{ gettext('Enable FreeOTP') }}{% endblock %} {% block body %} -{% if user.is_totp %}

    {{ gettext('Enable FreeOTP') }}

    {{ gettext("You're almost done! To finish adding this new user, have them follow the instructions below to set up two-factor authentication with FreeOTP. Once they've added an entry for this account in the app, have them enter one of the 6-digit codes from the app to confirm that two-factor authentication is set up correctly.") }} @@ -16,19 +15,17 @@

    {{ gettext('Enable FreeOTP') }}

  • {{ gettext('Your phone will now be in "scanning" mode. When you are in this mode, scan the barcode below:') }}
    • -
    {{ user.shared_secret_qrcode }}
    +
    {{ qrcode }}

    {{ gettext("Can't scan the barcode? You can manually pair FreeOTP with this account by entering the following two-factor secret into the app:") }}

    -{{ user.formatted_otp_secret }} +{{ formatted_otp_secret }}

    {{ gettext('Once you have paired FreeOTP with this account, enter the 6-digit verification code below:') }}

    -{% else %} -

    {{ gettext('Enable YubiKey (OATH-HOTP)') }}

    -

    {{ gettext('Once you have configured your YubiKey, enter the 6-digit code below:') }}

    -{% endif %} -
    + + + diff --git a/securedrop/models.py b/securedrop/models.py index fc66837ba8..a68f9bed1a 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -4,15 +4,12 @@ import os import uuid from hmac import compare_digest -from io import BytesIO from logging import Logger from typing import Any, Callable, Dict, List, Optional, Union import argon2 -import qrcode # Using svg because it doesn't require additional dependencies -import qrcode.image.svg import two_factor from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives.kdf import scrypt @@ -20,7 +17,6 @@ from encryption import EncryptionManager, GpgKeyNotFoundError from flask import url_for from flask_babel import gettext, ngettext -from markupsafe import Markup from passphrases import PassphraseGenerator from sqlalchemy import Boolean, Column, DateTime, ForeignKey, Integer, LargeBinary, String, Text from sqlalchemy.exc import IntegrityError @@ -614,26 +610,9 @@ def hotp(self) -> two_factor.HOTP: else: raise ValueError(f"{self} is not using HOTP") - @property - def shared_secret_qrcode(self) -> Markup: - uri = self.totp.get_provisioning_uri(self.username) - - qr = qrcode.QRCode(box_size=15, image_factory=qrcode.image.svg.SvgPathImage) - qr.add_data(uri) - img = qr.make_image() - - svg_out = BytesIO() - img.save(svg_out) - return Markup(svg_out.getvalue().decode("utf-8")) - @property def formatted_otp_secret(self) -> str: - """The OTP secret is easier to read and manually enter if it is all - lowercase and split into four groups of four characters. The secret is - base32-encoded, so it is case insensitive.""" - sec = self.otp_secret - chunks = [sec[i : i + 4] for i in range(0, len(sec), 4)] - return " ".join(chunks).lower() + return two_factor.format_secret(self.otp_secret) def verify_2fa_token(self, token: Optional[str]) -> str: if not token: diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index ef836a1f5f..ca72bea30c 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1275,21 +1275,22 @@ def test_admin_resets_user_hotp(journalist_app, test_admin, test_journo): old_secret = journo.otp_secret valid_secret = "DEADBEEF01234567DEADBEEF01234567DEADBEEF" - with InstrumentedApp(journalist_app) as ins: - resp = app.post( - url_for("admin.reset_two_factor_hotp"), - data=dict(uid=test_journo["id"], otp_secret=valid_secret), - ) - - # fetch altered DB object - journo = Journalist.query.get(journo.id) + resp = app.post( + url_for("admin.reset_two_factor_hotp"), + data=dict(uid=test_journo["id"], otp_secret=valid_secret), + ) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) - new_secret = journo.otp_secret - assert old_secret != new_secret - assert not journo.is_totp + # fetch altered DB object + journo = Journalist.query.get(journo.id) - # Redirect to admin 2FA view - ins.assert_redirects(resp, url_for("admin.new_user_two_factor", uid=journo.id)) + new_secret = journo.otp_secret + assert old_secret != new_secret + assert not journo.is_totp def test_admin_resets_user_hotp_error(mocker, journalist_app, test_admin, test_journo): @@ -1349,13 +1350,15 @@ def test_user_resets_hotp(journalist_app, test_journo): test_journo["otp_secret"], ) - with InstrumentedApp(journalist_app) as ins: - resp = app.post( - url_for("account.reset_two_factor_hotp"), - data=dict(otp_secret=new_secret), - ) - # should redirect to verification page - ins.assert_redirects(resp, url_for("account.new_two_factor")) + resp = app.post( + url_for("account.reset_two_factor_hotp"), + data=dict(otp_secret=new_secret), + ) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) # Re-fetch journalist to get fresh DB instance user = Journalist.query.get(test_journo["id"]) @@ -1444,11 +1447,12 @@ def test_admin_resets_user_totp(journalist_app, test_admin, test_journo): test_admin["otp_secret"], ) - with InstrumentedApp(journalist_app) as ins: - resp = app.post( - url_for("admin.reset_two_factor_totp"), data=dict(uid=test_journo["id"]) - ) - ins.assert_redirects(resp, url_for("admin.new_user_two_factor", uid=test_journo["id"])) + resp = app.post(url_for("admin.reset_two_factor_totp"), data=dict(uid=test_journo["id"])) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) # Re-fetch journalist to get fresh DB instance user = Journalist.query.get(test_journo["id"]) @@ -1468,10 +1472,12 @@ def test_user_resets_totp(journalist_app, test_journo): test_journo["otp_secret"], ) - with InstrumentedApp(journalist_app) as ins: - resp = app.post(url_for("account.reset_two_factor_totp")) - # should redirect to verification page - ins.assert_redirects(resp, url_for("account.new_two_factor")) + resp = app.post(url_for("account.reset_two_factor_totp")) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) # Re-fetch journalist to get fresh DB instance user = Journalist.query.get(test_journo["id"]) @@ -1511,34 +1517,16 @@ def test_admin_new_user_2fa_redirect(journalist_app, test_admin, test_journo): ) with InstrumentedApp(journalist_app) as ins: resp = app.post( - url_for("admin.new_user_two_factor", uid=test_journo["id"]), - data=dict(token=TOTP(test_journo["otp_secret"]).now()), + url_for("admin.new_user_two_factor_totp"), + data=dict( + token=TOTP(test_journo["otp_secret"]).now(), + otp_secret=test_journo["otp_secret"], + userid=test_journo["id"], + ), ) ins.assert_redirects(resp, url_for("admin.index")) -@flaky(rerun_filter=utils.flaky_filter_xfail) -@pytest.mark.parametrize("locale", get_test_locales()) -def test_http_get_on_admin_new_user_two_factor_page( - config, journalist_app, test_admin, test_journo, locale -): - with journalist_app.test_client() as app: - login_journalist( - app, - test_admin["username"], - test_admin["password"], - test_admin["otp_secret"], - ) - resp = app.get( - url_for("admin.new_user_two_factor", uid=test_journo["id"], l=locale), - ) - - assert page_language(resp.data) == language_tag(locale) - msgids = ["Enable FreeOTP"] - with xfail_untranslated_messages(config, locale, msgids): - assert gettext(msgids[0]) in resp.data.decode("utf-8") - - @flaky(rerun_filter=utils.flaky_filter_xfail) @pytest.mark.parametrize("locale", get_test_locales()) def test_http_get_on_admin_add_user_page(config, journalist_app, test_admin, locale): @@ -1573,21 +1561,22 @@ def test_admin_add_user(journalist_app, test_admin): assert first.status_code == 200 password = utils.extract_password(first.data) - with InstrumentedApp(journalist_app) as ins: - resp = app.post( - url_for("admin.add_user"), - data=dict( - username=username, - first_name="", - last_name="", - password=password, - otp_secret="", - is_admin=None, - ), - ) - - new_user = Journalist.query.filter_by(username=username).one() - ins.assert_redirects(resp, url_for("admin.new_user_two_factor", uid=new_user.id)) + resp = app.post( + url_for("admin.add_user"), + data=dict( + username=username, + first_name="", + last_name="", + password=password, + otp_secret="", + is_admin=None, + ), + ) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) @flaky(rerun_filter=utils.flaky_filter_xfail) @@ -2110,7 +2099,7 @@ def test_admin_adds_invalid_first_last_name_to_user(config, journalist_app, test is_admin=None, ), ) - assert resp.status_code == 302 + assert resp.status_code == 200 journo = Journalist.query.filter(Journalist.username == new_user).one() overly_long_name = "a" * (Journalist.MAX_NAME_LEN + 1) @@ -2631,7 +2620,8 @@ def test_admin_page_restriction_http_posts(journalist_app, test_journo): url_for("admin.reset_two_factor_totp"), url_for("admin.reset_two_factor_hotp"), url_for("admin.add_user", user_id=test_journo["id"]), - url_for("admin.new_user_two_factor"), + url_for("admin.new_user_two_factor_totp"), + url_for("admin.new_user_two_factor_hotp"), url_for("admin.reset_two_factor_totp"), url_for("admin.reset_two_factor_hotp"), url_for("admin.edit_user", user_id=test_journo["id"]), @@ -2671,7 +2661,8 @@ def test_user_authorization_for_posts(journalist_app): url_for("col.delete_single", filesystem_id="1"), url_for("main.reply"), url_for("main.bulk"), - url_for("account.new_two_factor"), + url_for("account.new_two_factor_totp"), + url_for("account.new_two_factor_hotp"), url_for("account.reset_two_factor_totp"), url_for("account.reset_two_factor_hotp"), url_for("account.change_name"), @@ -2993,16 +2984,17 @@ def test_regenerate_totp(journalist_app, test_journo): test_journo["otp_secret"], ) - with InstrumentedApp(journalist_app) as ins: - resp = app.post(url_for("account.reset_two_factor_totp")) - - new_secret = Journalist.query.get(test_journo["id"]).otp_secret + resp = app.post(url_for("account.reset_two_factor_totp")) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) - # check that totp is different - assert new_secret != old_secret + new_secret = Journalist.query.get(test_journo["id"]).otp_secret - # should redirect to verification page - ins.assert_redirects(resp, url_for("account.new_two_factor")) + # check that totp is different + assert new_secret != old_secret def test_edit_hotp(journalist_app, test_journo): @@ -3017,19 +3009,20 @@ def test_edit_hotp(journalist_app, test_journo): test_journo["otp_secret"], ) - with InstrumentedApp(journalist_app) as ins: - resp = app.post( - url_for("account.reset_two_factor_hotp"), - data=dict(otp_secret=valid_secret), - ) - - new_secret = Journalist.query.get(test_journo["id"]).otp_secret + resp = app.post( + url_for("account.reset_two_factor_hotp"), + data=dict(otp_secret=valid_secret), + ) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) - # check that totp is different - assert new_secret != old_secret + new_secret = Journalist.query.get(test_journo["id"]).otp_secret - # should redirect to verification page - ins.assert_redirects(resp, url_for("account.new_two_factor")) + # check that totp is different + assert new_secret != old_secret def test_delete_data_deletes_submissions_retaining_source( diff --git a/securedrop/tests/test_two_factor_in_apps.py b/securedrop/tests/test_two_factor_in_apps.py index 184ffe832b..c67bb36a24 100644 --- a/securedrop/tests/test_two_factor_in_apps.py +++ b/securedrop/tests/test_two_factor_in_apps.py @@ -129,12 +129,18 @@ def test_can_login_after_regenerating_hotp(self, journalist_app, test_journo): # Who goes to the page to reset their 2fa as HOTP otp_secret = "0123456789abcdef0123456789abcdef01234567" b32_otp_secret = b32encode(unhexlify(otp_secret)).decode("ascii") - with InstrumentedApp(journalist_app) as ins: - resp = app.post("/account/reset-2fa-hotp", data=dict(otp_secret=otp_secret)) - ins.assert_redirects(resp, "/account/2fa") + resp = app.post("/account/reset-2fa-hotp", data=dict(otp_secret=otp_secret)) + assert resp.status_code == 200 + assert ( + '' + in resp.data.decode() + ) + with InstrumentedApp(journalist_app) as ins: # And they successfully confirm the reset - app.post("/account/2fa", data=dict(token=HOTP(b32_otp_secret).generate(0))) + app.post( + "/account/verify-2fa-hotp", data=dict(token=HOTP(b32_otp_secret).generate(0)) + ) ins.assert_message_flashed( "Your two-factor credentials have been reset successfully.", "notification" ) @@ -174,8 +180,12 @@ def test_rejects_invalid_token_in_new_user_2fa_page(self, journalist_app, test_a invalid_token = "000000" with InstrumentedApp(journalist_app) as ins: resp = app.post( - url_for("admin.new_user_two_factor", uid=test_admin["id"]), - data=dict(token=invalid_token), + url_for("admin.new_user_two_factor_totp"), + data=dict( + token=invalid_token, + otp_secret=test_admin["otp_secret"], + userid=test_admin["id"], + ), ) # Then the corresponding error message is displayed @@ -199,7 +209,10 @@ def test_rejects_invalid_token_in_account_2fa_page(self, journalist_app, test_jo # When they try to submit an invalid token for setting up 2fa for themselves invalid_token = "000000" with InstrumentedApp(journalist_app) as ins: - resp = app.post(url_for("account.new_two_factor"), data=dict(token=invalid_token)) + resp = app.post( + url_for("account.new_two_factor_totp"), + data=dict(token=invalid_token, otp_secret=test_journo["otp_secret"]), + ) # Then the corresponding error message is displayed assert resp.status_code == 200 diff --git a/securedrop/translations/messages.pot b/securedrop/translations/messages.pot index e2e2a2cbfa..f4f73299ee 100644 --- a/securedrop/translations/messages.pot +++ b/securedrop/translations/messages.pot @@ -284,43 +284,43 @@ msgstr "" msgid "CONTINUE" msgstr "" -msgid "Enable FreeOTP" +msgid "Enable YubiKey (OATH-HOTP)" msgstr "" -msgid "You're almost done! To finish resetting your two-factor authentication, follow the instructions below to set up FreeOTP. Once you've added the entry for your account in the app, enter one of the 6-digit codes from the app to confirm that two-factor authentication is set up correctly." +msgid "Once you have configured your YubiKey, enter the 6-digit verification code below:" msgstr "" -msgid "Install FreeOTP on your phone" +msgid "Verification code" msgstr "" -msgid "Open the FreeOTP app" +msgid "Submit verification code" msgstr "" -msgid "Tap the QR code symbol at the top" +msgid "SUBMIT" msgstr "" -msgid "Your phone will now be in \"scanning\" mode. When you are in this mode, scan the barcode below:" +msgid "Enable FreeOTP" msgstr "" -msgid "Can't scan the barcode? You can manually pair FreeOTP with your SecureDrop account by entering the following two-factor secret into the app:" +msgid "You're almost done! To finish resetting your two-factor authentication, follow the instructions below to set up FreeOTP. Once you've added the entry for your account in the app, enter one of the 6-digit codes from the app to confirm that two-factor authentication is set up correctly." msgstr "" -msgid "Once you have paired FreeOTP with this account, enter the 6-digit verification code below:" +msgid "Install FreeOTP on your phone" msgstr "" -msgid "Enable YubiKey (OATH-HOTP)" +msgid "Open the FreeOTP app" msgstr "" -msgid "Once you have configured your YubiKey, enter the 6-digit verification code below:" +msgid "Tap the QR code symbol at the top" msgstr "" -msgid "Verification code" +msgid "Your phone will now be in \"scanning\" mode. When you are in this mode, scan the barcode below:" msgstr "" -msgid "Submit verification code" +msgid "Can't scan the barcode? You can manually pair FreeOTP with your SecureDrop account by entering the following two-factor secret into the app:" msgstr "" -msgid "SUBMIT" +msgid "Once you have paired FreeOTP with this account, enter the 6-digit verification code below:" msgstr "" msgid "Admin Interface" @@ -410,13 +410,13 @@ msgstr "" msgid "Enter a new HOTP secret formatted as a 40-digit hexadecimal string. Spaces will be ignored:" msgstr "" -msgid "You're almost done! To finish adding this new user, have them follow the instructions below to set up two-factor authentication with FreeOTP. Once they've added an entry for this account in the app, have them enter one of the 6-digit codes from the app to confirm that two-factor authentication is set up correctly." +msgid "Once you have configured your YubiKey, enter the 6-digit code below:" msgstr "" -msgid "Can't scan the barcode? You can manually pair FreeOTP with this account by entering the following two-factor secret into the app:" +msgid "You're almost done! To finish adding this new user, have them follow the instructions below to set up two-factor authentication with FreeOTP. Once they've added an entry for this account in the app, have them enter one of the 6-digit codes from the app to confirm that two-factor authentication is set up correctly." msgstr "" -msgid "Once you have configured your YubiKey, enter the 6-digit code below:" +msgid "Can't scan the barcode? You can manually pair FreeOTP with this account by entering the following two-factor secret into the app:" msgstr "" msgid "Navigation" diff --git a/securedrop/two_factor.py b/securedrop/two_factor.py index 5542c381e4..b9a915624d 100644 --- a/securedrop/two_factor.py +++ b/securedrop/two_factor.py @@ -2,8 +2,13 @@ import binascii import secrets from datetime import datetime +from io import BytesIO from typing import Optional +import qrcode + +# Using svg because it doesn't require additional dependencies +import qrcode.image.svg from cryptography.hazmat.primitives.hashes import SHA1 from cryptography.hazmat.primitives.twofactor import InvalidToken, hotp, totp @@ -16,6 +21,14 @@ def random_base32(length: int = 32) -> str: return "".join(secrets.choice(chars_to_use) for _ in range(length)) +def format_secret(sec: str) -> str: + """The OTP secret is easier to read and manually enter if it is all + lowercase and split into four groups of four characters. The secret is + base32-encoded, so it is case insensitive.""" + chunks = [sec[i : i + 4] for i in range(0, len(sec), 4)] + return " ".join(chunks).lower() + + class OtpSecretInvalid(Exception): """Raised when a user's OTP secret is invalid - for example, too short""" @@ -137,3 +150,14 @@ def verify(self, token: str, time: datetime) -> None: def get_provisioning_uri(self, account_name: str) -> str: return self._totp.get_provisioning_uri(account_name=account_name, issuer="SecureDrop") + + def qrcode_svg(self, account_name: str) -> bytes: + uri = self.get_provisioning_uri(account_name) + + qr = qrcode.QRCode(box_size=15, image_factory=qrcode.image.svg.SvgPathImage) + qr.add_data(uri) + img = qr.make_image() + + svg_out = BytesIO() + img.save(svg_out) + return svg_out.getvalue() From 0b0258296a24742b6d72d7411957a7a6a893e173 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Thu, 29 Aug 2024 14:44:44 -0700 Subject: [PATCH 08/10] SecureDrop 2.10.0~rc1 --- changelog.md | 29 +++++++++++++++++++++++++++++ securedrop/debian/changelog | 2 +- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/changelog.md b/changelog.md index 9cd18e4be2..e956cd5765 100644 --- a/changelog.md +++ b/changelog.md @@ -2,6 +2,35 @@ ## 2.10.0~rc1 +This release contains fixes for issues described in the most recent security audit by 7A Security, see +our [blog post](TK) for more details. It also contains other maintenance fixes. + +### Security + +* Don't allow admins to look up arbitrary users' TOTP secrets via the web (SEC-01-001 WP4) +* Validate user provided same password back to server (SEC-01-002 WP4) +* Require POST requests for `/logout` for CSRF protection (SEC-01-003 WP4) +* Set password for redis access (SEC-01-008 WP3) +* Set `SameSite=Strict` on all cookies for more CSRF protection + +### Web applications +* Dependency updates: + * sequoia-openpgp (Rust crate) from 1.20.0 to 1.21.1 (#7197) + * setuptools from 56.0.0 to 70.3.0 for CVE-2024-6345 (#7205, #7214) + * openssl (Rust crate) from 0.10.60 to 0.10.66 for RUSTSEC-2024-0357 (#7206) + +### Journalist Workstation +* Dependency updates: + * setuptools from 56.0.0 to 70.3.0 for CVE-2024-6345 (#7205, #7214) + * Remove d2to1 and pbr (#7205) + +### Development +* Don't point people to the decommissioned SecureDrop forum (#7204) +* Migrate all CI jobs to GitHub Actions (#7216, #7217, #7218, #7219, #7220, #7222, #7223) +* Improve staging job by using upstream gcloud-sdk image and enforcing GCE VM lifespan (#7215, #7224) +* Dependency updates: + * certifi from 2023.7.22 to 2024.7.4 for CVE-2024-39689 (#7199) + * Remove pytest-catchlog (#7199) ## 2.9.0 diff --git a/securedrop/debian/changelog b/securedrop/debian/changelog index 3898493b11..5ff30b2232 100644 --- a/securedrop/debian/changelog +++ b/securedrop/debian/changelog @@ -2,7 +2,7 @@ securedrop (2.10.0~rc1+focal) focal; urgency=medium * see changelog.md - -- SecureDrop Team Fri, 28 Jun 2024 11:37:37 -0400 + -- SecureDrop Team Thu, 29 Aug 2024 14:42:38 -0700 securedrop (2.9.0+focal) focal; urgency=medium From 911c6773f53872df6ac0fe120b91b6a64f81d95d Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Tue, 17 Sep 2024 16:06:19 -0400 Subject: [PATCH 09/10] SecureDrop 2.10.0 --- changelog.md | 4 ++-- install_files/ansible-base/group_vars/all/securedrop | 2 +- molecule/shared/stable.ver | 2 +- securedrop/debian/changelog | 6 ++++++ securedrop/setup.py | 2 +- securedrop/version.py | 2 +- 6 files changed, 12 insertions(+), 6 deletions(-) diff --git a/changelog.md b/changelog.md index e956cd5765..153554b385 100644 --- a/changelog.md +++ b/changelog.md @@ -1,9 +1,9 @@ # Changelog -## 2.10.0~rc1 +## 2.10.0 This release contains fixes for issues described in the most recent security audit by 7A Security, see -our [blog post](TK) for more details. It also contains other maintenance fixes. +our [blog post](https://securedrop.org/news/securedrop-2_10_0-released/) for more details. It also contains other maintenance fixes. ### Security diff --git a/install_files/ansible-base/group_vars/all/securedrop b/install_files/ansible-base/group_vars/all/securedrop index 446ef8d1fa..8a934480c5 100644 --- a/install_files/ansible-base/group_vars/all/securedrop +++ b/install_files/ansible-base/group_vars/all/securedrop @@ -2,7 +2,7 @@ # Variables that apply to both the app and monitor server go in this file # If the monitor or app server need different values define the variable in # hosts_vars/app.yml or host_vars/mon.yml -securedrop_version: "2.10.0~rc1" +securedrop_version: "2.10.0" securedrop_app_code_sdist_name: "securedrop-app-code-{{ securedrop_version | replace('~', '-') }}.tar.gz" grsecurity: true diff --git a/molecule/shared/stable.ver b/molecule/shared/stable.ver index c8e38b6140..10c2c0c3d6 100644 --- a/molecule/shared/stable.ver +++ b/molecule/shared/stable.ver @@ -1 +1 @@ -2.9.0 +2.10.0 diff --git a/securedrop/debian/changelog b/securedrop/debian/changelog index 5ff30b2232..e3914a7b07 100644 --- a/securedrop/debian/changelog +++ b/securedrop/debian/changelog @@ -1,3 +1,9 @@ +securedrop (2.10.0+focal) focal; urgency=medium + + * see changelog.md + + -- SecureDrop Team Tue, 17 Sep 2024 16:05:58 -0400 + securedrop (2.10.0~rc1+focal) focal; urgency=medium * see changelog.md diff --git a/securedrop/setup.py b/securedrop/setup.py index 21f855ff59..e48eaffa57 100644 --- a/securedrop/setup.py +++ b/securedrop/setup.py @@ -4,7 +4,7 @@ setuptools.setup( name="securedrop-app-code", - version="2.10.0~rc1", + version="2.10.0", author="Freedom of the Press Foundation", author_email="securedrop@freedom.press", description="SecureDrop Server", diff --git a/securedrop/version.py b/securedrop/version.py index 8a567eeeef..1c622223ba 100644 --- a/securedrop/version.py +++ b/securedrop/version.py @@ -1 +1 @@ -__version__ = "2.10.0~rc1" +__version__ = "2.10.0" From b46ee14f3d7315b835771f09ec2ef2b2a605c83f Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Wed, 18 Sep 2024 14:43:02 -0400 Subject: [PATCH 10/10] Ensure rq_config.py permissions are restored on next upgrade In a future 2.10.1 upgrade, the global chown over /var/www/securedrop would've blown away rq_config.py's root:www-data ownership, breaking read access for www-data. Add an exclusion, just like the existing one for config.py. --- securedrop/debian/securedrop-app-code.postinst | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/securedrop/debian/securedrop-app-code.postinst b/securedrop/debian/securedrop-app-code.postinst index 0858cb40ed..3660e7deea 100644 --- a/securedrop/debian/securedrop-app-code.postinst +++ b/securedrop/debian/securedrop-app-code.postinst @@ -251,12 +251,16 @@ case "$1" in chown -R root:root /var/www/securedrop chmod 755 /var/www/securedrop - # Make sure config.py is owned by root and readable by www-data, + # Make sure config.py and rq_config.py are owned by root and readable by www-data, # but not world-readable if [ -f "/var/www/securedrop/config.py" ]; then chown root:www-data /var/www/securedrop/config.py chmod 640 /var/www/securedrop/config.py fi + if [ -f "/var/www/securedrop/rq_config.py" ]; then + chown root:www-data /var/www/securedrop/rq_config.py + chmod 640 /var/www/securedrop/rq_config.py + fi # And logo needs to be writable by webserver user # If there's no custom logo yet, copy the default in its place if [ ! -f "/var/www/securedrop/static/i/custom_logo.png" ]; then