Warehouse: 2FA Ask for password not username when disabling 2FA

[jezdez]

Describe the bug
When disabling 2FA for an account, it would make sense to ask for the user's password and not the username for confirmation as that would make it much harder to do drive-by disabling on unattended computers.

Expected behavior

Ask for password when disabling 2FA.

To Reproduce

Try disabling 2FA on https://pypi.org/manage/account/.

[alexwlchan]

Yeah, I thought this was a bit odd.

Separate ticket, but possibly related: should Warehouse require a password to enable 2FA? To prevent similar drive-by enabling of 2FA on unattended computers.

[woodruffw]

This was a subject of some discussion in #5567 (although I'm currently having trouble finding the exact comment).

As far as I know, requiring a password (or just a second factor) when disabling 2FA is not a consistent practice across major services. Both Google and GitHub will prompt you for some kind of authentication if you haven't accessed their security pages in a while but both retain a cookie that allows future visits without re-authentication, eliminating the drive-by protection. Google requires the user's password, while GitHub allows the user to use one of their second factor methods. Twitter requires the user's password twice: once to access the security page, and again to actually disable 2FA.

The theory behind the current behavior is that (1) the user is already in an authenticated context, one where they have (almost) absolute control over the state of their account, (2) anything that strictly decreases the security of their account should require confirmation, but not require them to jump through hoops. Therefore, we make them confirm their intent by typing their username out, but don't encourage security fatigue by re-prompting for their password.

On a more general note, a drive-by attacker can currently do far worse than disable the user's 2FA method: they can transfer all packages to a temporary account, delete the current account (only the username is required), and then create a new, identically named account that they control completely. As far as I know (I might be wrong!), none of that would be visible from an package consumer's perspective.

In summary: I think we should treat drive-by attacks as black swan events and mitigate them in other ways (audit logging, email alerts when an account's security posture changes, &c). That isn't to say that changing the behavior to require a password would be _bad_; I just think it's unnecessary and attempts to cover an attack that I think is unlikely (why reduce the account's security to a single factor when you can own it entirely?)

We could take an approach like Google or GitHub, but I see relatively little payoff: the two are still vulnerable to drive-bys once the user is authenticated.

[jezdez]

This was a subject of some discussion in #5567 (although I'm currently having trouble finding the exact comment).

As far as I know, requiring a password (or just a second factor) when disabling 2FA is not a consistent practice across major services. Both Google and GitHub will prompt you for some kind of authentication if you haven't accessed their security pages in a while but both retain a cookie that allows future visits without re-authentication, eliminating the drive-by protection. Google requires the user's password, while GitHub allows the user to use one of their second factor methods. Twitter requires the user's password twice: once to access the security page, and again to actually disable 2FA.

The theory behind the current behavior is that (1) the user is already in an authenticated context, one where they have (almost) absolute control over the state of their account, (2) anything that strictly decreases the security of their account should require confirmation, but not require them to jump through hoops. Therefore, we make them confirm their intent by typing their username out, but don't encourage security fatigue by re-prompting for their password.

I understand (1) for sure, but don't follow you regarding (2) to be honest. It seems to me that confirmation via a password is not an unreasonable amount of effort, while using the username effectively is no effort at all. IOW entering a username to authorize a possibly dangerous change is not an expected UX pattern (to me!), in contrast to entering a password.

On a more general note, a drive-by attacker can currently do far worse than disable the user's 2FA method: they can transfer all packages to a temporary account, delete the current account (only the username is required), and then create a new, identically named account that they control completely. As far as I know (I might be wrong!), none of that would be visible from an package consumer's perspective.

Sure, although I didn't raise this issue for any complex drive-by scenario, but about one that is relatively easy to achieve with very little exposure to a victim's computer, which would reduce the risk for an attack. Additional means to inform the user when 2FA is disabled would of course be useful.

(Another idea for a more complex attack: a compromised browser extension can disable 2FA without user interaction since it can parse the username from the account settings page)

In summary: I think we should treat drive-by attacks as black swan events and mitigate them in other ways (audit logging, email alerts when an account's security posture changes, &c). That isn't to say that changing the behavior to require a password would be _bad_; I just think it's unnecessary and attempts to cover an attack that I think is unlikely (why reduce the account's security to a single factor when you can own it entirely?)

FWIW I gave feedback to a feature that I'm beta-testing, so this seems like a case of an unfinished product feature. What struck me as odd about the use of a username for disabling 2FA was basically me not having seen the pattern before (at all) and the chance to simply use the established pattern to use the user's password for confirming a possibly dangerous action. If in doubt I think it makes sense to not use the username but the password, basically.

