Enforce 160-bit HOTP secret length and verify OTP secret length on login.

securedrop/journalist_app/admin.py

@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 
 import os
+import binascii
 from typing import Optional
 from typing import Union
 
@@ -132,6 +133,17 @@ def add_user() -> Union[str, werkzeug.Response]:
                       'There was an error with the autogenerated password. '
                       'User not created. Please try again.'), 'error')
                 form_valid = False
+            except (binascii.Error, TypeError) as e:
+                if "Non-hexadecimal digit found" in str(e):
+                    flash(gettext(
+                        "Invalid HOTP secret format: "
+                        "please only submit letters A-F and numbers 0-9."),
+                          "error")
+                else:
+                    flash(gettext(
+                        "An unexpected error occurred! "
+                        "Please inform your admin."), "error")
+                form_valid = False
             except InvalidUsernameException as e:
                 form_valid = False
                 # Translators: Here, "{message}" explains the problem with the username.

securedrop/journalist_app/api.py

@@ -19,7 +19,8 @@
 from journalist_app import utils
 from models import (Journalist, Reply, SeenReply, Source, Submission,
                     LoginThrottledException, InvalidUsernameException,
-                    BadTokenException, WrongPasswordException)
+                    BadTokenException, InvalidOTPSecretException,
+                    WrongPasswordException)
 from sdconfig import SDConfig
 from store import NotEncrypted
 
@@ -133,7 +134,7 @@ def get_token() -> Tuple[flask.Response, int]:
 
             return response, 200
         except (LoginThrottledException, InvalidUsernameException,
-                BadTokenException, WrongPasswordException):
+                BadTokenException, InvalidOTPSecretException, WrongPasswordException):
             return abort(403, 'Token authentication failed.')
 
     @api.route('/sources', methods=['GET'])

securedrop/journalist_app/forms.py

@@ -6,15 +6,41 @@
 from wtforms import Field
 from wtforms import (TextAreaField, StringField, BooleanField, HiddenField,
                      ValidationError)
-from wtforms.validators import InputRequired, Optional
+from wtforms.validators import InputRequired, Optional, DataRequired, StopValidation
 
-from models import Journalist, InstanceConfig
+from models import Journalist, InstanceConfig, HOTP_SECRET_LENGTH
+
+from typing import Any
+
+
+class RequiredIf(DataRequired):
+
+    def __init__(self, other_field_name: str, *args: Any, **kwargs: Any) -> None:
+        self.other_field_name = other_field_name
+
+    def __call__(self, form: FlaskForm, field: Field) -> None:
+        if self.other_field_name in form:
+            other_field = form[self.other_field_name]
+            if bool(other_field.data):
+                self.message = gettext(
+                    'The "{name}" field is required when "{other_name}" is set.'
+                    .format(other_name=self.other_field_name, name=field.name))
+                super(RequiredIf, self).__call__(form, field)
+            else:
+                field.errors[:] = []
+                raise StopValidation()
+        else:
+            raise ValidationError(
+                gettext(
+                    'The "{other_name}" field was not found - it is required by "{name}".'
+                    .format(other_name=self.other_field_name, name=field.name))
+                )
 
 
 def otp_secret_validation(form: FlaskForm, field: Field) -> None:
     strip_whitespace = field.data.replace(' ', '')
     input_length = len(strip_whitespace)
