Encrypt TOTP secrets.

[soatok]

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

[soatok]

Hmm... This will need a composer install to make it work in Travis CI.

[georgestephanis]

I support this in concept, and the code looks good to me from a quick skim. I'm leaving a preliminary approval until I can have the time to give it a thorough test -- I'm just occupied with Application Passwords stuff at the moment with Beta 1 about to go out for 5.6

[janw-me]

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.

[janw-me]

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.

[soatok]

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.

[jeffpaul]

@georgestephanis any thoughts on a review of this PR and whether it's ready for consideration in v0.8.0?

[jeffpaul]

Given the 0.8.0 focus on U2F deprecation, I'm going to punt this to a future release.

[georgestephanis]

@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]

@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.

[calvinalkan]

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

[soatok]

I wasn't confident in my ability to write to wp-config as I'm not a WordPress developer.

[calvinalkan]

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

[soatok]

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:

1. 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)
2. 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.

[calvinalkan]

Any reason not to use https://paragonie.com/book/pecl-libsodium/read/04-secretkey-crypto.md ?

[soatok]

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.

[calvinalkan]

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.

[soatok]

Or raise the PHP version and use the de-facto PHP implementation straight from the source of the standard.

WordPress raising the minimum PHP version would be great. I wouldn't count on it happening just for 2FA.

That said, I dont know why not just use sodium compat to encrypt secrets.

Which part of sodium_compat? How will you handle key rotation?

[soatok]

To wit: https://github.com/soatok/wp-paseto

[soatok]

Using wp-paseto as a proof of concept, this is how you would protect against Confused Deputy Attacks while supporting key rotation.

<?php

$keyring = new PasetoKeyManager(array(
    'key-id-1' => sodium_hex2bin(/* hex encode your key here */),
    'key-id-2' => sodium_hex2bin(/* hex encode your key here */),
    // ...
    'key-id-65535' => sodium_hex2bin(/* hex encode your key here */)
));


$encrypted = $keyring->encrypt(
    $user_totp_ecret,
    'key-id-65535',
    'totp-secret-for-user' . $user_id
);

As long as there exists a definition that maps key-id to an actual cryptographic secret, you can decrypt this value. This gives you key rotation.

Additionally, the user ID is used (with a hard-coded prefix) as an "implicit assertion" (authenticated data that is not stored in the footer). This gives you a defense against confused deputy attacks.

Feel free to implement the necessary bits yourself elsewhere.

[calvinalkan]

Actually, why do we need encryption with AD here.

Assuming an attacker has WRITE access to the database he could just delete the TOTP settings for the target user in which case the plugin will not require TOTP anyway

[soatok]

Actually, why do we need encryption with AD here.

Assuming an attacker has WRITE access to the database he could just delete the TOTP settings for the target user in which case the plugin will not require TOTP anyway

What if we add a code configuration that enforces TOTP for all users and thus removes that avenue for attack if an attacker only gets access to the SQL database?

AAD is useful for preventing Confused Deputy attacks. If you're going to encrypt, you should always use the AAD mechanism to bind ciphertexts to a given context, even if you don't think you need it.

[calvinalkan]

What if we add a code configuration that enforces TOTP for all users and thus removes that avenue for attack if an attacker only gets access to the SQL database?

Yes, that was my idea also and I think it's the only way do to this.

IE, after all admins of the site configured 2FA, write a constant to the config that makes it mandatory to have 2FA preconfigured on login for admins.

[pkevan]

What if we add a code configuration that enforces TOTP for all users and thus removes that avenue for attack if an attacker only gets access to the SQL database?

We've tried to tackle this outside the plugin (https://github.com/WordPress/wporg-two-factor/blob/trunk/wporg-two-factor.php#L78-L130), it's very much dependant on the site config, and/or the site owners preference as to how far you go with capabilities, since there isn't anything stopping you changing an lower level account to have full access via capabilities.

[iandunn]

This has gotten very long, so I think a summary will help organize things.

Goal

To protect against an attacker who has read access to database, but not write. if they can read the plain TOTP seed then they can generate valid tokens. If they have write access, though, there's nothing we can do.

