Add MFA protection to several pages of the PAUSE #455
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ use Email::MIME; | |
| use Data::Dumper; | ||
| use PAUSE::Web::Config; | ||
| use PAUSE::Web::Exception; | ||
| use Auth::GoogleAuth; | ||
|
|
||
| our $VERSION = "1072"; | ||
|
|
||
|
|
@@ -40,6 +41,17 @@ sub version { | |
| $version; | ||
| } | ||
|
|
||
| sub authenticator_for { | ||
| my ($self, $user) = @_; | ||
| my $cpan_alias = lc($user->{userid}) . '@cpan.org'; | ||
|
There was a problem hiding this comment. I don't think we should add the There was a problem hiding this comment. Removed |
||
| my $secret32 = $user->{mfa_secret32}; | ||
|
There was a problem hiding this comment. A comment here explaining that There was a problem hiding this comment. The generated secret32 at https://metacpan.org/dist/Auth-GoogleAuth/source/lib/Auth/GoogleAuth.pm#L21-25 might not be strong enough. Is it better for us to generate it by ourselves using Crypt::URandom instead of just adding a comment? |
||
| return Auth::GoogleAuth->new({ | ||
| secret32 => $secret32, | ||
| issuer => $PAUSE::Config->{MFA_ISSUER} || 'PAUSE', | ||
| key_id => $cpan_alias, | ||
| }); | ||
| } | ||
|
|
||
| sub hostname { | ||
| my $self = shift; | ||
| $PAUSE::Config->{SERVER_NAME} || Sys::Hostname::hostname(); | ||
|
|
||
| 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; | ||
|
There was a problem hiding this comment. 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: There was a problem hiding this comment. Add There was a problem hiding this comment. 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")) { | ||
|
There was a problem hiding this comment. This block of code says:
This means that you can supply an empty string in It would probably be better to invert the logic and start with There was a problem hiding this comment. 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; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| package PAUSE::Web::Plugin::WithMFAProtection; | ||
|
|
||
| use Mojo::Base 'Mojolicious::Plugin'; | ||
|
|
||
| our $VERSION = '1.00'; | ||
|
|
||
| sub register { | ||
| my ( $self, $app ) = @_; | ||
|
|
||
| my $routes = $app->routes; | ||
|
|
||
| $routes->add_condition( | ||
| with_mfa_protection => sub { | ||
| my ( $route, $c ) = @_; | ||
|
|
||
| my $u = $c->active_user_record; | ||
|
|
||
| # XXX: The active user record does not have mfa when an admin user is pretending someone else. | ||
| return 1 unless $u->{mfa}; | ||
|
|
||
| my $otp = $c->req->body_params->param('otp'); | ||
| if (defined $otp and $otp ne '') { | ||
| if ($otp =~ /\A[0-9]{6}\z/) { | ||
| return 1 if $c->app->pause->authenticator_for($u)->verify($otp); | ||
| } elsif ($otp =~ /\A[a-z0-9]{5}\-[a-z0-9]{5}\z/) { # maybe one of the recovery codes? | ||
| require PAUSE::Crypt; | ||
| my $pause = $c->stash(".pause"); | ||
| my @recovery_codes = split / /, $u->{mfa_recovery_codes} // ''; | ||
| for my $code (@recovery_codes) { | ||
| if (PAUSE::Crypt::password_verify($otp, $code)) { | ||
| my $new_codes = join ' ', grep { $_ ne $code } @recovery_codes; | ||
| my $dbh = $c->app->pause->authen_connect; | ||
| my $tbl = $PAUSE::Config->{AUTHEN_USER_TABLE}; | ||
| my $sql = "UPDATE $tbl SET mfa_recovery_codes = ?, changed = ?, changedby = ? WHERE user = ?"; | ||
| $dbh->do($sql, undef, $new_codes, 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); | ||
|
There was a problem hiding this comment. Probably it's fine, but: probably we need to escape There was a problem hiding this comment. As the PAUSE is now a Mojo app, error messages stored in the There was a problem hiding this comment. This sort of internal error is really none of the user’s business. Normal users won’t be able to act on it anyway, so it’s no use to them, but it might be of use to an attacker, whom it might help with crafting an injection if such a vulnerability is ever present somewhere. Showing the raw error to the user is fine if the application is running in some kind of dev/debug mode, but in production it should only be logged somewhere (ideally with some kind of a request ID (which maybe already exists – apologies for not knowing)) and shown to the user only as a generic 500 “sorry, something went wrong” error screen. I don’t know how the rest of the code handles errors though (apologies again), so this comment might be about the codebase in general and not actionable within this PR. |
||
| return 1; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| # special case for upload | ||
| if (my $upload = $c->req->upload("pause99_add_uri_httpupload")) { | ||
| if ($upload->size) { | ||
| $PAUSE::Config->{INCOMING_TMP} =~ s|/$||; | ||
|
|
||
| my $filename = $upload->filename; | ||
| $filename =~ s(.*/)()gs; # no slash | ||
| $filename =~ s(.*\\)()gs; # no backslash | ||
| $filename =~ s(.*:)()gs; # no colon | ||
| $filename =~ s/[^A-Za-z0-9_\-\.\@\+]//g; # only ASCII-\w and - . @ + allowed | ||
| my $to = "$PAUSE::Config->{INCOMING_TMP}/$filename"; | ||
| # my $fhi = $upl->fh; | ||
| if (-f $to && -s _ == 0) { # zero sized files are a common problem | ||
| unlink $to; | ||
| } | ||
| if (eval { $upload->move_to($to) }){ | ||
| warn "h1[File successfully copied to '$to']filename[$filename]"; | ||
| } else { | ||
| die PAUSE::Web::Exception->new(ERROR => "Couldn't copy file '$filename' to '$to': $!"); | ||
| } | ||
| unless ($upload->filename eq $filename) { | ||
| require Dumpvalue; | ||
| my $dv = Dumpvalue->new; | ||
| $c->req->param("pause99_add_uri_httpupload",$filename); | ||
| $c->req->param("pause99_add_uri_httpupload_renamed_from",$upload->filename); | ||
| } | ||
| $c->req->param("pause99_add_uri_httpupload_stashed", $filename); | ||
| } | ||
| } | ||
| $c->render('mfa_check'); | ||
| return; | ||
| } | ||
| ); | ||
|
|
||
| $routes->add_shortcut( | ||
| with_mfa_protection => sub { | ||
| my ($route) = @_; | ||
| return $route->requires( with_mfa_protection => 1 ); | ||
| } | ||
| ); | ||
|
|
||
| $routes->add_shortcut( | ||
| with_csrf_and_mfa_protection => sub { | ||
| my ($route) = @_; | ||
| return $route->requires( with_csrf_protection => 1, with_mfa_protection => 1 ); | ||
| } | ||
| ); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| 1; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like I'm probably going to ask a lot of questions. I hope it's useful and not just annoying.
mfamean "MFA is enabled"? If so, is there any value to having that andmfa_secret32? I would expect that you can't temporarily turn off MFA and turn it back on with the same secret, so no extra field is needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to store
mfa_last_verified_atwith the time window in which it was verified. If a OTP can be used more than once, it isn't a OTP! Better practice is something like:Otherwise, the token used at 12:00:00 has 89 seconds to be exfiltrated and used again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we don't need
mfacolumn anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed mfa column with e1549a5