Add MFA protection to several pages of the PAUSE #455

charsbar · 2024-04-26T11:18:08Z

This is a replacement of wolfsage#5 . It should work, but we need some discussion before proceeding.

Where should be put uploaded files before we move them into incoming? (currently /tmp)
Messages in email/html
Cronjob to clean up temporary files
How many times should we allow users to enter an auth code?
Where should we add protection to?
Needs tests!
How to communicate with uploader CLIs
and probably more!

New configuration options are (for now):

INCOMING_TMP: where uploaded files from MFA-enabled users are temporary put
MFA_ISSUER: a name you'll see in your authenticator app

rjbs · 2024-04-26T16:04:55Z

doc/authen_pause.schema.txt

@@ -56,6 +56,9 @@ CREATE TABLE usertable (
  `changed` int(11) DEFAULT NULL,
  changedby char(10) DEFAULT NULL,
  lastvisit datetime DEFAULT NULL,
+  mfa tinyint(1) DEFAULT 0,
+  mfa_secret32 varchar(16) DEFAULT NULL,
+  mfa_recovery_codes text DEFAULT NULL,


I feel like I'm probably going to ask a lot of questions. I hope it's useful and not just annoying.

Does mfa mean "MFA is enabled"? If so, is there any value to having that and mfa_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.

It would be good to store mfa_last_verified_at with 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:

TOTP tokens change every 30s

we accept the TOTP token for now, 30s ago, and 30s from now

we store the time seed of any verified TOTP

we never accept a TOTP with a seed less than or equal to the last one

Otherwise, the token used at 12:00:00 has 89 seconds to be exfiltrated and used again.

Does mfa mean "MFA is enabled"? If so, is there any value to having that and mfa_secret32

Maybe we don't need mfa column anymore.

Removed mfa column with e1549a5

rjbs · 2024-04-26T16:07:20Z

lib/PAUSE.pm

@@ -100,6 +100,7 @@ $PAUSE::Config ||=
     HTTP_ERRORLOG => '/usr/local/apache/logs/error_log', # harmless use in cron-daily
     INCOMING => $IS_PAUSE_US ? 'ftp://localhost/incoming/' : 'ftp://pause.perl.org/incoming/',
     INCOMING_LOC => '/home/ftp/incoming/',
+     INCOMING_TMP => '/tmp/',


Why are we putting MFA-enabled-user uploads in a different place?

Isn't it confusing putting distributions that are not yet confirmed in the same /incoming/ directory? If not, I'm happy to put them under /incoming/ and remove some code.

I hadn't gotten far enough, when reading this (commit by commit) to understand that the process was "upload, then confirm". Having a second location makes sense. (Obviously, it'd be great to not have to confirm after upload.)

Obviously, it'd be great to not have to confirm after upload

If you pass a valid otp field value when you upload a file, the otp validation is done at the time of upload. However, the valid otp may change while uploading a large file. So we need to give users a chance to try a new otp without uploading the same file again.

rjbs · 2024-04-26T16:17:22Z

lib/pause_2017/PAUSE/Web/Plugin/WithMFAProtection.pm

+                            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);


Probably it's fine, but: probably we need to escape $dbh->errstr for entities?

As the PAUSE is now a Mojo app, error messages stored in the ERROR list should eventually be escaped at https://github.com/andk/pause/blob/master/lib/pause_2017/templates/layouts/layout.html.ep#L58 , but it's arguable if we should show this kind of raw error message to a user.

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.

rjbs · 2024-04-27T15:54:43Z

I think I more or less understand this, but I have some questions about what our longer-term plan is or should be. Rolling this out as a temporary security measure seems okay to me, but I think we'd get a lot more value out of real API tokens for uploading and for real sessions that can be re-authenticated. I know that's not going to happen in the next two days, but maybe we can discuss approving that as the next goal tomorrow.

What I'd actually love to see is something like a paragraph explaining (to the users who will have to use this) how it's meant to work. We'll need to show them something like this when it changes, and also it'd help me understand whether this is all going to work right!

…d users are put first

… stuff

…_LOC

wolfsage · 2024-04-28T09:46:38Z

lib/pause_2017/PAUSE/Web/Context.pm

