Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MFA protection to several pages of the PAUSE #455

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add Mfa controller and a template for it
  • Loading branch information
charsbar committed Apr 28, 2024
commit e6f7b1a049eca3dd32b688e7df7cb9a8890414b4
80 changes: 80 additions & 0 deletions lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package PAUSE::Web::Controller::User::Mfa;

use Mojo::Base "Mojolicious::Controller";
use Auth::GoogleAuth;
use PAUSE::Crypt;
use Crypt::URandom qw(urandom);
use Convert::Base32 qw(encode_base32);

sub edit {
my $c = shift;
my $pause = $c->stash(".pause");
my $mgr = $c->app->pause;
my $req = $c->req;
my $u = $c->active_user_record;

my $auth = $c->app->pause->authenticator_for($u);
$pause->{mfa_qrcode} = $auth->qr_code;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. I don't love this module. qr_code gives a link to quickchart.io which means the url containing the secrets to set up the 2fa are in someone else's web logs. I don't think we should do this.

I think we should construct our own images and inline them, something like:

  my $uri = "otpauth://totp/$label?secret=$key&issuer=$org.$domain";

  my $img = plot_qrcode($uri, {
    size          => 4,
    margin        => 4,
    version       => 1,
    level         => 'M',
    casesensitive => 1,
  });

  $img->write(data => \my $qr_png, type => "png")
    or die "Failed to write image: " . $img->errstr;

  my $data = URI->new("data:");
  $data->data($qr_png);
  $data->media_type('image/png');

  # And now in the html  <img src="$data" />

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add _generate_qrcode with c2472f1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and modified parameters with 0d09dbd

if (!$u->{mfa_secret32}) {
my $dbh = $mgr->authen_connect;
my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE};
my $sql = "UPDATE $tbl SET mfa_secret32 = ?, changed = ?, changedby = ? WHERE user = ?";
$dbh->do($sql, undef, $auth->secret32, time, $pause->{User}{userid}, $u->{userid})
or push @{$pause->{ERROR}}, sprintf(qq{Could not enter the data into the database: <i>%s</i>.},$dbh->errstr);
}

if (uc $req->method eq 'POST' and $req->param("pause99_mfa_sub")) {
Copy link
Collaborator

@wolfsage wolfsage Apr 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block of code says:

  1. The user is updating their mfa (pause99_mfa_sub has a value)
  2. Did they provided a 6 digit code in pause99_mfa_code? Does it verify? No? Fail
  3. Did they provide a backup code in pause99_mfa_code? Does it verify? No? Fail
  4. Success. Update/disable the mfa

This means that you can supply an empty string in pause99_mfa_code, or any string that doesn't match the two regexps, and successfully disable mfa

It would probably be better to invert the logic and start with my $success = 0; and then only set $success to true under very explicit circumstances. Then, we only take action if $success is true. This can help avoid these kind of logic bugs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inverted the logic with 684f2f0

my $code = $req->param("pause99_mfa_code");
$req->param("pause99_mfa_code", undef);
if ($code =~ /\A[0-9]{6}\z/ && !$auth->verify($code)) {
$pause->{error}{invalid_code} = 1;
return;
} elsif ($code =~ /\A[a-z0-9]{5}\-[a-z0-9]{5}\z/ && $u->{mfa_recovery_codes} && $req->param("pause99_mfa_reset")) {
my @recovery_codes = split / /, $u->{mfa_recovery_codes} // '';
if (!grep { PAUSE::Crypt::password_verify($code, $_) } @recovery_codes) {
$pause->{error}{invalid_code} = 1;
return;
}
}
my ($mfa, $secret32, $recovery_codes);
if ($req->param("pause99_mfa_reset")) {
$mfa = 0;
$secret32 = undef;
$recovery_codes = undef;
$c->flash(mfa_disabled => 1);
} else {
$mfa = 1;
$secret32 = $auth->secret32;
$c->flash(mfa_enabled => 1);
my @codes = _generate_recovery_codes();
$c->flash(recovery_codes => \@codes);
$recovery_codes = join " ", map { PAUSE::Crypt::hash_password($_) } @codes;
}
my $dbh = $mgr->authen_connect;
my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE};
my $sql = "UPDATE $tbl SET mfa = ?, mfa_secret32 = ?, mfa_recovery_codes = ?, changed = ?, changedby = ? WHERE user = ?";
if ($dbh->do($sql, undef, $mfa, $secret32, $recovery_codes, time, $pause->{User}{userid}, $u->{userid})) {
my $mailblurb = $c->render_to_string("email/user/mfa/edit", format => "email");
my $header = {Subject => "User update for $u->{userid}"};
my @to = $u->{secretemail};
$mgr->send_mail_multi(\@to, $header, $mailblurb);
} else {
push @{$pause->{ERROR}}, sprintf(qq{Could not enter the data
into the database: <i>%s</i>.},$dbh->errstr);
}
$c->redirect_to('/authenquery?ACTION=mfa');
}
}