-    if input_length != 40:
+    if input_length != HOTP_SECRET_LENGTH:
         raise ValidationError(
             ngettext(
                 'HOTP secrets are 40 characters long - you have entered {num}.',
@@ -74,8 +100,8 @@ class NewUserForm(FlaskForm):
     is_admin = BooleanField('is_admin')
     is_hotp = BooleanField('is_hotp')
     otp_secret = StringField('otp_secret', validators=[
-        otp_secret_validation,
-        Optional()
+        RequiredIf("is_hotp"),
+        otp_secret_validation
     ])
 
 

securedrop/journalist_app/utils.py

@@ -16,6 +16,7 @@
     BadTokenException,
     FirstOrLastNameError,
     InvalidPasswordLength,
+    InvalidOTPSecretException,
     InvalidUsernameException,
     Journalist,
     LoginThrottledException,
@@ -30,6 +31,7 @@
     Submission,
     WrongPasswordException,
     get_one_or_else,
+    HOTP_SECRET_LENGTH,
 )
 from store import add_checksum_for_file
 
@@ -93,6 +95,7 @@ def validate_user(
     try:
         return Journalist.login(username, password, token)
     except (InvalidUsernameException,
+            InvalidOTPSecretException,
             BadTokenException,
             WrongPasswordException,
             LoginThrottledException,
@@ -111,6 +114,11 @@ def validate_user(
                 "Please wait at least {num} seconds before logging in again.",
                 period
             ).format(num=period)
+        elif isinstance(e, InvalidOTPSecretException):
+            login_flashed_msg += ' '
+            login_flashed_msg += gettext(
+                "Your 2FA details are invalid"
+                " - please contact an administrator to reset them.")
         else:
             try:
                 user = Journalist.query.filter_by(
@@ -134,21 +142,26 @@ def validate_hotp_secret(user: Journalist, otp_secret: str) -> bool:
     :param otp_secret: the new HOTP secret
     :return: True if it validates, False if it does not
     """
+    strip_whitespace = otp_secret.replace(' ', '')
+    secret_length = len(strip_whitespace)
+
+    if secret_length != HOTP_SECRET_LENGTH:
+        flash(ngettext(
+                'HOTP secrets are 40 characters long - you have entered {num}.',
+                'HOTP secrets are 40 characters long - you have entered {num}.',
+                secret_length
+            ).format(num=secret_length), "error")
+        return False
+
     try:
         user.set_hotp_secret(otp_secret)
     except (binascii.Error, TypeError) as e:
         if "Non-hexadecimal digit found" in str(e):
             flash(gettext(
-                "Invalid secret format: "
+                "Invalid HOTP secret format: "
                 "please only submit letters A-F and numbers 0-9."),
                   "error")
             return False
-        elif "Odd-length string" in str(e):
-            flash(gettext(
-                "Invalid secret format: "
-                "odd-length secret. Did you mistype the secret?"),
-                  "error")
-            return False
         else:
             flash(gettext(
                 "An unexpected error occurred! "

securedrop/journalist_templates/admin_edit_hotp_secret.html

@@ -1,10 +1,11 @@
 {% extends "base.html" %}
 {% block body %}
+<h1>{{ gettext('Change HOTP Secret')}}</h1>
+<p>{{ gettext("Enter a new HOTP secret formatted as a 40-digit hexadecimal string. Spaces will be ignored:") }}</p>
 <form method="post">
   <input name="csrf_token" type="hidden" value="{{ csrf_token() }}">
   <input type="hidden" name="uid" value="{{ uid }}">
   <p>
-    <label for="otp_secret">{{ gettext('Change Secret') }}</label>
     <input name="otp_secret" type="text" placeholder="{{ gettext('HOTP Secret') }}"><br>
   </p>
   <button type="submit">{{ gettext('CONTINUE') }}</button>

securedrop/models.py

@@ -36,6 +36,13 @@
 
 ARGON2_PARAMS = dict(memory_cost=2**16, rounds=4, parallelism=2)
 
+# Required length for hex-format HOTP secrets as input by users
+HOTP_SECRET_LENGTH = 40  # 160 bits == 40 hex digits (== 32 ascii-encoded chars in db)
+
+# Minimum length for ascii-encoded OTP secrets - by default, secrets are now 160-bit (32 chars)
+# but existing Journalist users may still have 80-bit (16-char) secrets
+OTP_SECRET_MIN_ASCII_LENGTH = 16  # 80 bits == 40 hex digits (== 16 ascii-encoded chars in db)
+
 
 def get_one_or_else(query: Query,
                     logger: 'Logger',
@@ -364,6 +371,11 @@ class BadTokenException(Exception):
     """Raised when a user logins in with an incorrect TOTP token"""
 
 
+class InvalidOTPSecretException(Exception):
+
+    """Raised when a user's OTP secret is invalid - for example, too short"""
+
+
 class PasswordError(Exception):
 
     """Generic error for passwords that are invalid.
@@ -696,6 +708,9 @@ def login(cls,
                 user.uuid in Journalist.INVALID_USERNAMES:
             raise InvalidUsernameException(gettext("Invalid username"))
 
+        if len(user.otp_secret) < OTP_SECRET_MIN_ASCII_LENGTH:
+            raise InvalidOTPSecretException(gettext("Invalid OTP secret"))
+
         if LOGIN_HARDENING:
             cls.throttle_login(user)
 

securedrop/tests/test_integration.py

@@ -639,7 +639,7 @@ def test_user_change_password(journalist_app, test_journo):
 def test_login_after_regenerate_hotp(journalist_app, test_journo):
     """Test that journalists can login after resetting their HOTP 2fa"""
 
-    otp_secret = 'aaaaaa'
+    otp_secret = '0123456789abcdef0123456789abcdef01234567'
     b32_otp_secret = b32encode(unhexlify(otp_secret))
 
     # edit hotp