They really should be hashed :)

It can't be hashed. It'd have to just be encrypted.

My 2 cents:

• for the recovery tokens (scratch_tokens) hashing vs encryption depends on the UI, if the user should be able to view them again after the first generation or not.
• the secret needs to be available to calculate the TOTP, so cannot be hashed and needs to be encrypted. Here again it depends on the UI. If the 2fa is used in the password reset workflow or in any other places where the user password will not be available while calculating the TOTP, then PBKDF2 is not anymore an option unless using a centralized password (higher threat if leaked) or HSM-based solutions (more expensive, in particular if need to sustain high traffic).
• we are keeping the 2fa table in the same database of each wiki and I guess accessed with the same MySQL account (I didn't check the code though, but the wikiuser MySQL user has grants to read it). Have we considered separating it?

In T145915#2792021, @Volans wrote:

• we are keeping the 2fa table in the same database of each wiki and I guess accessed with the same MySQL account (I didn't check the code though, but the wikiuser MySQL user has grants to read it). Have we considered separating it?

Any wiki that uses CentralAuth has it stored in one big table on s7's centralauth db, so logging into any other clustered wiki mean they get the same 2FA protection, and all is hunky dory

Private, fishbowl etc which aren't CentralAuth'd are in their local db.

There is certainly security improvements storing it on other server(s), but the usual reason for doing that so far has been to store things like AFT, Echo, Flow etc in extension1 where they have potentially a lot of data

In T145915#2792021, @Volans wrote:

• for the recovery tokens (scratch_tokens) hashing vs encryption depends on the UI, if the user should be able to view them again after the first generation or not.

Although some sites (such as Google, Twitter) let you view them, we've vaguely decided on another ticket that this isn't probably something we want to do, and as such, need a nicer way to regenerate them

In T145915#2792096, @Reedy wrote:

In T145915#2792021, @Volans wrote:

• we are keeping the 2fa table in the same database of each wiki and I guess accessed with the same MySQL account (I didn't check the code though, but the wikiuser MySQL user has grants to read it). Have we considered separating it?

Any wiki that uses CentralAuth has it stored in one big table on s7's centralauth db, so logging into any other clustered wiki mean they get the same 2FA protection, and all is hunky dory

My point was that both user and 2fa infos are in the same DB

In T145915#2792021, @Volans wrote:

• the secret needs to be available to calculate the TOTP, so cannot be hashed and needs to be encrypted. Here again it depends on the UI. If the 2fa is used in the password reset workflow or in any other places where the user password will not be available while calculating the TOTP, then PBKDF2 is not anymore an option unless using a centralized password (higher threat if leaked) or HSM-based solutions (more expensive, in particular if need to sustain high traffic).

The user may not have a password at all. With AuthManager, a user could use a non-password-based PrimaryAuthenticationProvider (if one is available on the wiki) along with OATHAuth.

In T145915#2792021, @Volans wrote:

My 2 cents:

• for the recovery tokens (scratch_tokens) hashing vs encryption depends on the UI, if the user should be able to view them again after the first generation or not.

I believe I said this in another bug, but it would be better to generate new scratch tokens rather than allowing users to see old ones again. IMO it's a small defense against misuse of scratch tokens, and avoids transmitting the same secret to the user more than once. (And, of course, it has the added benefit of allowing us to hash them!)

In T145915#2792835, @Parent5446 wrote:

In T145915#2792021, @Volans wrote:

My 2 cents:

• for the recovery tokens (scratch_tokens) hashing vs encryption depends on the UI, if the user should be able to view them again after the first generation or not.

I believe I said this in another bug, but it would be better to generate new scratch tokens rather than allowing users to see old ones again. IMO it's a small defense against misuse of scratch tokens, and avoids transmitting the same secret to the user more than once. (And, of course, it has the added benefit of allowing us to hash them!)

T131788

@Xaosflux noticed this vulnerability in a discussion on enwiki, brought it up to me privately, and asked to be included in the discussion when I mentioned we already had a bug about it. Any objections?

My ISP are having great fun with these sorts of things atm... http://www.revk.uk/2016/12/the-totp-seed-storage-dilemma.html

http://sakurity.com/blog/2015/07/18/2fa.html is good fun too

In T145915#2792021, @Volans wrote:

we are keeping the 2fa table in the same database of each wiki and I guess accessed with the same MySQL account (I didn't check the code though, but the wikiuser MySQL user has grants to read it). Have we considered separating it?

I've been meaning to file a bug about that for a while: T183420: Authentication data should not be available through the normal DB abstraction layer

Any objection to making this task public? It is a bit of a stretch of calling this a security vulnerability - it is basically the OATHAuth equivalent of T150647: Deploy EncryptedPassword to Wikimedia Sites which is also not private. (The similar T119451: Consider using "pepper" for our hashed passwords is also not secret, OTOH {T50698} is, so it looks like we don't have consistent standards in this regard...)

Does this mean that scratch_tokens is stored in clear text?

In T145915#5024930, @MarcoAurelio wrote:

Does this mean that scratch_tokens is stored in clear text?

Yes. Those can be hashed though, I believe once shown to the user we don't need the real value anymore.

In T145915#3853275, @Tgr wrote:

Any objection to making this task public? It is a bit of a stretch of calling this a security vulnerability - it is basically the OATHAuth equivalent of T150647: Deploy EncryptedPassword to Wikimedia Sites which is also not private. (The similar T119451: Consider using "pepper" for our hashed passwords is also not secret, OTOH {T50698} is, so it looks like we don't have consistent standards in this regard...)

@Reedy, @sbassett, can we make this public? I agree with @Tgr that this is not really a security vulnerability, more of a hardening feature in case of a DB leak or SQLI.

My proposal to fix this is:

• Use libsodium's crypto_secretbox as described at https://paragonie.com/book/pecl-libsodium/read/04-secretkey-crypto.md.
  • We'll add a new secret key, $wgOATHSecretKey, fallback to core's $wgSecretKey if not set.
  • We can either reuse the user id as the nonce or generate a random string and store it... please advise on what would be preferred.
• The encryption would take place during (de)serialization, so in TOTPKey::newFromArray() and ::jsonSerialize(). During serialization, we'll set a 'encrypted' => true flag so deserialization knows that it's encrypted and not plain text.
  • Note that the deserialization code needs to be deployed first, so that if a group0 user enables encrypted 2FA, the group2 wiki can still deserialize it.
• A maintenance script will load each row, deserialize and reserialize and save again for migration.

I'm intentionally leaving alone scratch tokens because we want to migrate those to something else entirely (T232336: Separate recovery codes into a separate 2FA module), and can figure out a hashing scheme then.

In T145915#7720542, @Legoktm wrote:

• We can either reuse the user id as the nonce or generate a random string and store it... please advise on what would be preferred.

Ignore this question, we have to use a random nonce, because private wikis will have overlapping user ids, and the docs say "The nonce doesn't have to be confidential, but it should never ever be reused with the same key". Anyways, sticking the random nonce in the JSON blob is also trivial. We don't even need the encrypted => flag then, we can just look isset( $data['nonce'] ).

Here's what I've got so far, it works in my limited testing. I would appreciate some feedback or a thumbs up that I'm going in the right direction before I keep working on this.

In the case the secret key is ever leaked and needs rotating, I think it would also be a good idea as a follow-up to have a script that can decrypt using the old, leaked key and then re-encrypt using the new secret key.

In T145915#7727019, @Legoktm wrote:

Here's what I've got so far, it works in my limited testing.

Two questions that come to mind:

• Is $wgSecretKey the right format? The sodium calls will throw otherwise, and while the installer generates a 32-byte key, it doesn't seem to be explicitly required or documented.
• Is this worth making libsodium a hard requirement? It's a low bar for a crypto-related extension, but it would be pretty trivial to skip encryption/decryption when there's no sodium support.

(Nit: the unset() call looks pointless.)

In T145915#7727050, @Tgr wrote:

Two questions that come to mind:

• Is $wgSecretKey the right format? The sodium calls will throw otherwise, and while the installer generates a 32-byte key, it doesn't seem to be explicitly required or documented.

I briefly looked into $wgSecretKey earlier, and yeah, it has no guarantees on format, though I think we can expect most installs to use the generated default. The other thing that concerns me is that currently docs state it's safe to just reset whenever, which would not be true if we started using it for encryption. Maaaybe core should have a $wgEncryptionKey or something with stricter requirements on format and resetability, but I don't really want to get into that now.

• Is this worth making libsodium a hard requirement? It's a low bar for a crypto-related extension, but it would be pretty trivial to skip encryption/decryption when there's no sodium support.

I am leaning to making it optional too, especially since it's bundled and core doesn't require libsodium (yet). This would also mean that we don't need to fallback to $wgSecretKey, and only enable encryption support when libsodium and $wgOATHSecret are set. I think for most installs just having 2FA would be a general increase in security even if the 2FA part is vulnerable to the database being leaked.

I should have another patch for review later this week, thanks for the feedback!

In T145915#7720542, @Legoktm wrote:

@Reedy, @sbassett, can we make this public? I agree with @Tgr that this is not really a security vulnerability, more of a hardening feature in case of a DB leak or SQLI.

Since there's no exploitable/exploited vuln here (unless someone secretly owns our dbs or we're unknowingly leaking it somewhere - no evidence to support those notions right now), we can make this public.

Change 766557 had a related patch set uploaded (by Legoktm; author: Legoktm):

[mediawiki/extensions/OATHAuth@master] [WIP] Optionally encrypt OTP secret in the database

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

oh no :( several times when I lost access, I just looked up the secret in the database.

Jokes aside, thank you for working on this Kunal!

I've updated the patch a bit: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/OATHAuth/+/766557. Would appreciate some CR as this is needed to advance T232336.

Change #766557 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@master] Optionally encrypt OTP secret in the database

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

Change #1187101 had a related patch set uploaded (by SBassett; author: Legoktm):

[mediawiki/extensions/OATHAuth@wmf/1.45.0-wmf.18] Optionally encrypt OTP secret in the database

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

Change #1187101 merged by jenkins-bot:

[mediawiki/extensions/OATHAuth@wmf/1.45.0-wmf.18] Optionally encrypt OTP secret in the database

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

Mentioned in SAL (#wikimedia-operations) [2025-09-11T20:07:34Z] <sbassett@deploy1003> Started scap sync-world: Backport for [[gerrit:1187101|Optionally encrypt OTP secret in the database (T145915)]]

Mentioned in SAL (#wikimedia-operations) [2025-09-11T20:13:24Z] <sbassett@deploy1003> sbassett: Backport for [[gerrit:1187101|Optionally encrypt OTP secret in the database (T145915)]] synced to the testservers (see https://wikitech.wikimedia.org/wiki/Mwdebug). Changes can now be verified there.

Mentioned in SAL (#wikimedia-operations) [2025-09-11T20:20:13Z] <sbassett@deploy1003> Finished scap sync-world: Backport for [[gerrit:1187101|Optionally encrypt OTP secret in the database (T145915)]] (duration: 12m 38s)