Merge pull request #5958 from evilaliv3/fix-5933

Bump 2FA secret bit length from 80 to 160 bits as recommended by RFC4226

.circleci/config.yml

@@ -230,7 +230,7 @@ jobs:
 
       - run:
           name: Install libqt5designer5
-          command: sudo apt-get update && sudo apt-get install -y libqt5designer5
+          command: sudo apt-get update --allow-releaseinfo-change && sudo apt-get install -y libqt5designer5
 
       - run:
           name: Install requirements

securedrop/alembic/env.py

@@ -47,7 +47,7 @@ def run_migrations_offline():
     """
     url = config.get_main_option("sqlalchemy.url")
     context.configure(
-        url=url, target_metadata=target_metadata, literal_binds=True)
+        url=url, target_metadata=target_metadata, compare_type=True, literal_binds=True)
 
     with context.begin_transaction():
         context.run_migrations()
@@ -69,6 +69,7 @@ def run_migrations_online():
         context.configure(
             connection=connection,
             target_metadata=target_metadata,
+            compare_type=True,
             render_as_batch=True
         )
 

securedrop/alembic/versions/de00920916bf_updates_journalists_otp_secret_length_.py

@@ -0,0 +1,38 @@
+"""Updates journalists.otp_secret length from 16 to 32
+
+Revision ID: de00920916bf
+Revises: 1ddb81fb88c2
+Create Date: 2021-05-21 15:51:39.202368
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = 'de00920916bf'
+down_revision = '1ddb81fb88c2'
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    with op.batch_alter_table('journalists', schema=None) as batch_op:
+        batch_op.alter_column('otp_secret',
+               existing_type=sa.VARCHAR(length=16),
+               type_=sa.String(length=32),
+               existing_nullable=True)
+
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    with op.batch_alter_table('journalists', schema=None) as batch_op:
+        batch_op.alter_column('otp_secret',
+               existing_type=sa.String(length=32),
+               type_=sa.VARCHAR(length=16),
+               existing_nullable=True)
+
+    # ### end Alembic commands ###

securedrop/models.py

@@ -27,7 +27,7 @@
 
 from typing import Callable, Optional, Union, Dict, List, Any
 from logging import Logger
-from pyotp import OTP
+from pyotp import TOTP, HOTP
 
 
 LOGIN_HARDENING = True
@@ -404,7 +404,7 @@ class Journalist(db.Model):
     is_admin = Column(Boolean)  # type: Column[Optional[bool]]
     session_nonce = Column(Integer, nullable=False, default=0)
 
-    otp_secret = Column(String(16), default=pyotp.random_base32)
+    otp_secret = Column(String(32), default=pyotp.random_base32)
     is_totp = Column(Boolean, default=True)  # type: Column[Optional[bool]]
     hotp_counter = Column(Integer, default=0)  # type: Column[Optional[int]]
     last_token = Column(String(6))
@@ -556,6 +556,9 @@ def valid_password(self, passphrase: 'Optional[str]') -> bool:
                     "Should never happen: pw_salt is none for legacy Journalist {}".format(self.id)
                 )
 
+            # For type checking
+            assert isinstance(self.pw_hash, bytes)
+
             is_valid = pyotp.utils.compare_digest(
                 self._scrypt_hash(passphrase, self.pw_salt),
                 self.pw_hash)
@@ -584,14 +587,14 @@ def set_hotp_secret(self, otp_secret: str) -> None:
         self.hotp_counter = 0
 
     @property
-    def totp(self) -> 'OTP':
+    def totp(self) -> 'TOTP':
         if self.is_totp:
             return pyotp.TOTP(self.otp_secret)
         else:
             raise ValueError('{} is not using TOTP'.format(self))
 
     @property
-    def hotp(self) -> 'OTP':
+    def hotp(self) -> 'HOTP':
         if not self.is_totp:
             return pyotp.HOTP(self.otp_secret)
         else:
