Merge pull request #13 from freedomofpress/idor-2fa

Don't allow admins to look up arbitrary users' TOTP secrets via the web

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,

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")
 

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,22 +219,101 @@ 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
         )
         # 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"))
+    @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)
 

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 %}
+<h1>{{ gettext('Enable YubiKey (OATH-HOTP)') }}</h1>
+<p>{{ gettext('Once you have configured your YubiKey, enter the 6-digit verification code below:') }}</p>
+<form id="check-token" method="post" action="{{url_for('account.new_two_factor_hotp')}}">
+  <input name="csrf_token" type="hidden" value="{{ csrf_token() }}">
+  <label for="token">{{ gettext('Verification code') }}</label>
+  <input name="token" id="token" type="text" placeholder="123456">
+  <button type="submit" aria-label="{{ gettext('Submit verification code') }}">{{ gettext('SUBMIT') }}</button>
+</form>
+{% endblock %}

securedrop/journalist_templates/account_new_two_factor.html → 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 %}
 <h1>{{ gettext('Enable FreeOTP') }}</h1>
 <p>
   {{ 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 @@ <h1>{{ gettext('Enable FreeOTP') }}</h1>
   <li>{{ gettext('Your phone will now be in "scanning" mode. When you are in this mode, scan the barcode below:') }}
   </li>
 </ol>
-<div id="qrcode-container">{{ user.shared_secret_qrcode }}</div>
+<div id="qrcode-container">{{ qrcode }}</div>
 <p>
   {{ 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:") }}
 </p>
-<mark id="shared-secret">{{ user.formatted_otp_secret }}</mark>
+<mark id="shared-secret">{{ formatted_otp_secret }}</mark>
+
 <p>{{ gettext("Once you have paired FreeOTP with this account, enter the 6-digit verification code below:") }}</p>
-{% else %}
-<h1>{{ gettext('Enable YubiKey (OATH-HOTP)') }}</h1>
-<p>{{ gettext('Once you have configured your YubiKey, enter the 6-digit verification code below:') }}</p>
-{% endif %}
-<form id="check-token" method="post">
+<form id="check-token" method="post" action="{{url_for('account.new_two_factor_totp')}}">
   <input name="csrf_token" type="hidden" value="{{ csrf_token() }}">
+  <input name="otp_secret" type="hidden" value="{{ otp_secret }}">
   <label for="token">{{ gettext('Verification code') }}</label>
   <input name="token" id="token" type="text" placeholder="123456">
   <button type="submit" aria-label="{{ gettext('Submit verification code') }}">{{ gettext('SUBMIT') }}</button>

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 %}
+<h1>{{ gettext('Enable YubiKey (OATH-HOTP)') }}</h1>
+<p>{{ gettext('Once you have configured your YubiKey, enter the 6-digit code below:') }}</p>
+<form id="check-token" method="post" action="{{url_for('admin.new_user_two_factor_hotp')}}">
+  <input name="csrf_token" type="hidden" value="{{ csrf_token() }}">
+  <input name="uid" type="hidden" value="{{ user.id }}">
+  <label for="token">{{ gettext('Verification code') }}</label>
+  <input name="token" id="token" type="text" placeholder="123456">
+  <button type="submit" aria-label="{{ gettext('Submit verification code') }}">{{ gettext('SUBMIT') }}</button>
+</form>
+{% endblock %}