Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for Argon2 support before using constant PASSWORD_ARGON2X #1228

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

z-e-r-0-t
Copy link
Contributor

Description

Adding a new mail account on a system without PHP Argon2X support an exception will be triggered.

e1

-> Check if Argon2X is supported in PHP (==constant is defined) before using it.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Adding a mail account on a system without PHP Argon2X support was possible after patch

@d00p
Copy link
Member

d00p commented Jan 16, 2024

The API in EmailAccounts.add() uses the setting system.passwordcryptfunc to determine the hash to be used. This setting, when opening the security settings, gets all available password-hashes (see https://github.com/Froxlor/Froxlor/blob/v2.1/actions/admin/settings/210.security.php#L53) , which uses Crypt::getAvailablePasswordHashes() (see https://github.com/Froxlor/Froxlor/blob/v2.1/lib/Froxlor/System/Crypt.php#L97) which only adds available hashes to the select for the setting

So, most likely a hash-algorithm is/was set which became unavailable.

@d00p
Copy link
Member

d00p commented Jan 16, 2024

Here's a more "systemwide"-safe solution:

diff --git a/lib/Froxlor/Settings.php b/lib/Froxlor/Settings.php
index dad81814..056ed8ed 100644
--- a/lib/Froxlor/Settings.php
+++ b/lib/Froxlor/Settings.php
@@ -26,6 +26,7 @@
 namespace Froxlor;

 use Froxlor\Database\Database;
+use Froxlor\System\Crypt;
 use PDO;
 use PDOStatement;

@@ -159,7 +160,17 @@ class Settings
                $result = null;
                if (isset(self::$data[$sstr[0]][$sstr[1]])) {
                        $result = self::$data[$sstr[0]][$sstr[1]];
+
+                       // if password-crypt-func is requested, check whether it's (still) available
+                       if ($sstr[0] == 'system' && $sstr[1] == 'passwordcryptfunc') {
+                               $available_hashes = Crypt::getAvailablePasswordHashes();
+                               if (!array_key_exists($result, $available_hashes)) {
+                                       $result = PASSWORD_DEFAULT;
+                                       self::Set('system.passwordcryptfunc', $result);
+                               }
+                       }
                }
+
                return $result;
        }

@z-e-r-0-t
Copy link
Contributor Author

z-e-r-0-t commented Jan 16, 2024

system.passwordcryptfunc is set to PASSWORD_BCRYPT/2y when the error occurs.

The problem is that the PHP version used doesn't support Argon2. If Argon2 is unsupported the constants PASSWORD_ARGON2I and PASSWORD_ARGON2ID aren't defined in PHP -> exception is triggered e.g at https://github.com/Froxlor/Froxlor/blob/dbf83c6f24b4dfdc6749eddc1b4c7e03878b5ae5/lib/Froxlor/Api/Commands/EmailAccounts.php#L160

@d00p
Copy link
Member

d00p commented Jan 16, 2024

Yes, you're right. Totally missed that part - your fix seems exactly what is needed

@d00p d00p merged commit 8304701 into froxlor:main Jan 16, 2024
9 checks passed
@z-e-r-0-t z-e-r-0-t deleted the argon2_fix branch January 16, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants