Attempting to disable TOTP OATHAuth using a scratch code fails, but still consumes a scratch code
Closed, ResolvedPublicSecurity
Actions

Assigned To

Authored By

	Xaosflux
	Fri, Apr 26, 2:03 AM

Description

Steps to reproduce:

Have 2FA enabled
Log in
Go to Special:Manage_Two-factor_authentication
Use Disable for TOTP
Use a scratch code for the validation

What happens:
Successful disabling is reported
2FA is NOT actually disabled
The scratch code is consumed

What should have happened:
2FA should have been disabled

Security risk: Availability
Users may be locked out of their account without any ability to recover

Was able to replicate this in production on testwiki, also received private user reports of the same problem

Details

Author Affiliation: Wikimedia Communities

Related Objects

Mentioned Here: T244348: Recovery option for WebAuthn

Event Timeline

Xaosflux created this task.Fri, Apr 26, 2:03 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFri, Apr 26, 2:03 AM

Xaosflux added a project: MediaWiki-extensions-OATHAuth.Fri, Apr 26, 2:04 AM

Xaosflux added a subscriber: RoySmith.Fri, Apr 26, 9:19 AM

Xaosflux changed Author Affiliation from N/A to Wikimedia Communities.Fri, Apr 26, 12:09 PM

This seems like a regression from https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OATHAuth/+/964988 - using a scratch token updates the key data to mark that as used and the not-great implementation of OATHUserRepository::persist() gives all keys new IDs means that the removeKey call in TOTPDisableForm won't find the key to delete.

Tagging MediaWiki-Platform-Team for code review for a patch that I'm going to upload shortly.

Does this need to stay private? I'm not immediately seeing any ways to abuse this.

It probably doesn't need to be private, as the avenue for triggering the availability problem requires you to already have the secret information for the account that would be impacted

taavi changed the visibility from "Custom Policy" to "Public (No Login Required)".Fri, Apr 26, 1:28 PM

taavi changed the edit policy from "Custom Policy" to "All Users".

Change #1024687 had a related patch set uploaded (by Majavah; author: Majavah):

[mediawiki/extensions/OATHAuth@master] Fix disabling TOTP keys with scratch tokens

https://gerrit.wikimedia.org/r/1024687

gerritbot added a project: Patch-For-Review.Fri, Apr 26, 1:29 PM

Xaosflux moved this task from Backlog to Bugs on the MediaWiki-extensions-OATHAuth board.Fri, Apr 26, 1:46 PM

I'm not familiar with the codebase, so this may be a silly question...

The patch description describes a bug in persist() and how it is being replaced by updateKey() but the old code is still in place because of a dependency from WebAuthn. Does this mean that there still exists a different scenario that exposes this problem when the buggy code is invoked by WebAuthn?

In T363548#9747981, @RoySmith wrote:

I'm not familiar with the codebase, so this may be a silly question...

The patch description describes a bug in persist() and how it is being replaced by updateKey() but the old code is still in place because of a dependency from WebAuthn. Does this mean that there still exists a different scenario that exposes this problem when the buggy code is invoked by WebAuthn?

Not 100% sure of your questions, however webauthn doesn't support any recovery at all currently (T244348)

sbassett added a project: SecTeam-Processed.Fri, Apr 26, 2:36 PM

sbassett moved this task from Incoming to Watching on the Security-Team board.

sbassett added a subscriber: Reedy.

Xaosflux renamed this task from Attempting to disable OATHAuth using a scratch code fails, but still consumes a scratch code to Attempting to disable TOTP OATHAuth using a scratch code fails, but still consumes a scratch code.Fri, Apr 26, 2:39 PM

Reedy added a project: MW-1.42-release.Fri, Apr 26, 2:43 PM

Reedy updated the task description. (Show Details)Fri, Apr 26, 3:12 PM

Thanks for the quick fix @taavi!

Change #1024687 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] Fix disabling TOTP keys with scratch tokens

https://gerrit.wikimedia.org/r/1024687

Change #1024713 had a related patch set uploaded (by Majavah; author: Majavah):

[mediawiki/extensions/OATHAuth@REL1_42] Fix disabling TOTP keys with scratch tokens

https://gerrit.wikimedia.org/r/1024713

In T363548#9747981, @RoySmith wrote:

The patch description describes a bug in persist() and how it is being replaced by updateKey() but the old code is still in place because of a dependency from WebAuthn. Does this mean that there still exists a different scenario that exposes this problem when the buggy code is invoked by WebAuthn?

As far as I can see, no, at the moment WebAuthn doesn't seem to be doing anything that would cause this bug to appear. However there is a patch in review that could have introduced similar issues, and I've sent an additional patch (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WebAuthn/+/1024705) to fix the potentially problematic use of persist() there as well.

ReleaseTaggerBot added a project: MW-1.43-notes (1.43.0-wmf.3; 2024-04-30).Fri, Apr 26, 7:00 PM

alaa subscribed.Fri, Apr 26, 10:06 PM

Change #1025174 had a related patch set uploaded (by Majavah; author: Majavah):

[mediawiki/extensions/OATHAuth@wmf/1.43.0-wmf.2] Fix disabling TOTP keys with scratch tokens

https://gerrit.wikimedia.org/r/1025174

Change #1025174 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@wmf/1.43.0-wmf.2] Fix disabling TOTP keys with scratch tokens

https://gerrit.wikimedia.org/r/1025174

Mentioned in SAL (#wikimedia-operations) [2024-04-29T08:17:59Z] <taavi@deploy1002> Started scap: Backport for [[gerrit:1025174|Fix disabling TOTP keys with scratch tokens (T363548)]]

Mentioned in SAL (#wikimedia-operations) [2024-04-29T08:20:28Z] <taavi@deploy1002> taavi: Backport for [[gerrit:1025174|Fix disabling TOTP keys with scratch tokens (T363548)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-04-29T08:33:23Z] <taavi@deploy1002> Finished scap: Backport for [[gerrit:1025174|Fix disabling TOTP keys with scratch tokens (T363548)]] (duration: 15m 27s)

I backported this to 1.43.0-wmf.2 so this is now fixed on Wikimedia sites. Leaving the task open until the backport issue to REL1_42 issue is resolved so this stays visible in the MW-1.42-release board.

ReleaseTaggerBot edited projects, added MW-1.43-notes (1.43.0-wmf.2; 2024-04-23); removed MW-1.43-notes (1.43.0-wmf.3; 2024-04-30).Mon, Apr 29, 9:00 AM

Jdforrester-WMF moved this task from Blocker to Not a blocker on the MW-1.42-release board.Mon, Apr 29, 2:53 PM

sbassett changed the task status from Open to In Progress.Mon, Apr 29, 5:56 PM

sbassett triaged this task as Medium priority.

sbassett awarded a token.

Change #1024713 merged by Majavah:

[mediawiki/extensions/OATHAuth@REL1_42] Fix disabling TOTP keys with scratch tokens

https://gerrit.wikimedia.org/r/1024713

Maintenance_bot removed a project: Patch-For-Review.Sun, May 5, 7:30 AM

taavi closed this task as Resolved.Sun, May 5, 7:34 AM