+sub authenticator_for {
+  my ($self, $user) = @_;
+  my $cpan_alias = lc($user->{userid}) . '@cpan.org';
+  my $secret32   = $user->{mfa_secret32};


A comment here explaining that $user->{mfa_secret32} will be undef the first time this is set up, and that Auth::GoogleAuth will generate one for us, would be nice I think.

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?

wolfsage · 2024-04-28T10:04:46Z

lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm

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


This block of code says:

The user is updating their mfa (pause99_mfa_sub has a value)

Did they provided a 6 digit code in pause99_mfa_code? Does it verify? No? Fail

Did they provide a backup code in pause99_mfa_code? Does it verify? No? Fail

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

Inverted the logic with 684f2f0

wolfsage · 2024-04-28T10:11:20Z

lib/pause_2017/templates/user/mfa/edit.html.ep

@@ -0,0 +1,59 @@
+% layout 'layout';
+% my $pause = stash(".pause") || {};
+% my $cpan_alias = lc($pause->{HiddenUser}{userid}) . '@cpan.org';


I think $cpan_alias is unused in this file

Removed $cpan_alias with ca5ea28

wolfsage · 2024-04-28T10:16:37Z

lib/pause_2017/templates/user/mfa/edit.html.ep

+
+<div>
+<p>CODE: <%= text_field "pause99_mfa_code" => '',
+    size => 10,


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

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!

Fixed the size with efdf931

wolfsage · 2024-04-28T10:30:40Z

lib/pause_2017/PAUSE/Web/Context.pm

@@ -40,6 +41,17 @@ sub version {
  $version;
 }

+sub authenticator_for {
+  my ($self, $user) = @_;
+  my $cpan_alias = lc($user->{userid}) . '@cpan.org';


I don't think we should add the @cpan.org bit. I don't use a cpan email address and this confuses me. I would just have it be the PAUSE login name

Removed @cpan.org part with abb0141

wolfsage · 2024-04-28T10:40:41Z

lib/pause_2017/PAUSE/Web/Controller/User/Mfa.pm

+  my $u = $c->active_user_record;
+
+  my $auth = $c->app->pause->authenticator_for($u);
+  $pause->{mfa_qrcode} = $auth->qr_code;


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

Add _generate_qrcode with c2472f1

and modified parameters with 0d09dbd

wolfsage · 2024-04-28T10:47:42Z

I think there's a few real problems to fix, but otherwise this would be a good thing to do.

I have reservations about using Auth::GoogleAuth's qr_code image links.

I think we could relatively easily add a full set of tests to make sure the behaviour works as we expect so we don't have to hand test things every time. (You could inject a known totp secret into a test user, and either optionally provide a way (if tests don't already have it)) to fake the current time, or modify things so you can inject a timestamp into the totp stuff so we can have consistent tokens to test against)

…ery codes

charsbar marked this pull request as draft April 26, 2024 11:18

rjbs reviewed Apr 26, 2024

View reviewed changes

charsbar added 11 commits April 28, 2024 15:53

Add Auth::GoogleAuth to cpanfile

adc4cbf

Alter the authen_pause schema and add a one-off script to alter

78ffb03

Add INCOMING_TMP (/tmp/ for now) where incoming files from MFA-enable…

d7ed0e7

…d users are put first

Add WithMFAProtection plugin

356fed5

Add x_mfa_protection to places where apparently appropriate

1c6756b

Add authentication_for helper to hide the details of Auth::GoogleAuth…

db61391

… stuff

Move the incoming files put in the INCOMING_TMP directory to INCOMING…

c28a487

…_LOC

Add Mfa controller and a template for it

e6f7b1a

Add an email template for mfa

34f0879

Add a template for authentication code

a3c6f7a

Add x_csrf_and_mfa_protection helper

d53a7e4

charsbar force-pushed the mfa branch from 0f2e33b to d53a7e4 Compare April 28, 2024 06:56

wolfsage reviewed Apr 28, 2024

View reviewed changes

charsbar added 8 commits April 28, 2024 21:14

Invert the mfa verification logic (wolfsage++)

684f2f0

Remove an unused variable from mfa/edit.html.ep

ca5ea28

Fix the length of mfa_code field to match the current length of recov…

efdf931

…ery codes

PAUSE userid should be enough for the key_id passed to Auth::GoogleAuth

abb0141

Generate qrcode image by ourselves

c2472f1

Requires Imager::QRCode

a93739e

Modify qrcode parameters

0d09dbd

Remove mfa column and use mfa_secret32 instead

e1549a5

rjbs added the webui label Apr 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MFA protection to several pages of the PAUSE #455

Add MFA protection to several pages of the PAUSE #455

charsbar commented Apr 26, 2024 •

edited

Loading

rjbs Apr 26, 2024

rjbs Apr 26, 2024

charsbar Apr 26, 2024

charsbar Apr 28, 2024

rjbs Apr 26, 2024

charsbar Apr 26, 2024

rjbs Apr 27, 2024

charsbar Apr 27, 2024

rjbs Apr 26, 2024

charsbar Apr 26, 2024

ap Apr 27, 2024

rjbs commented Apr 27, 2024

wolfsage Apr 28, 2024

charsbar Apr 28, 2024

wolfsage Apr 28, 2024 •

edited

Loading

charsbar Apr 28, 2024

wolfsage Apr 28, 2024

charsbar Apr 28, 2024

wolfsage Apr 28, 2024

rjbs Apr 28, 2024

charsbar Apr 28, 2024

wolfsage Apr 28, 2024

charsbar Apr 28, 2024

wolfsage Apr 28, 2024

charsbar Apr 28, 2024

charsbar Apr 28, 2024

wolfsage commented Apr 28, 2024

Add MFA protection to several pages of the PAUSE #455

Are you sure you want to change the base?

Add MFA protection to several pages of the PAUSE #455

Conversation

charsbar commented Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjbs commented Apr 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfsage Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wolfsage commented Apr 28, 2024

charsbar commented Apr 26, 2024 •

edited

Loading

wolfsage Apr 28, 2024 •

edited

Loading