extend 2fa protection beyond login

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
General
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

(Blocks: 2 bugs)

Production
Dependency tree / graph

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
2fa protection needs to also apply to:
- password changing
- api-key generation
- disabling the api_key_only preference
(Assignee)

Updated

2 years ago
Depends on: 1199088
(Assignee)

Comment 1

2 years ago
+ password resetting ('forgot password' workflow)

Comment 2

2 years ago
I guess this would go toward the end of the "forgot password" workflow, so attackers can't use it to find out whether an email address has an account.
(Assignee)

Comment 3

2 years ago
(In reply to Jesse Ruderman from comment #2)
> I guess this would go toward the end of the "forgot password" workflow, so
> attackers can't use it to find out whether an email address has an account.

correct - it'll happen at the time you need to set a new password.  it wouldn't be required to request a forgot-password email.

Comment 4

2 years ago
(In reply to Jesse Ruderman from comment #2)
> I guess this would go toward the end of the "forgot password" workflow, so
> attackers can't use it to find out whether an email address has an account.

Though anyone can create an account on BMO and then query https://bugzilla.mozilla.org/user_profile?login=foo@bar.com or use the CC list auto-complete feature to achieve the same.
(Assignee)

Updated

2 years ago
Blocks: 1201983
(Assignee)

Updated

2 years ago
Blocks: 1202393
(Assignee)

Updated

2 years ago
No longer depends on: 1199088
(Assignee)

Updated

2 years ago
Blocks: 1202886
(Assignee)

Updated

2 years ago
Blocks: 1202281
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Updated

2 years ago
Blocks: 1203008
(Assignee)

Comment 5

2 years ago
Created attachment 8658583 [details] [diff] [review]
1199087_1.patch

- extends 2fa protection to:
  - setting a new password via prefs
  - setting a new email address via prefs
  - setting a new password via password-reset
  - adding a new api key
  - re-enabling a revoked api key
  - switching api_key_only pref to off
- updates login verification to use new flow, and
  - honour 'Restrict this session to this IP address' setting (bug 1201983)
  - honour 'remember login' setting (bug 1202281)
  - return you to the page that requested login (bug 1202886)
- show an icon next to actions that require 2fa
- adds enrolment specific code-failed message (bug 1202393)
- adds token attached data
  - carries state across 2ga request
- adds short-lived tokens
  - these tokens have a max lifetime of 1 hour instead of 3 days
Attachment #8658583 - Flags: review?(dylan)
Attachment #8658583 - Flags: feedback?(dkl)
Comment on attachment 8658583 [details] [diff] [review]
1199087_1.patch

Review of attachment 8658583 [details] [diff] [review]:
-----------------------------------------------------------------

Partial review, will keep working at it. No show-stoppers yet, but see notes.

::: Bugzilla/Constants.pm
@@ +470,5 @@
>  
>  # The maximum number of days a token will remain valid.
>  use constant MAX_TOKEN_AGE => 3;
> +# The maximum number of hours a short-lived token will remain valid.
> +use constant MAX_SHORT_TOKEN_AGE => 1;

So this is hours and the next constant is days. Perhaps MAX_SHORT_TOKEN_HOURS?

::: Bugzilla/Token.pm
@@ +490,5 @@
>      return 1;
>  }
>  
> +sub set_token_extra_data {
> +    my ($token, $data) = @_;

We should check the length of $data here. I'd rather know early if the encoded json is too big than have a hard-to-debug error later.

@@ +499,5 @@
> +
> +sub get_token_extra_data {
> +    my ($token) = @_;
> +    trick_taint($token);
> +    my $data = scalar Bugzilla->dbh->selectrow_array(

This is surprising, I would expect $data to be an integer 1 here.
my ($data) = ... may be more idiomatic.

::: js/account.js
@@ +143,5 @@
> +    $('#apikey-toggle-revoked')
> +        .click(function(event) {
> +            event.preventDefault();
> +            $('.apikey_revoked.bz_tui_hidden').removeClass('bz_tui_hidden');
> +            var $this = $(this);

$this is not used?
Also, I like either $this or that, but not both in the same file.


also, I totally missed the console.log call that's in this file. Let's remove that with this patch set. :)
Blocks: 1196743
Comment on attachment 8658583 [details] [diff] [review]
1199087_1.patch

Review of attachment 8658583 [details] [diff] [review]:
-----------------------------------------------------------------

r- for minor inconvenience

::: template/en/default/mfa/totp/verify.html.tmpl
@@ +17,4 @@
>    Please enter your verification code from your TOTP application:
>  </p>
>  
> +<form method="POST" action="[% postback.action FILTER none %]">

nit: elements inside the form element arn't indented more.
Attachment #8658583 - Flags: review?(dylan) → review-
(Assignee)

Comment 8

2 years ago
Created attachment 8661396 [details] [diff] [review]
1199087_2.patch

- MAX_SHORT_TOKEN_AGE --> MAX_SHORT_TOKEN_HOURS
- move json encoding/decoding into set_token_extra_data/get_token_extra_data
- add check for json length
- other minor changes
Attachment #8658583 - Attachment is obsolete: true
Attachment #8658583 - Flags: feedback?(dkl)
Attachment #8661396 - Flags: review?(dylan)

Comment 9

2 years ago
Dylan, do you know when you will be able to review this? Bug 1202281 is dependant on this one - and I've just had to disable 2FA on my account since I've reached the point where I cannot face having to sign in (and unlock phone, manually type 2fa token etc) multiple times a day. Thanks :-)
Ah mcote has just let me know you're in TRIBE this week - makes sense :-)
(Assignee)

Updated

2 years ago
Blocks: 1205680
(Assignee)

Updated

2 years ago
Blocks: 1199090
Comment on attachment 8661396 [details] [diff] [review]
1199087_2.patch

Review of attachment 8661396 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan

nice fix, I look forward to making everything use tokendata!
Attachment #8661396 - Flags: review?(dylan) → review+
(Assignee)

Comment 12

2 years ago
schema only:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   04b55ee..ff04d30  master -> master
(Assignee)

Comment 13

2 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   2e42540..043c752  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Blocks: 1211750
You need to log in before you can comment on or make changes to this bug.