sub _generate_recovery_codes {
my @codes;
for (1 .. 8) {
my $code = encode_base32(urandom(6));
$code =~ tr/lo/89/;
$code =~ s/^(.{5})/$1-/;
push @codes, $code;
}
@codes;
}

1;
59 changes: 59 additions & 0 deletions lib/pause_2017/templates/user/mfa/edit.html.ep
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
% layout 'layout';
% my $pause = stash(".pause") || {};
% my $cpan_alias = lc($pause->{HiddenUser}{userid}) . '@cpan.org';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think $cpan_alias is unused in this file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed $cpan_alias with ca5ea28


<input type="hidden" name="HIDDENNAME" value="<%= $pause->{HiddenUser}{userid} %>">

% if (flash('mfa_enabled')) {
<div class="messagebox info">
<p><b>Multifactor Authentication is enabled.</b></p>
<p>Recovery codes:</p>
<code>
<ul>
% for my $code (@{ flash('recovery_codes') }) {
<li><%= $code %>
% }
</ul>
</code>
<p>Please write down these codes, as they will not show again.</p>
</div>
% } elsif (flash('mfa_disabled')) {
<div class="messagebox info">
<p>Multifactor Authentication is disabled.</p>
</div>
% }

<h3><% if (!$pause->{HiddenUser}{mfa}) { %>Enable<% } else { %>Disable<% } %> Multifactor Authentication for <%= $pause->{HiddenUser}{userid} %>
% if (exists $pause->{UserGroups}{admin}) {
(lastvisit <%= $pause->{HiddenUser}{lastvisit} || "before 2005-12-02" %>)
% }
</h3>

% if (my $error = $pause->{error}) {
<div class="messagebox error">
<b>ERROR</b>:
% if ($error->{invalid_code}) {
Verification Code is invalid.
% }
</div>
<hr>
% }
% if (!$pause->{HiddenUser}{mfa}) {
<div>
<p>Submit 6-digit code to enable Multifactor Authentication.</p>
<img src="<%= $pause->{mfa_qrcode} %>">
</div>
% } else {
<p>Submit 6-digit code to disable Multifactor Authentication.</p>
<%= hidden_field "pause99_mfa_reset" => 1 %>
% }

<div>
<p>CODE: <%= text_field "pause99_mfa_code" => '',
size => 10,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 1 short for recovery codes which means you can't use them to disable your mfa like sub edit intends in Mfa.pm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that true?

        my $code = encode_base32(urandom(6));
        $code =~ tr/lo/89/;
        $code =~ s/^(.{5})/$1-/;
        push @codes, $code;

So, 6 bytes b32 encoded is 10 bytes. Then l and o become 8 and 9, still 10 bytes. Then we put a - in the middle. Okay, you're right, thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the size with efdf931

maxlength => 10,
%>
<input type="submit" name="pause99_mfa_sub" value="Submit"></p>
</div>

%= csrf_field