extend 2fa protection beyond login

RESOLVED FIXED

Status

()

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2fa protection needs to also apply to:
- password changing
- api-key generation
- disabling the api_key_only preference
Depends on: 1199088
+ password resetting ('forgot password' workflow)
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.
(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.
(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.
Blocks: 1201983
Blocks: 1202393
No longer depends on: 1199088
Blocks: 1202886
Blocks: 1202281
Priority: -- → P1
Blocks: 1203008
Posted patch 1199087_1.patch (obsolete) — Splinter Review
- 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. :)
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-
- 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)
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 :-)
Blocks: 1205680
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+
schema only:

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