@@ -698,6 +701,8 @@ def login(cls,
 
             # Prevent TOTP token reuse
             if user.last_token is not None:
+                # For type checking
+                assert isinstance(token, str)
                 if pyotp.utils.compare_digest(token, user.last_token):
                     raise BadTokenException("previously used two-factor code "
                                             "{}".format(token))

securedrop/requirements/python3/securedrop-app-code-requirements.in

@@ -18,7 +18,7 @@ mod_wsgi
 passlib
 pretty-bad-protocol>=3.1.1
 psutil>=5.6.6
-pyotp
+pyotp>=2.6.0
 qrcode
 redis>=3.3.6
 rq>=1.1.0

securedrop/requirements/python3/securedrop-app-code-requirements.txt

@@ -211,9 +211,9 @@ psutil==5.7.0 \
 pycparser==2.18 \
     --hash=sha256:99a8ca03e29851d96616ad0404b4aad7d9ee16f25c9f9708a11faf2810f7b226
     # via cffi
-pyotp==2.2.6 \
-    --hash=sha256:8f0df1fcf9e86cec41f0a31c91212b1a04fca6dd353426917222b21864b9310b \
-    --hash=sha256:dd9130dd91a0340d89a0f06f887dbd76dd07fb95a8886dc4bc401239f2eebd69
+pyotp==2.6.0 \
+    --hash=sha256:9d144de0f8a601d6869abe1409f4a3f75f097c37b50a36a3bf165810a6e23f28 \
+    --hash=sha256:d28ddfd40e0c1b6a6b9da961c7d47a10261fb58f378cb00f05ce88b26df9c432
     # via -r requirements/python3/securedrop-app-code-requirements.in
 python-dateutil==2.7.2 \
     --hash=sha256:3220490fb9741e2342e1cf29a503394fdac874bc39568288717ee67047ff29df \

securedrop/tests/migrations/migration_de00920916bf.py

@@ -0,0 +1,73 @@
+# -*- coding: utf-8 -*-
+import random
+import uuid
+
+from sqlalchemy import text
+
+from db import db
+from journalist_app import create_app
+from .helpers import random_chars
+
+random.seed('くコ:彡')
+
+
+class Helper:
+
+    def __init__(self):
+        self.journalist_id = None
+
+    def create_journalist(self, otp_secret="ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"):
+        if self.journalist_id is not None:
+            raise RuntimeError('Journalist already created')
+
+        params = {
+            'uuid': str(uuid.uuid4()),
+            'username': random_chars(50),
+            'session_nonce': 0,
+            'otp_secret': otp_secret
+        }
+        sql = '''INSERT INTO journalists (uuid, username, otp_secret, session_nonce)
+                 VALUES (:uuid, :username, :otp_secret, :session_nonce)
+              '''
+        self.journalist_id = db.engine.execute(text(sql), **params).lastrowid
+
+
+class UpgradeTester(Helper):
+    """
+    Checks schema to verify that the otp_secret varchar "length" has been updated.
+    Varchar specified length isn't enforced by sqlite but it's good to verify that
+    the migration worked as expected.
+    """
+
+    def __init__(self, config):
+        Helper.__init__(self)
+        self.config = config
+        self.app = create_app(config)
+
+    def load_data(self):
+        with self.app.app_context():
+            self.create_journalist()
+
+    def check_upgrade(self):
+        with self.app.app_context():
+            journalists_sql = "SELECT * FROM journalists"
+            journalist = db.engine.execute(text(journalists_sql)).first()
+            assert len(journalist['otp_secret']) == 32  # Varchar ignores length
+
+
+class DowngradeTester(Helper):
+
+    def __init__(self, config):
+        Helper.__init__(self)
+        self.config = config
+        self.app = create_app(config)
+
+    def load_data(self):
+        with self.app.app_context():
+            self.create_journalist()
+
+    def check_downgrade(self):
+        with self.app.app_context():
+            journalists_sql = "SELECT * FROM journalists"
+            journalist = db.engine.execute(text(journalists_sql)).first()
+            assert len(journalist['otp_secret']) == 32  # Varchar ignores length