diff --git a/Bugzilla/App/Controller/MFA/Duo.pm b/Bugzilla/App/Controller/MFA/Duo.pm index 8dad382a77..997d8228ce 100644 --- a/Bugzilla/App/Controller/MFA/Duo.pm +++ b/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}); diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index 503626e4d7..5147beaa07 100644 --- a/Bugzilla/Install/DB.pm +++ b/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__ diff --git a/Bugzilla/MFA.pm b/Bugzilla/MFA.pm index 9f08c1b484..177c3ceb13 100644 --- a/Bugzilla/MFA.pm +++ b/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); diff --git a/Bugzilla/MFA/Duo.pm b/Bugzilla/MFA/Duo.pm index ffd2b905b4..4af0418a47 100644 --- a/Bugzilla/MFA/Duo.pm +++ b/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) = @_; diff --git a/t/mfa-duo-verify.t b/t/mfa-duo-verify.t new file mode 100644 index 0000000000..2c4196305a --- /dev/null +++ b/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; diff --git a/template/en/default/account/prefs/mfa.html.tmpl b/template/en/default/account/prefs/mfa.html.tmpl index e003bf2454..3a11022d7e 100644 --- a/template/en/default/account/prefs/mfa.html.tmpl +++ b/template/en/default/account/prefs/mfa.html.tmpl @@ -87,10 +87,14 @@ [% INCLUDE "mfa/protected.html.tmpl" %] [% END %] -
- - [% INCLUDE "mfa/protected.html.tmpl" %] -
+ [%# Duo has no verification form, so a recovery code could never be + # entered. Recovery is handled by Duo Security itself. %] + [% IF user.mfa != 'Duo' %] +
+ + [% INCLUDE "mfa/protected.html.tmpl" %] +
+ [% END %]

diff --git a/token.cgi b/token.cgi index 35c2a48316..25ca510bc0 100755 --- a/token.cgi +++ b/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); } diff --git a/userprefs.cgi b/userprefs.cgi index d3028c6703..a660df65bb 100755 --- a/userprefs.cgi +++ b/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); }