Bug 2060356

Bugzilla/App/Controller/MFA/Duo.pm

@@ -63,8 +63,10 @@ sub callback {
 
   # Retrieve the event data from the mfa token
   my $provider = Bugzilla::MFA->new_from($user, 'Duo');
-  my $event
-    = $provider->verify_token($mfa_cookie, {no_redirect => 1, no_delete => 1});
+  # provider_callback skips the duo_verified gate: we are the request that is
+  # about to establish it.
+  my $event = $provider->verify_token($mfa_cookie,
+    {no_redirect => 1, no_delete => 1, provider_callback => 1});
   if (!$event) {
     return $self->code_error('duo_client_error',
       {reason => ERR_INVALID_MFA_COOKIE});

Bugzilla/Install/DB.pm

@@ -847,6 +847,9 @@ sub update_table_definitions {
   # Bug 1806896 - xavier.lhour@gmail.com
   _migrate_flag_state_activity();
 
+  # Bug 2060356 - dkl@mozilla.com
+  _remove_duo_recovery_codes();
+
   ################################################################
   # New --TABLE-- changes should go *** A B O V E *** this point #
   ################################################################
@@ -4538,6 +4541,19 @@ sub _migrate_flag_state_activity {
   $dbh->bz_drop_table('flag_state_activity');
 }
 
+sub _remove_duo_recovery_codes {
+  my $dbh = Bugzilla->dbh;
+
+  # Duo users were able to generate recovery codes but never had a form in
+  # which to enter one, so these rows are unusable secrets. Recovery for Duo
+  # is handled by Duo Security itself.
+  $dbh->do(
+    "DELETE FROM profile_mfa
+      WHERE name LIKE 'recovery.%'
+            AND user_id IN (SELECT userid FROM profiles WHERE mfa = 'Duo')"
+  );
+}
+
 1;
 
 __END__

Bugzilla/MFA.pm

@@ -54,6 +54,11 @@ sub prompt { }
 # throws errors if code is invalid
 sub check { }
 
+# throws errors if the event does not carry proof of a successful verification.
+# only meaningful for providers which verify out-of-band (ie. can_verify_inline
+# is false), where the proof is recorded on the event by a separate request.
+sub verify_event { }
+
 # if true verification can happen inline (during enrollment/pref changes)
 # if false then the mfa provider requires an intermediate verification page
 sub can_verify_inline {0}
@@ -86,6 +91,12 @@ sub verify_token {
   # return event data
   my $event = get_token_extra_data($token);
 
+  # Verification performed out-of-band (Duo) records its result on the event
+  # rather than throwing from check().  The provider's own callback runs this
+  # before that result exists and passes provider_callback to opt out; every
+  # other caller must be gated here.
+  $self->verify_event($event) if $event && !$options->{provider_callback};
+
   unless ($options->{no_delete}) {
     delete_token($token);
 

Bugzilla/MFA/Duo.pm

@@ -20,6 +20,25 @@ sub can_verify_inline {
   return 0;
 }
 
+# Duo verification happens in a separate request handled by
+# Bugzilla::App::Controller::MFA::Duo, which sets duo_verified on the event
+# once the authorization code has been exchanged.  Without it the mfa token
+# only proves the prompt was issued, not that the user passed Duo.
+sub verify_event {
+  my ($self, $event) = @_;
+  return if $event->{duo_verified};
+  ThrowUserError('duo_user_error', {reason => 'Invalid Duo Security MFA Code'});
+}
+
+# Duo users have no way to enter a recovery code -- there is no Duo
+# verification form, prompt() redirects straight to Duo.  Recovery is handled
+# by Duo itself (bypass codes, self-service device management).
+sub generate_recovery_codes {
+  my ($self) = @_;
+  ThrowUserError('duo_user_error',
+    {reason => 'Recovery codes are not available when using Duo Security.'});
+}
+
 sub enroll {
   my ($self, $params) = @_;
 

t/mfa-duo-verify.t

@@ -0,0 +1,104 @@
+#!/usr/bin/env perl
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+use 5.10.1;
+use strict;
+use warnings;
+use lib qw( . lib local/lib/perl5 );
+
+use Bugzilla::Test::MockDB;
+use Bugzilla::Test::MockLocalconfig urlbase => 'http://bmo.test/';
+use Bugzilla::Test::MockParams (duo_uri => 'http://duo.test/');
+use Bugzilla::Test::Util qw(create_user);
+
+use Bugzilla;
+use Bugzilla::Constants;
+use Bugzilla::MFA;
+use Bugzilla::Token qw(issue_short_lived_session_token);
+use JSON::MaybeXS qw(encode_json);
+use Test::More;
+use Try::Tiny;
+
+BEGIN { Bugzilla->extensions }
+
+Bugzilla->usage_mode(USAGE_MODE_TEST);
+Bugzilla->error_mode(ERROR_MODE_DIE);
+Bugzilla->input_params({});
+
+my $user = create_user('duo-user@mozilla.test', '*');
+Bugzilla->set_user($user);
+Bugzilla->dbh->do('UPDATE profiles SET mfa = ? WHERE userid = ?',
+  undef, 'Duo', $user->id);
+
+# Mint an mfa session token carrying $event, the way verify_prompt does.
+# set_token_extra_data is not used directly because its upsert is MySQL-only.
+sub mfa_token {
+  my ($event) = @_;
+  my $token = issue_short_lived_session_token('mfa', $user);
+  Bugzilla->dbh->do('INSERT INTO token_data (token, extra_data) VALUES (?, ?)',
+    undef, $token, encode_json($event));
+  return $token;
+}
+
+sub dies_like {
+  my ($code, $re, $name) = @_;
+  my $err;
+  try { $code->() } catch { $err = $_ };
+  like($err // '(did not die)', $re, $name);
+}
+
+my $event = {
+  reason   => 'creating an API key',
+  actions  => [{type => 'create', description => 'test key'}],
+  postback => {action => 'userprefs.cgi', fields => {tab => 'apikey'}},
+};
+
+my $provider = Bugzilla::MFA->new_from($user, 'Duo');
+isa_ok($provider, 'Bugzilla::MFA::Duo');
+
+# The core of bug 2060356: a session-cookie attacker can trigger verify_prompt
+# and recover the mfa token from the Set-Cookie header without ever completing
+# Duo. Replaying it must not yield a verified event.
+dies_like(
+  sub { $provider->verify_token(mfa_token($event), {no_delete => 1}) },
+  qr/Invalid Duo Security MFA Code/,
+  'verify_token rejects an event that never passed Duo'
+);
+
+# Duo's own callback runs before duo_verified exists, so it opts out.
+{
+  my $got = $provider->verify_token(mfa_token($event),
+    {no_delete => 1, no_redirect => 1, provider_callback => 1});
+  is($got->{reason}, 'creating an API key',
+    'provider_callback bypasses the gate for the Duo callback itself');
+}
+
+# The happy path: the callback has recorded a successful code exchange.
+{
+  my $verified = {%$event, duo_verified => 1};
+  my $got      = $provider->verify_token(mfa_token($verified));
+  is($got->{actions}[0]{type}, 'create', 'verify_token accepts a verified event');
+}
+
+# A recovery code must not stand in for Duo verification. Duo users have no
+# form to enter one, and generating them is now blocked outright.
+dies_like(
+  sub { $provider->generate_recovery_codes() },
+  qr/Recovery codes are not available/,
+  'Duo refuses to generate recovery codes'
+);
+
+# Providers that verify inline are unaffected: the base verify_event is a no-op.
+{
+  my $dummy = Bugzilla::MFA->new_from($user, 'Dummy');
+  isa_ok($dummy, 'Bugzilla::MFA::Dummy');
+  my $got = $dummy->verify_token(mfa_token($event), {no_delete => 1});
+  is($got->{reason}, 'creating an API key',
+    'non-Duo providers are not gated on duo_verified');
+}
+
+done_testing;

template/en/default/account/prefs/mfa.html.tmpl

@@ -87,10 +87,14 @@
           [% INCLUDE "mfa/protected.html.tmpl" %]
         </div>
       [% END %]
-      <div>
-        <button type="button" id="mfa-recovery">Generate Printable Recovery Codes</button>
-        [% INCLUDE "mfa/protected.html.tmpl" %]
-      </div>
+      [%# Duo has no verification form, so a recovery code could never be
+        # entered. Recovery is handled by Duo Security itself. %]
+      [% IF user.mfa != 'Duo' %]
+        <div>
+          <button type="button" id="mfa-recovery">Generate Printable Recovery Codes</button>
+          [% INCLUDE "mfa/protected.html.tmpl" %]
+        </div>
+      [% END %]
     </div>
 
     <p class="mfa-api-blurb">

token.cgi

@@ -513,11 +513,6 @@ sub mfa_event_from_token {
   # verify
   my $event = $user->mfa_provider->verify_token($token);
 
-  # If we got this far and MFA is Duo, we should be verified
-  if ($user->mfa eq 'Duo' && !$event->{duo_verified}) {
-    ThrowUserError('duo_user_error', {reason => 'Invalid Duo Security MFA Code'});
-  }
-
   return ($user, $event);
 }
 

userprefs.cgi

@@ -736,7 +736,12 @@ sub SaveMFA {
     ThrowUserError('password_incorrect');
   }
 
-  my $mfa = $cgi->param('mfa') // $user->mfa;
+  # The provider performs the verification, so it has to be the user's real
+  # one.  The request parameter is only meaningful while enrolling -- the
+  # prefs UI emits it solely in the not-yet-enrolled branch -- and honouring
+  # it afterwards would let the caller name a provider whose checks are
+  # no-ops (anything but TOTP/Duo falls through to MFA::Dummy).
+  my $mfa = $user->mfa || $cgi->param('mfa');
   my $provider = Bugzilla::MFA->new_from($user, $mfa) // return;
 
   my $reason;
@@ -745,6 +750,10 @@ sub SaveMFA {
     $reason = 'Two-factor enrollment';
   }
   elsif ($action eq 'recovery') {
+    if ($mfa eq 'Duo') {
+      ThrowUserError('duo_user_error',
+        {reason => 'Recovery codes are not available when using Duo Security.'});
+    }
     $reason = 'Recovery code generation';
   }
   elsif ($action eq 'disable') {
@@ -811,15 +820,10 @@ sub SaveMFAupdate {
 sub SaveMFAcallback {
   my $mfa_token = shift;
   my $user      = Bugzilla->user;
-  my $mfa       = Bugzilla->cgi->param('mfa');
+  my $mfa       = $user->mfa || Bugzilla->cgi->param('mfa');
   my $provider  = Bugzilla::MFA->new_from($user, $mfa) // return;
   my $event     = $provider->verify_token($mfa_token);
 
-  # Must have passed the Duo verification to proceed to update
-  if ($mfa eq 'Duo' && !$event->{duo_verified}) {
-    ThrowUserError('duo_user_error', {reason => 'Invalid Duo Security MFA Code'});
-  }
-
   SaveMFAupdate($event->{action}, $mfa);
 }