We could take an approach like Google or GitHub, but I see relatively little payoff: the two are still vulnerable to drive-bys once the user is authenticated.

shrug I haven't done any user research to proof my experience, so I let you decide what to do with my feedback.

[pradyunsg]

I too think asking for the username is odd, and asking for the password is more appropriate for an action like removing 2FA. Asking for a username is functionally equivalent to a confirmation dialog but requiring slightly more typing/effort for nothing much here.

I don't think asking for the username for a destructive action is a pattern I've seen elsewhere and switching to either a confirmation dialog or to asking for the password are both wins in this case IMHO.

[brainwane]

@nlhkabu I have a suggestion here, which is: perhaps you could skim https://simplysecure.org/knowledge-base/ to check whether there's research or advice there on what to require of a user in this scenario?

Thanks for elaborating on your thinking here, @jezdez -- I appreciate the detail!

[nlhkabu]

@brainwane thank you for this link - looks like a great resource, however, unfortunately, I couldn't find anything specific to this scenario.

My preference here is to change this to a password validation pattern, based on this feedback.

I don't think it _matters_ if it is no more secure (although obviously it would matter if it were _less_ secure) -> if it is more familiar and/or makes people feel more comfortable, that's enough to justify the change from a UX perspective.

[nlhkabu]

@woodruffw I've added this to the OTF security work milestone. Let's make a PR for this once the webauthn PR is done?

The new modal should look like this:

Additional features:

• The user should be able to toggle the visibility of the password, as on our other password fields
• The button should be disabled until a password is entered (as per the current behaviour)

[woodruffw]

Let's make a PR for this once the webauthn PR is done?

Sounds good to me!

[trishankatdatadog]

@brainwane 💯 agreed that as in with other websites (say GitHub or Google), and assuming the user is logged in, the password should be required to edit any sensitive security setting, not just 2FA. Maybe a decorator would help?

[nlhkabu]

As discussed today with @woodruffw, we plan to update this UI to:

• Ask for password on TOTP modal
• Ask for password and key label on webauthn modal

[pradyunsg]

The button should be disabled until the correct password is entered (as per the current behaviour)

I'll flag that this seems a little odd and tricky to achieve to me; since the password checking should always happen on the backend.

[alexwlchan]

The button should be disabled until the correct password is entered (as per the current behaviour)

I'll flag that this seems a little odd and tricky to achieve to me; since the password checking should always happen on the backend.

Agreed. I wonder if this should actually be:

The button should be disabled until a non-empty password is entered

---

Two notes:

• Elsewhere in the UI, the button is disabled until you enter any password, regardless of correctness.

  I made a todo note a while back to poke at this a bit and see what’s going on. But I went to look just now and I couldn’t find anywhere in the settings panels where the user-side code is checking the correctness of the password. Closest I could find was the “Change Password” section:

  

  Here, the button is disabled until you enter something in the “Old password” field (my password is not three characters 😉 ) and both the passwords entered in the “New password” field match (which is a reasonably check to perform client-side).

  If I go ahead and click “Update password”, I get an error:

  

• The current “disable 2FA” screen checks for correctness of username.

  This is a PyPI username (I think), but it's not my username:

  

  The button doesn’t get enabled until I enter my username – which isn’t an unreasonable check to perform client-side, because my username isn’t secret information. Is this what @nlhkabu meant by “as per the current behaviour”?

[nlhkabu]

Yes, you're right @alexwlchan - I'll update my comment above :)

[brainwane]

This requires some JavaScript work. @yeraydiazdiaz or @di do you have time to help with this?

[yeraydiazdiaz]

I can have a go at it.

[brainwane]

That would be great, thank you, @yeraydiazdiaz! This is an issue we want to resolve before we can declare the WebAuthn beta finished, so I particularly appreciate your help with this.

[jezdez]

😍 y’all!

[di]

Reopening, looks like #6408 broke project and account deletion, so I reverted it in #6525.

Here's the stack trace from an attempt at account deletion:

HTTPNotFound: The resource could not be found.
  File "pyramid/tweens.py", line 13, in _error_handler
    response = request.invoke_exception_view(exc_info)
  File "pyramid/view.py", line 779, in invoke_exception_view
    raise HTTPNotFound
