the JI /account/2fa page text is misleading, giving the impression that 2FA isn't reset until a successful code is input

[zenmonkeykstop]

Description

When a JI user's TOTP 2FA is reset, the /account/2fa page is displayed with the message "you're almost done!" and inviting the user to "finish resetting your two-factor authentication" by updating their mobile device's app with the new secret. A quick read of the instructions would imply that 2FA is reset when the user confirms it by entering the new verification code.

However at that point 2FA has already been reset in the journalists table in the database.

Steps to Reproduce

• log in to JI, click on account edit link
• click RESET MOBILE APP CREDENTIALS

Expected Behavior

2FA is reset when a new valid code is entered

Actual Behavior

2FA is reset immediately, without any confirmation that the user has recorded the new secret

Comments

To reduce the risk of the user being locked out of their account (by ignoring the CTA to update the secret, or updating the secret incorrectly) either:

1. the page should be updated to say that they must update with the new secret, or
2. the secret should not be updated in the database until the user inputs a valid verification code.

[eloquence]

Tagged as good first issue; to any contributor wanting to work on it, please share an implementation proposal here first. Thank you! :)

[anorthall]

Is this still an open issue? I just tried the steps to reproduce and the text above the button on /account/account reads:

If your two-factor authentication credentials have been lost or compromised, or you got a new device, you can reset your credentials here. If you do this, make sure you are ready to set up your new device, otherwise you will be locked out of your account.

Additionally, when clicking on the button, a javascript alert is produced confirming the user wishes to reset OTP. Is this sufficient?

If not, I would be happy to try and implement a solution where the database is not updated until such time a new secret is provided by the user, and to make the instructions reflect such a scenario.

[anorthall]

Implementation proposal

• Add a new attribute to Journalist model: temp_otp_secret = Column(String(16), nullable=True), and a new method, generate_temp_totp_shared_secret() which will save a temporary secret to the DB
• Change the Reset mobile app credentials button on /account/account to take you to /account/2fa instead of immediately resetting the secret.
• Make /account/2fa generate the temp_otp_secret listed above, and instruct the user that their old secret will not be reset until they verify against the new temporary one.
• Once the user successfully verifies against the temp_otp_secret, update otp_secret with the value of temp_otp_secret and instruct the user that their OTP secret has been reset. Discard temp_otp_secret value.

[anorthall]

Additionally, maybe rather than using a temporary field stored in the database, perhaps the temporary OTP secret could be stored as a cryptosigned cookie. Might give better performance and be a bit cleaner.

[sea-kelp]

Hi @eloquence, I was just wondering if I could pick up this task (if it's still needed)

My implementation proposal is similar to the one suggested by @anorthall above -- to generate a new TOTP secret, which would be stored in the signed Flask session cookie upon visiting the 2fa page, and to commit it to the db only after the user has verified their token.

Current Flow

1. User visits account page and clicks "Reset Mobile App Credentials"
2. Javascript prompt confirming user choice to reset MFA code. User clicks OK
3. POST request to /account/reset-2fa-totp, which generates a new TOTP secret for the user
4. Redirect to /account/2fa which asks user to set up and verify MFA code
5. POST request to account/2fa, which checks the user-inputted MFA token against the TOTP secret stored in the database

New Proposal

1. User visits account page and clicks "Reset Mobile App Credentials"
2. ~~Javascript prompt confirming user choice to reset MFA code. User clicks OK~~ (Not needed because the secret is no longer instantly reset)
3. Button links user to /account/2fa, which: a. Generates a new TOTP secret stored in the Flask session b. Displays instructions asking user to set up and verify MFA code
4. POST request to account/2fa, which checks the user-inputted MFA token against the TOTP secret stored in the Flask session and commits the secret to the database, if verified

Please let me know what you think

[eloquence]

Thanks for offering to help with this @sea-kelp! Yes, I think the original issue is still very much valid. We'll kick the approach you proposed around a bit and will respond within the next few days. :)

[zenmonkeykstop]

Hi @sea-kelp, this approach seems sound to me. We just merged another contributor's change to drop the pyotp dependency in favour of using cryptography's implementation, so this part of the codebase should be stable and ready for you to have at it!

Let us know if you have any questions or run into any problems during the implementation - either here or on the Gitter room at: https://gitter.im/freedomofpress/securedrop

[sea-kelp]

Thanks @zenmonkeykstop! I had a quick question: Do you have any thoughts on what the expected behavior should be if the user visits the /account/2fa page directly?

The /account/2fa route is shared with HOTP and currently just validates the token against the user's current secret (expecting it to be reset in /account/reset-2fa-[ht]otp). With my changes, we will take the HOTP secret as input in /account/reset-2fa-hotp and add it to the session, before redirecting to /account/2fa.

There can be an edge case where, if the user visits the page directly:

• If the user previously input an HOTP secret (and so there an existing secret in the session), it uses the old secret
• If the user did not previously input an HOTP secret, there's no secret to verify

This probably won't affect most users but I thought I'd raise the question before I got too far into this task. Let me know what you think!

[sea-kelp]

Hi @zenmonkeykstop and/or @eloquence, when you have a moment, I'd love to get your thoughts on #6747