Deploy EncryptedPassword to Wikimedia Sites
Open, MediumPublic
Actions

Assigned To

None

Authored By

	tstarling
	Nov 14 2016, 10:41 AM

Description

For improved security given the threat of SQL injection etc., I suggest deploying the EncryptedPassword password class. This is configured with something like:

$wgPasswordConfig['E'] = [
	'class' => 'EncryptedPassword',
	'underlying' => 'pbkdf2',
	'secrets' => [ $wmgPasswordSecretKey ],
	'cipher' => 'aes-256-cbc',
];
$wgPasswordConfig['BE'] = [
	'class' => 'LayeredParameterizedPassword',
	'types' => [ 'B', 'E' ],
];
$wgPasswordDefault = 'E';

Where $wmgPasswordSecretKey is in PrivateSettings.php. I used short names (E, BE) since the name is put into the DB , and the hash size ends up being quite close to the 255 byte maximum that can fit in a tinyblob.

Then add a suitable prefix to old-style bare hashes:

UPDATE user SET user_password = CONCAT(':B:', user_id, ':', user_password) WHERE user_password RLIKE '^[0-9a-f]{32}$';

Then wrap all B-type hashes:

mwscript maintenance/wrapOldPasswords.php --type BE

Requires https://gerrit.wikimedia.org/r/#/c/321359/ and a minor update to wrapOldPasswords.php to stop it from throwing an exception due to $wgAuth->allowSetLocalPassword() being false.

Related incident: https://wikitech.wikimedia.org/wiki/Incident_documentation/20161112-OurMine

Related Objects

Mentioned In: T72910: Logout users with MD5 password hash
T150605: Publish an analysis of the OurMine hack
T145915: OATHAuth OTP shouldn't be stored in cleartext in the DB

Event Timeline

tstarling created this task.Nov 14 2016, 10:41 AM

Restricted Application added subscribers: JEumerus, Matanya, Aklapper. · View Herald TranscriptNov 14 2016, 10:41 AM

Paladox subscribed.Nov 14 2016, 10:59 AM

MarcoAurelio subscribed.Nov 14 2016, 11:19 AM

Do we know how many affected user rows there are? To the extent, do we need to get opsen/dba involved for doing this?

I note the script doesn't respect slave lag, so gonna add a wfWaitForSlaves() call now

In T150647#2792724, @Reedy wrote:

I note the script doesn't respect slave lag, so gonna add a wfWaitForSlaves() call now

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

In T150647#2792724, @Reedy wrote:

Do we know how many affected user rows there are? To the extent, do we need to get opsen/dba involved for doing this?

In enwiki, there's 4.5 million user rows with old unprefixed password hashes, and 12 million user rows with :B: prefixes. I don't think we need DBA support since we can just do the query in batches, with LIMIT or partitioned by user_id.

So, the thinking here is that we are mitigating exposure of old, non-upgraded password hashes correct?

And the assumption is that, were there to be some vulnerability that allows an attacker access to the database, that vulnerability does not also yield access to the encryption key?

In T150647#2794490, @dpatrick wrote:

So, the thinking here is that we are mitigating exposure of old, non-upgraded password hashes correct?

And the assumption is that, were there to be some vulnerability that allows an attacker access to the database, that vulnerability does not also yield access to the encryption key?

This would also wrap them in pbkdf2, so it will help even if the attacker has the encryption key. However, an attacker with db access but no config file access does sound like a very plausible attack scenario too.

https://gerrit.wikimedia.org/r/#/c/321604/ can be used to update the unprefixed and :A: rows in a replication-safe way.

In T150647#2794615, @Bawolff wrote:

In T150647#2794490, @dpatrick wrote:

So, the thinking here is that we are mitigating exposure of old, non-upgraded password hashes correct?

And the assumption is that, were there to be some vulnerability that allows an attacker access to the database, that vulnerability does not also yield access to the encryption key?

