two-factor
two-factor copied to clipboard
WordPress
Encrypt TOTP secrets.
This exposes a versioned authenticated encryption interface (powered by libsodium, which is polyfilled in WordPress via sodium_compat).
Encryption is opportunistic: Unencrypted TOTP secrets will automatically be upgraded to an encrypted secret upon retrieval or update of their TOTP secret.
Version 1 of the under-the-hood protocol uses the HMAC-SHA256 hash of a constant string and SECURE_AUTH_SALT to generate a key. This can be changed safely in version 2 without breaking old users.
Version 1 uses XChaCha20-Poly1305 to encrypt the TOTP secrets. The authentication tag on the ciphertext is also validated over the user's database ID and the version prefix.
Threat model
- Protects against read-only SQLi due to encryption.
- Protects against copy-and-paste attacks (replacing TOTP secrets from one account with another's), since the ciphertext+tag are bound to the user's database ID.
- Protects against chosen-ciphertext attacks (IND-CCA2).
- Does not protect against replay attacks.
- Does not protect against attackers capable of reading the salt from the filesystem.
Cryptography Primitives
- Key derivation: HMAC-SHA256 of a high-entropy secret
- Encryption: XChaCha20-Poly1305
This relies on the constant "SECURE_AUTH_SALT" what if it ever changes? The decrypt will not work, especially if not backup of the old wp-config is found. It's rare, but a legit way to force everybody to re-login.
To clarify, maybe this isn't a problem with Two-factor, But I came here from the Application Password intergration Guide And @georgestephanis refered here as a way to store application password. In this way it can cause problems.
This relies on the constant "SECURE_AUTH_SALT" what if it ever changes? The decrypt will not work, especially if not backup of the old wp-config is found.
Using a different constant is fine. I didn't want to muck with WP Core for this PR.
@georgestephanis any thoughts on a review of this PR and whether it's ready for consideration in v0.8.0?
Given the 0.8.0 focus on U2F deprecation, I'm going to punt this to a future release.
@soatok I'd abstracted out the encryption methods into the parent class to make them easier for other providers to adopt as well -- any concerns on that front from your end? I renamed a couple of them slightly to yoink the explicit totp references there.
Also -- with sodium_compat in wp core, what's the point of adding it to composer here? Is it more like ... safety compat for old versions of core or is it still necessary for the travis_ci?
@soatok I'd abstracted out the encryption methods into the parent class to make them easier for other providers to adopt as well -- any concerns on that front from your end? I renamed a couple of them slightly to yoink the explicit totp references there.
No concerns with what's implemented. Other providers may want to add domain separation versus yours, so we might want to expand serialize_aad to support a "provider type" input. But beyond that, this should be fine.
Also -- with sodium_compat in wp core, what's the point of adding it to composer here? Is it more like ... safety compat for old versions of core or is it still necessary for the travis_ci?
It's mostly just to ensure the build passes. If it gets it from WP, that's sufficient, and it can be removed.
What is the reasoning behind the HMAC ?
Just use libsodium to generate a random key on activation and write it to wp-config.
wp_salt must not be used here as you will never be able to rotate them again
I wasn't confident in my ability to write to wp-config as I'm not a WordPress developer.
It might not be possible to write to it on some hosts. That should not compromise the security of the majority.
But regardless, hashing the key does not seem to serve a purpose. Unless Im missing something
It might not be possible to write to it on some hosts. That should not compromise the security of the majority.
OK, well, if you or someone else wants to tackle the wp-config stuff, I'd prefer that over messing with salts.
But regardless, hashing the key does not seem to serve a purpose. Unless Im missing something
It does two things:
- It uses a PRF (HMAC-SHA256) to ensure that the actual key used for encryption is a uniformly random bit string (the WordPress salts are not, but they have sufficient entropy within)
- It uses a constant to provide domain separation from any other protocols that hash the salt.
But either way, it'd be better to replace that with a wp-config directive.
Any reason not to use https://paragonie.com/book/pecl-libsodium/read/04-secretkey-crypto.md ?
Secretbox is secure encryption, but not AEAD.
AEAD lets you authenticate both some encrypted data and additional data. This lets you bind an encrypted value to some context and prevent confused deputy attacks.
Secretbox is secure encryption, but not AEAD.
AEAD lets you authenticate both some encrypted data and additional data. This lets you bind an encrypted value to some context and prevent confused deputy attacks.
Yes, the only additional data that is relevant here is the user_id right?
This means an attacker with write access to the DB can't swap a user's TOTP secret with one from another user I guess.
Correct. That's why we want AEAD instead of just authenticated encryption.
I don't think this will work since it seems that the plugin does not have a build step to generate a Release artifact that contains composer dependencies.
And since the plugin want to support Wordpress 4.3 sodium_compat is not available.
I believe its only in 5.2
https://paragonie.com/blog/2019/05/wordpress-5-2-mitigating-supply-chain-attacks-against-33-internet
Not that I'd care about dropping support for prehistoric versions but its not my decision.
A potential issue I'm chasing is that I think WP Core isn't properly loading the sodium_compat polyfill. Specifically, it is only loaded if sodium_crypto_box is not defined in wp-includes/compat.php. However, it should always be loaded because some distros don't have all of the sodium functions defined, specifically sodium_crypto_aead_xchacha20poly1305_ietf_encrypt. See the php72compat.php file and how it's loaded in sodium_compat/autoload.php
I haven't been able to confirm on those PHP installations specifically, currently chasing user reports. But it's my current working theory.
https://github.com/WordPress/WordPress/blob/32c0cfe3b3e1047e532ecd6a8c133512acefefba/wp-includes/compat.php#L341-L343
I haven't been able to confirm on those PHP installations specifically, currently chasing user reports. But it's my current working theory.
Some functions are intended to be loaded independently; i.e. https://github.com/paragonie/sodium_compat/blob/master/lib/stream-xchacha20.php and https://github.com/paragonie/sodium_compat/blob/master/lib/ristretto255.php
See also: https://github.com/paragonie/sodium_compat/blob/cb15e403ecbe6a6cc515f855c310eb6b1872a933/autoload.php#L60-L75
So, yes, your hypothesis is 100% correct. WordPress should probably use one of the Ristretto255 functions as its toggle for "is the extension loaded?"
Regardless of what follows below, two quick pointers on reading this PR:
- SECURE_AUTH_SALT as used as the encryption key isn't going to always be defined on older installations
- SECURE_AUTH_SALT may be defined as
put your unique phrase hereon semi-broken configs -
wp_salt( 'secure_auth' )exists as a wrapper for those reason, which pulls from the DB instead, which kind of makes this not exactly SQLi-proof.
So it might be better to use something like $key = defined( 'TWO_FACTOR_TOTP_ENCRYPTION_KEY' ) ? TWO_FACTOR_TOTP_ENCRYPTION_KEY : wp_salt( 'secure_auth' );
But looking at this PR, I'm questioning if encryption is actually what's needed here?
The attack surface being protected against here really boils down to: a) meta_value exposure (via SQLi or badly configured plugins) b) meta_value being copied to another user (rare human error, which I can't realistically see being an issue that matters)
That leads me to suggest that perhaps instead of storing the raw key in the database, it should've been storing a seed to the key instead. That isn't something that could be progressively-upgraded for existing users though, it'd only be viable for TOTP's created going forward.
For example, exposure of the meta_value is just as protected in this example, as it would be if it were encrypted using the same key
function get_user_totp_key( $user_id ) {
- return (string) get_user_meta( $user_id, self::SECRET_META_KEY, true );
+ $key = (string) get_user_meta( $user_id, self::SECRET_META_KEY, true );
+ $key = hash_hmac( 'sha1', $user_id . '|' . $key, wp_salt( 'secure_auth' ), true );
+ return self::base32_encode( $key );
}
I haven't put much thought into this, past validation that the key lengths remain the same, and that meta_value is useless by itself..
I get it, hashing is not encryption. I'm not claiming it is, or is a viable replacement for it, only that in this situation, encryption isn't the only answer to the problem of "Avoid account takeover with SQLi" when it comes to TOTP's.
If a hash was used, I think it'd need to use a dedicated constant. Otherwise it wouldn't be possible to rotate the WP salts without requiring users to setup 2FA again.
If a hash was used, I think it'd need to use a dedicated constant. Otherwise it wouldn't be possible to rotate the WP salts without requiring users to setup 2FA again.
Correct.
@dd32
b) meta_value being copied to another user (rare human error, which I can't realistically see being an issue that matters)
It's called a Confused Deputy Attack.
@dd32
b) meta_value being copied to another user (rare human error, which I can't realistically see being an issue that matters)
It's called a Confused Deputy Attack.
Spot on
If a hash was used, I think it'd need to use a dedicated constant. Otherwise it wouldn't be possible to rotate the WP salts without requiring users to setup 2FA again.
The same is true of the current proposed encryption, as rotating salts will result in existing secrets being unable to be decypted, and most people rotating salts won't have the previous values saved for long. That's why I suggested a dedicated constant TWO_FACTOR_TOTP_ENCRYPTION_KEY as an option above, but it wouldn't be used by all.
It's called a Confused Deputy Attack.
I was taking it back to the basic implementation failure, rather than the security term used, to make it easier for non-security folk understand. In the case of my comment, the attack was still taken into account, and I would still expect that any encryption done would still use a per-user seeded encryption.
The other aspect worth considering here is the export/import of users - Does two-factor follow the export/import? is it expected that it'll be operational on the new site? (it won't be, if it uses different salts or encryption key, but it also likely won't have the same user_id either, so perhaps user_login is the appropriate user variable to use there)
I rebased the branch to fix the merge conflicts, so if you have a local copy you'll need to
-
git checkout -b 389-backup -
git fetch && git reset origin/main --hard, orgit pull --rebase
If a hash was used, I think it'd need to use a dedicated constant. Otherwise it wouldn't be possible to rotate the WP salts without requiring users to setup 2FA again.
The same is true of the current proposed encryption, as rotating salts will result in existing secrets being unable to be decypted, and most people rotating salts won't have the previous values saved for long. That's why I suggested a dedicated constant
TWO_FACTOR_TOTP_ENCRYPTION_KEYas an option above, but it wouldn't be used by all.
With just the current PR yes, but part of the idea was to prompt the user when we detect the salt has changed and automatically reencrypt all the stored secrets with the new key.
@iandunn your latest commit will make it impossible to ever rotate salts.
If a hash was used, I think it'd need to use a dedicated constant. Otherwise it wouldn't be possible to rotate the WP salts without requiring users to setup 2FA again.
The same is true of the current proposed encryption, as rotating salts will result in existing secrets being unable to be decypted, and most people rotating salts won't have the previous values saved for long. That's why I suggested a dedicated constant
TWO_FACTOR_TOTP_ENCRYPTION_KEYas an option above, but it wouldn't be used by all.With just the current PR yes, but part of the idea was to prompt the user when we detect the salt has changed and automatically reencrypt all the stored secrets with the new key.
And how do you want to reencrypt all keys if the old key is lost due to salt Rotation?