XF 2.3 Security Recovery Addon

[Ozzy47]

I'm working on an addon that will allow a user to login using a secret phrase. They save the phrase from their user details page like this:

    protected function accountDetailsSaveProcess(\XF\Entity\User $visitor)
    {
        $form = parent::accountDetailsSaveProcess($visitor);
    
        $secretPhrase = $this->filter('secret_phrase', 'str');
        \XF::logError('Secret phrase: ' . $secretPhrase);
    
        if ($secretPhrase !== '')
        {
            // Generate a unique salt for the user (32 bytes = 64 characters)
            $salt = bin2hex(random_bytes(32));
            
            // Generate a random pepper (32 characters)
            $pepper = bin2hex(random_bytes(16)); // Generates 32 hexadecimal characters
    
            // Combine the secret phrase, salt, and pepper
            $combined = $secretPhrase . $salt . $pepper;
    
            // Hash the combined value using Argon2 (stronger hashing)
            $hashedSecretPhrase = password_hash($combined, PASSWORD_ARGON2ID);
    
            // Store the hashed secret phrase, the salt, and the pepper
            $visitor->ozzmodz_secret_phrase_hash = $hashedSecretPhrase;
            $visitor->ozzmodz_secret_phrase_salt = $salt;
            $visitor->ozzmodz_secret_phrase_pepper = $pepper;
    
            // Save the changes
            $form->basicEntitySave($visitor, []);
            \XF::logError('Saving hashed phrase: ' . $visitor->ozzmodz_secret_phrase_hash);
        }
    
        return $form;
    }

Then if they forgot their password and no longer have access to their email, they can go to the recovery page, enter their secret phrase, and if correct it logs them in. We do this like so:

    public function actionRecover()
    {
        $input = $this->filter([
            'username' => 'str',
            'secret_phrase' => 'str'
        ]);
    
        /** @var \XF\Entity\User|null $user */
        $user = $this->finder('XF:User')
            ->where('username', $input['username'])
            ->fetchOne();
    
        if (!$user || !$user->ozzmodz_secret_phrase_hash || !$user->ozzmodz_secret_phrase_salt || !$user->ozzmodz_secret_phrase_pepper) {
            // Log missing data if needed for debugging purposes (this shouldn't be exposed to the user)
            \XF::logError("Missing data during secret phrase recovery for user: " . $input['username']);
            return $this->error(\XF::phrase('secret_phrase_invalid'));
        }
    
        // Retrieve the stored salt and pepper (they should be used only internally)
        $storedSalt = $user->ozzmodz_secret_phrase_salt;
        $storedPepper = $user->ozzmodz_secret_phrase_pepper;
    
        // Combine the user-entered secret phrase with the stored salt and pepper
        $combined = $input['secret_phrase'] . $storedSalt . $storedPepper;
    
        // Hash the combined value using Argon2
        if (password_verify($combined, $user->ozzmodz_secret_phrase_hash)) {
            // Success: Log the user in
            \XF::asVisitor($user, function() {
                // Switching the visitor context
            });
            $this->session()->changeUser($user);
    
            // Log IP if needed for security reasons
            $this->repository('XF:Ip')->logIp($user->user_id, $this->request->getIp(), 'user', $user->user_id, 'secret_phrase_login');
    
            return $this->redirect($this->buildLink('index'), \XF::phrase('you_have_been_logged_in'));
        } else {
            // Log the failed attempt for debugging purposes
            \XF::logError("Failed secret phrase verification for user: " . $input['username']);
            return $this->error(\XF::phrase('secret_phrase_invalid'));
        }
    }

This just does not seem secure to me.

 

[Kirby]

I am by no means a security expert, but I'd say it probably doesn't feel secure because it isn't secure

You are basically just creating a second password / account recovery question (without the question ...) that is subject to the same flaws as passwords (pishing, weak password / phrase, dictionary attacks, etc.).

// Generate a unique salt for the user (32 bytes = 64 characters)
$salt = bin2hex(random_bytes(32));
 
// Generate a random pepper (32 characters)
$pepper = bin2hex(random_bytes(16)); // Generates 32 hexadecimal characters

IMHO this is nonsense.
A pepper is used to store it in a different location than the salt, but you are storing both in the exact same location (user entity).

So its' basically just the equivalent of

// Generate a unique salt for the user (48 bytes = 96 characters)
$salt = bin2hex(random_bytes(48));

... which is somewhat pointless as password_hash() does add a salt by itself anyway.

If you want to continue with the idea I'd at least

• Make sure that the phrase has enough entropy and is not a single word (currently the code would accept even just 1)
• Make sure that the phrase isn't too long either to prevent DoS attacks on the hash algorithm
• Limit successful usage (eg. only once per month) or even better: invalidate after login
• Add a flood check, maybe completely lock the feature after X failures without successful login

A completely different approach could be public key cryptograhpy:

1. Use the Web Crypto API to generate a Ed2259 keypair on the client
2. Store the public key on the server
3. Ask the user to store the private key in a secure location

When doing account recovery

1. Generate a challenge for the user on the server
2. Read the private key on the client and sign the challenge (ideally detachable like a USB stick)
3. Verify that the challenge was signed with the private key using the correspondig public key on the server

Last but not least:
You are doing too much business logic in a controller, move that to services.

 

[Ozzy47]

Hmmm. some things to definitely go through.

 

[Ozzy47]

So XF is doing this with login with password:

• Argon2ID password hashing
• Salted hashes
• Login attempt limiting
• CAPTCHA on repeated failures
• Optional 2FA

What I am currently doing (still building):

• Argon2ID secret phrase hashing
• Salt + pepper hashes (although I need to probably move the pepper)
• Length/entropy checks (min of 16 characters and three words, max of 256)
• Logging and rate-limiting (after 5 invalid attempts, lock out submission for 15 minutes)
• Auto-invalidation after use

I moved the business logic into a service as well. I also am storing the pepper in it's own table, it is still randomly generated when a user creates a secret phrase.

 

[Ozzy47]

I have also added a HP to the form that redirects bots to the index

 

[MentaL]

What i did In vbulletin was have a private field with a secret key that could be used for support. If they knew the key then support could be more given on that account.