laravel / fortify

Backend controllers and scaffolding for Laravel authentication.
https://laravel.com/docs/fortify
MIT License
1.62k stars 294 forks source link

Adding context length configuration for 2FA to ensure better security standards #567

Closed MattLoyeD closed 2 months ago

MattLoyeD commented 2 months ago

Summary

We need to ensure a minimum length for 2FA secret, current secret length by default is 80-bit (16 characters), but 128-bit (26 characters) is becoming minimum in some cases and the best default is 160-bit.

It's recommended to use 128-bit or 160-bit because some Authenticator apps may have problems with non-RFC-recommended lengths (Namely https://play.google.com/store/apps/details?id=org.fedorahosted.freeotp).

Proposal

Just add some contextual config('fortify-options.two-factor-authentication.secret-length', 16), it will be retro compatible and secured as well. In https://github.com/laravel/fortify/blob/dd2c276e3df1ac3f47a5ab248178ea35dce1b099/src/TwoFactorAuthenticationProvider.php#L43

<?php
    /**
     * Generate a new secret key.
     *
     * @param  int  $secret_length = 16
     * @return string
     */
    public function generateSecretKey(int  $secret_length = 16 )
    {
        return $this->engine->generateSecretKey($secret_length);
    }

In https://github.com/laravel/fortify/blob/dd2c276e3df1ac3f47a5ab248178ea35dce1b099/src/Actions/EnableTwoFactorAuthentication.php#L37

<?php
   /**
     * Enable two factor authentication for the user.
     *
     * @param  mixed  $user
     * @param  bool  $force
     * @return void
     */
    public function __invoke($user, $force = false)
    {
        if (empty($user->two_factor_secret) || $force === true) {
            $secret_length = (int) config('fortify-options.two-factor-authentication.secret-length', 16);
            $user->forceFill([
                'two_factor_secret' => encrypt($this->provider->generateSecretKey($secret_length)),
                'two_factor_recovery_codes' => encrypt(json_encode(Collection::times(8, function () {
                    return RecoveryCode::generate();
                })->all())),
            ])->save();

            TwoFactorAuthenticationEnabled::dispatch($user);
        }
    }

There is also some adaptation to do on https://github.com/laravel/fortify/blob/dd2c276e3df1ac3f47a5ab248178ea35dce1b099/src/Contracts/TwoFactorAuthenticationProvider.php#L12

driesvints commented 2 months ago

Heya, thanks for submitting this.

This seems like a feature request or an improvement. For these, we'd appreciate a pull request instead so we can look at actual code. If you need feedback about an idea, we suggest to post an idea discussion here first. Please only use the issue tracker to report bugs and issues with this library.

Thanks!