Deploy EncryptedPassword to WMF
Open, Needs TriagePublic

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.

Restricted Application added subscribers: JEumerus, Matanya, Aklapper. · View Herald TranscriptNov 14 2016, 10:41 AM
Reedy added a subscriber: Reedy.Nov 14 2016, 4:42 PM

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

Reedy added a comment.Nov 14 2016, 4:43 PM

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

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

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.

dpatrick added a subscriber: dpatrick.EditedNov 15 2016, 2:28 AM

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?

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.

dpatrick added a comment.EditedNov 16 2016, 8:08 PM

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.

Tgr added a subscriber: Tgr.Nov 17 2016, 1:08 AM

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?)

Tgr added a comment.Nov 17 2016, 1:14 AM

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?

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.

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.

Reedy added a subscriber: Dereckson.

@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

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