TypeError: macro 'confirm_modal' takes no keyword argument 'index'
  File "warehouse/raven.py", line 40, in raven_tween
    return handler(request)
  File "pyramid/tweens.py", line 43, in excview_tween
    response = _error_handler(request, exc)
  File "pyramid/tweens.py", line 17, in _error_handler
    reraise(*exc_info)
  File "pyramid/compat.py", line 179, in reraise
    raise value
  File "pyramid/tweens.py", line 41, in excview_tween
    response = handler(request)
  File "warehouse/cache/http.py", line 74, in conditional_http_tween
    response = handler(request)
  File "warehouse/sanity.py", line 76, in sanity_tween_egress
    return unicode_redirects(handler(request))
  File "pyramid/router.py", line 148, in handle_request
    registry, request, context, context_iface, view_name
  File "pyramid/view.py", line 667, in _call_view
    response = view_callable(context, request)
  File "pyramid/config/views.py", line 169, in __call__
    return view(context, request)
  File "pyramid/config/views.py", line 188, in attr_view
    return view(context, request)
  File "pyramid/config/views.py", line 214, in predicate_wrapper
    return view(context, request)
  File "warehouse/cache/http.py", line 33, in wrapped
    return view(context, request)
  File "pyramid/viewderivers.py", line 514, in csrf_view
    return view(context, request)
  File "pyramid/viewderivers.py", line 325, in secured_view
    return view(context, request)
  File "warehouse/cache/origin/derivers.py", line 31, in wrapper_view
    return view(context, request)
  File "warehouse/metrics/views.py", line 34, in wrapper_view
    return view(context, request)
  File "pyramid/viewderivers.py", line 460, in rendered_view
    request, result, view_inst, context
  File "pyramid/renderers.py", line 451, in render_view
    return self.render_to_response(response, system, request=request)
  File "pyramid/renderers.py", line 474, in render_to_response
    result = self.render(value, system_values, request=request)
  File "pyramid/renderers.py", line 470, in render
    result = renderer(value, system_values)
  File "pyramid_jinja2/__init__.py", line 265, in __call__
    return template.render(system)
  File "jinja2/asyncsupport.py", line 76, in render
    return original_render(self, *args, **kwargs)
  File "jinja2/environment.py", line 1008, in render
    return self.environment.handle_exception(exc_info, True)
  File "jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "jinja2/_compat.py", line 37, in reraise
    raise value.with_traceback(tb)
  File "/opt/warehouse/src/warehouse/templates/manage/account.html", line 217, in top-level template code
    <button type="button" class="button -js-copy-hash tooltipped tooltipped-e" aria-label="Copy to clipboard" data-original-label="Copy to clipboard" data-clipboard-text="{{ macaroon.id }}">
  File "/opt/warehouse/src/warehouse/templates/manage/manage_base.html", line 212, in top-level template code
    <li>{{ error }}</li>
  File "/opt/warehouse/src/warehouse/templates/base.html", line 150, in top-level template code
    {% block body %}
  File "/opt/warehouse/src/warehouse/templates/base.html", line 252, in block "body"
    {% block content %}
  File "/opt/warehouse/src/warehouse/templates/manage/manage_base.html", line 65, in block "content"
    {% block main %}{% endblock %}
  File "/opt/warehouse/src/warehouse/templates/manage/account.html", line 655, in block "main"
    {{ confirm_button("Delete your PyPI account", "Username", user.username) }}
  File "jinja2/runtime.py", line 579, in _invoke
    rv = self._func(*arguments)
  File "/opt/warehouse/src/warehouse/templates/manage/manage_base.html", line 189, in template
    {{ confirm_modal(title, confirm_name, confirm_string, slug, index=None, extra_fields=extra_fields, action=action) }}

Project deletion does not raise an exception, it just flashes "Confirm the request" even if the user has correctly filled out the modal.

[brainwane]

Contractors on the OTF-funded work need to stop work on the security features in order to ensure we complete the accessibility and internationalization work by the end of the month. Therefore, while this is necessary to get us out of beta for this feature https://github.com/pypa/warehouse/issues/5661#issuecomment-515802931 , I'm removing it from the milestone.

[brainwane]

Is this something @yeraydiazdiaz or @rcipkins could try working on? This is the last item remaining before we get out of beta for the WebAuthn beta https://github.com/pypa/warehouse/issues/5661#issuecomment-515802931 and can then send out a followup email about this feature to pypi-announce https://mail.python.org/archives/list/[email protected]/thread/YTZWD5H4H3VCQTQVPRDLH2TTHVTJS7JQ/ .