Different approaches

1. Should we encrypt the seed, or hash it? Hashing would be simpler, but it's not clear if that is vulnerable to confused deputy attacks or not.
   1. If we encrypt, we need to decide which encryption function to use. sodium_crypto_aead_xchacha20poly1305_ietf_encrypt() is native in PHP and polyfilled in Core. PASETO supports easy migration, but would require custom code or bumping to PHP 7.
      1. If using sodium_crypto_aead_xchacha20poly1305_ietf_encrypt(), we'll need to upgrade the minimum plugin requirements to WP 5.2 for the polyfill.
2. Regardless of whether we hash or encrypt, we'll need a salt/key.
   1. We could use wp_salt(), but:
      1. That is sometimes stored in the database, so it wouldn't always protect against attackers with read access to the DB. That's not common, though, so maybe it's good enough, considering that gaining access to the DB is already an edge case?
      2. TOTP would break if Core salts were rotated. Although, the main reason to rotate the Core salts is if they were compromised, and in that scenario it's likely that the TOTP salt would also be compromised and need to be rotated.
   2. Instead of wp_salt(), we could use SECURE_AUTH_SALT directly, but:
      1. It isn't always defined, and;
      2. It doesn't always have a secure value even if it is defined.
   3. Instead of the Core salts, we could create our own new constant, like TWO_FACTOR_TOTP_ENCRYPTION_KEY. We'll need to figure out how to get it into wp-config, though.
      1. We could add it programmatically, but that isn't entirely reliable.
      2. We could ask admins to do it manually, but it's unlikely that many would, so they'd be left unprotected.
   4. A middle ground might be to use our dedicated key if it exists, but fallback to wp_salt( 'secure_auth' ) if it doesn't. That way admins could have the most secure option if they're willing to do the work, but if not they'd still have more protection than they do today. That may introduce more complexity when rotating, though.

Miscellaneous notes

1. The salt/key should be tied to a specific user, but user_login should be used instead of user_id, because the import/export process doesn't preserve IDs.
2. We want a way to rotate the salt/key without breaking TOTP, or easily migrate ciphertexts from the old salt to new one. We could detect this by keeping a sentinel value, and inform the admin.
3. We can warn admins when there's something that isn't setup securely, and tell them how to fix it.
4. We might want to make the approach generic enough so that other providers can use it.

[iandunn]

Based on the summary above, I think these are the decisions we need to make:

1. Hash or encrypt?
   1. If encrypt, which function?
2. Use only the Core salt, only our own constant, or use Core as the fallback for our constant?
   1. If Core, do we use wp_salt(), or SECURE_AUTH_SALT directly?
      1. If SECURE_AUTH_SALT, what do we do when it's not defined or has a weak value?
   2. If our own, do we create it programmatically, or make admins do it?
      1. If manual, what do we do when it doesn't exist?
3. Should we build this only for TOTP, or make it generic?

[calvinalkan]

@iandunn ping me again here pls if I don't comment within the next days

[pkevan]

If encrypt, which function?

I believe that WP.com uses sodium_crypto_aead_xchacha20poly1305_ietf_encrypt so probably a good choice.

Use only the Core salt, only our own constant, or use Core as the fallback for our constant?

Seems like being separate provides more flexibility, and probably less issues if/when it gets updated.

For creating it, I guess we need to consider read-only file systems, or systems where you can't directly write to files that are controlled by the host - not sure the best way to solve this though.

In an ideal world, the constant would be version controlled by the site itself, so perhaps having the ability to set this outside of the plugin is desirable?

[iandunn]

IMO, the best balance of security and practicality is:

1. Encrypt with sodium_crypto_aead_xchacha20poly1305_ietf_encrypt(). The only downside is that it doesn't make rotation easy like PASETO, but I don't think it's worth the extra complexity of rolling & auditing our own implementation.
2. Use TWO_FACTOR_ENCRYPTION_KEY if an admin manually created it, but wp_salt( 'secure_auth' ) if not. The problem with wp_salt() is an edge case of an edge case, so I don't think it's worth the trouble/bugs/support requests/etc that come with making everyone use the constant. Admins who need the strongest security can easily set it up, though.
3. Use the generic constant name above, because that can't be renamed in the future. The code should be in the TOTP provider for now, though. We can always move it later if needed, when we have a specific use case. YAGNI.