This would also wrap them in pbkdf2, so it will help even if the attacker has the encryption key. However, an attacker with db access but no config file access does sound like a very plausible attack scenario too.

Got it. And this infers a, perhaps, less likely scenario in which the encryption key is exposed but the encrypted hashes are not.

What's the rationale for encrypting the hashes rather than strongly rehashing the old hashes and including a type indicating the sequence of hashing needed to verify the submitted password?

(Also, to be clear, I'm not attacking the idea as negative. I'm more interested in documenting our reason for doing this, because if we do get cracked later, we may need to explain why we were hashing and encrypting. Encrypting sort of increases our attack surface since will have an additional key to protect.)

Actually this would do both

It will take the md5 hash, layer pbkdf over top, than layer aes over top that.

Probably blocked on some B hashes incorrectly labeled as A (see T91917) which would be much more annoying to fix if they get wrapped. (Although we are already doing some wrapping via LayeredParameterizedPassword so maybe spilt milk?)

Also, CentralAuth would require its own wrapping script, right?
Is it even worth to do this for the local databases, instead of just removing the passwords? How many non-SUL accounts do we have?

In T150647#2801301, @Tgr wrote:

Also, CentralAuth would require its own wrapping script, right?
Is it even worth to do this for the local databases, instead of just removing the passwords? How many non-SUL accounts do we have?

Probably not many that aren't vandals/throw aways/etc, but honestly I think we should just to have everything hashing related done with.

In T150647#2800391, @dpatrick wrote:

What's the rationale for encrypting the hashes rather than strongly rehashing the old hashes and including a type indicating the sequence of hashing needed to verify the submitted password?

If an attacker has the hash, they can easily verify whether a given password is the correct one. Even if hashing takes 2s, they can still do an offline GPU-based brute force attack against it, checking many passwords per second. If the hash is encrypted, then they need the encryption key to verify whether a guess is correct. So a database dump is effectively useless to the attacker, unless they also compromise the configuration.

Dereckson edited projects, added Wikimedia-Extension-setup; removed Wikimedia-Site-requests.Nov 20 2016, 5:30 AM

Luke081515 subscribed.Nov 20 2016, 11:05 AM

@Dereckson It's not an extension :), it's just core MW config

Peachey88 updated the task description. (Show Details)Nov 21 2016, 10:05 AM

Krenair moved this task from Backlog to External on the Wikimedia-Site-requests board.Dec 10 2016, 4:08 PM

Tgr mentioned this in T145915: OATHAuth OTP shouldn't be stored in cleartext in the DB.Dec 21 2017, 5:16 AM

• chasemp edited projects, added Security-team-backlog; removed Security-Team.Sep 4 2018, 4:10 PM

FWIW, I personally think it would be simpler to just mix a secret (like wgSecretKey but probably a dedicated one) into the hash. I'm not that much a fan of the EncryptedPassword class

Legoktm mentioned this in T150605: Publish an analysis of the OurMine hack.Sep 12 2018, 5:49 PM

sbassett subscribed.Dec 3 2018, 6:09 PM

Xaosflux subscribed.Mar 14 2019, 8:45 PM

• chasemp edited projects, added Security-Team; removed Security-team-backlog.Dec 23 2019, 5:12 PM

• chasemp moved this task from Incoming to Back Orders on the Security-Team board.

• chasemp triaged this task as Medium priority.Dec 23 2019, 5:19 PM

sbassett mentioned this in T72910: Logout users with MD5 password hash.Sep 8 2020, 4:38 PM

Reedy renamed this task from Deploy EncryptedPassword to WMF to Deploy EncryptedPassword to Wikimedia Sites.Sep 2 2021, 4:57 PM

Reedy added a project: Security.

Reedy removed a subscriber: • dpatrick.

Krinkle added a project: Sustainability (Incident Followup).Sep 28 2021, 9:28 PM

Krinkle updated the task description. (Show Details)