Rotating can be sussed out in #456 , but I posted a comment there with my rough idea.

[iandunn]

@calvinalkan ping 🙂

[peterwilsoncc]

I'm not sure using a salt for encryption is wise.

My understanding of all the salt values stored in WP is that they're designed so they can be changed at any time. This can be useful in instances where everyone needs to be logged out at once. Change the auth salts and cookies are invalidated.

By using the salts to encrypt, there are two potential issues:

• it's no longer possible to change salts as a log everyone out method
• if a user is unaware and changes the constants, they will be locked out of their account/2FA will no longer work, depending on how error checking is done.

[dd32]

I do agree, that basing it off wp_salt( 'secure_auth' ) might not be ideal - just because of the likelyhood of existing documentation saying how to rotate those.
I would be in support of a dedicated option for it, but there's probably some edge-cases that are going to be problematic with that (Such as shared user tables)

The initial value of the option could be derived from a salt on the site at the time of activation however, as long as no other random/time-based seeds are added to it.

[calvinalkan]

Hash or encrypt?

Encrypt with AD.

If encrypt, which function

If possible don't implement ourselves. If PHP5 should be supported I highly recommend using defuse which supports 5.6.

Otherwise, Halite is the de-facto libsodium wrapper in PHP. (Active (GitHub issues) support only for PHP8+)

Halite uses:

• sodium_crypto_stream_xor for encryption and then:
• sodium_crypto_generichash for message authentication.

AD could be the user id or username since both SHOULD be immutable.

Use only the Core salt, only our constant, or use Core as the fallback for our constant

From most ideal to least ideal:

• Value supplied as an $ENV value that points to a file, docker secrets. etc.

• Value supplied through $ENV value directly.

• Value supplied as PHP constant (Risk of being leaked in backups etc.)

• Value not supplied as PHP constant => Try to write custom constant to wp-config.php

• Use key derived from wp-salt (if possible, depends on crypto implementation).

• Use wp-salt (might not, depending on crypto implementation).

  Example code:

declare(strict_types=1);

use LogicException;

use function file_get_contents;
use function is_string;
use function trim;

final class SecretStore
{
    // This is an abstraction for defined()
    private ConstantStore $constant_store;

    /**
     * @var callable(string): ?string
     */
    private $fetch_secret_custom;

    /**
     * @var array<non-empty-string, non-empty-string>
     */
    private array $cache = [];

    /**
     * @param ?callable(string): ?string $fallback
     */
    public function __construct(ConstantStore $constant_store, ?callable $fallback = null)
    {
        $this->constant_store = $constant_store;
        $this->fetch_secret_custom = $fallback ?: static fn () => null;
    }

    /**
     * @param non-empty-string $name
     *
     * @return non-empty-string
     *
     * @see https://github.com/symphonycms/symphonycms/issues/2569 for reasons to prefer $_SERVER over getenv
     */
    public function get(string $name): string
    {
        $value = $this->cache[$name] ?? null;

        if (null !== $value) {
            return $value;
        }

        if (isset($_SERVER[$name])) {
            /** @var mixed $server */
            $server = $_SERVER[$name];

            $file_contents = @file_get_contents((string) $server);

            /** @var mixed $value */
            $value = (false !== $file_contents)
                ? trim($file_contents)
                : $server;
        } elseif ($this->constant_store->exists($name)) {
            $value = $this->constant_store->getString($name);
        } else {
            $value = ($this->fetch_secret_custom)($name);
        }

        if (! is_string($value) || '' === $value) {
            throw new LogicException("The value for secret [{$name}] must be a non-empty-string.");
        }

        $this->cache[$name] = $value;

        return $value;
    }
}

Should we build this only for TOTP, or make it generic?

TOTP only.

[calvinalkan]

$value = ($this->fetch_secret_custom)($name);

This can be any callable that implements the fallback behavior, i.e write to wp-config, use wp-salt, whatever