#Security

Automatically closed - issue fixed for 2 weeks with no activity.

Using phpass as a fallback is interesting, I had a question in the MR about the implementation where it's not doing what I'd expect it to do, but conceptually could be a good option.

If we did that though - would we consider bringing the functionality of the phpass module (e.g. updating legacy passwords) back into the default implementation and marking that module obsolete? Since we'll need to keep all the phpass logic around permanently anyway. Could be a follow-up to this issue probably. It's unfortunate that our phpass implementation is not a 'real' phpass implementation (the passwords are only portable to and not from Drupal) but that was a decision taken over a decade ago now.

Added a new MR which builds on top of the length limit approach: When creating a new password hash which is unsuitable for bcrypt, use phpass as a fallback.

That could be accompanied by a requirement check: If bcrypt is in use, then recommend to either enable phpass or switch to argon2.

RE: #90 The new hook seems to be adding entity fields ie nothing to do with the account form fields..

I see this in the UserHooks.php file in the user core module.

  /**
   * Implements hook_entity_extra_field_info().
   */
  #[Hook('entity_extra_field_info')]
  public function entityExtraFieldInfo(): array {
    $fields['user']['user']['form']['account'] = [
      'label' => $this->t('User name and password'),
      'description' => $this->t('User module account form elements.'),
      'weight' => -10,
    ];
    $fields['user']['user']['form']['language'] = [
      'label' => $this->t('Language settings'),
      'description' => $this->t('User module form element.'),
      'weight' => 0,
    ];
    if (\Drupal::config('system.date')->get('timezone.user.configurable')) {
      $fields['user']['user']['form']['timezone'] = [
        'label' => $this->t('Timezone'),
        'description' => $this->t('System module form element.'),
        'weight' => 6,
      ];
    }
    $fields['user']['user']['display']['member_for'] = [
      'label' => $this->t('Member for'),
      'description' => $this->t("User module 'member for' view element."),
      'weight' => 5,
    ];
    return $fields;
  }

This new hook could have a bearing on this issue.

RE: #88
I edited #87 quite a lot before I saw #88 mainly in agreement with #88. Starting to get my head around it. I tracked down your comment in the related issue. So these optional form elements will now be always present in the render array but as 'hidden' fields when not needed? It sounds an enhancement if it is likely to be a restricted list of specific fields for the foreseeable future. If it were likely to grow to more than a few fields might not be an enhancement..

Why would we need a new MR? The current MR shows the diff. And the diff fits two-scrolls.

The re-arrangement of the code is exactly what we want, that's clear in the issue summary.

No lenghty comments are added or deleted, they are just moved.

This just needs someone willing to RTBC it.

RE #86 Yes, after running the test-only test seems pointless since no test coverage so far.

Just fyi rebases typically aren’t needed unless the MR no longer applies or if testing is randomly failing for a bug fix

Rebased 221 commits.

Test-only test passes:

ℹ️ Changes from faec1421c65a694b320f6c807c9546b4dde9482d
core/modules/user/src/AccountForm.php
If this list contains more files than what you changed, then you need to rebase your branch.
1️⃣ Reverting non test changes
grep: warning: * at start of expression
grep: warning: * at start of expression
↩️ Reverting core/modules/user/src/AccountForm.php
grep: warning: * at start of expression
2️⃣ Running test changes for this branch
Exiting with EXIT_CODE=0
Running after_script 00:01
Running after script...
$ sed -i "s#$CI_PROJECT_DIR/##" ./sites/default/files/simpletest/phpunit-*.xml || true
sed: can't read ./sites/default/files/simpletest/phpunit-*.xml: No such file or directory
Uploading artifacts for successful job 00:00
Uploading artifacts...
WARNING: ./sites/default/files/simpletest/phpunit-*.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-2058319) 
WARNING: ./sites/simpletest/browser_output: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-2058319) 
ERROR: No files to upload                          
Uploading artifacts...
WARNING: ./sites/default/files/simpletest/phpunit-*.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-2058319) 
ERROR: No files to upload                          
Cleaning up project directory and file based variables 00:01
Job succeeded

Alright going to leave for others

This was suggested by @alexpott in #1998648-52: "Administration pages language" user account setting should be hidden if not used → . Let's land this 12 years later.

Clean up. The existing tests passing is the test.

Can put back into review if you disagree though

If the scenario is already covered then why make the change? Assuming this change is to solve a problem that maybe wasn’t covered before.

What test coverage is this missing?

We don't need to test every form allows altering, that's not specific for any specific form but the form API, which is already covered.

Since there's been no follow up going to close this one out. If still a valid task please re-open.

Debated turning to a feature request but think we should have some test coverage that altering is possible.

Nextcloud is using argon2 since a long time. It appears that they introduced it around NC 14.0.2 (2018).

There are reports on nextcloud forums about login problems after server migrations though. But it looks like people are able to figure out what is causing them.

Argon2 support in Fedora 40 is available via libsodium:

# dnf install php-cli php-sodium

Argon2 support in CentOS 9 is available via epel and libsodium:

# dnf install epel-release
# dnf install php-cli php-sodium

It seems to me that argon2 support is widely available in PHP distro packages. I haven't found any distro which is shipping PHP without argon2 support lately.

Notable exceptions: Fedora 40 (and older), CentOS 9 (and older) https://github.com/znerol-scratch/php-argon2-survey

It seems to me that argon2 support is widely available in PHP distro packages. I haven't found any distro which isn't shipping PHP without argon2 support lately.

I guess the reason why PHP did not switch to argon2 for password hashing by default is because they still are not sure which implementation they want to use. They have compile time support for argon2 via libargon2, via libsodium and recently they added argon2 via openssl (PHP 8.4).

#28 will break if someone switches hosting or PHP version and suddenly Argon2 is no longer available, I am not sure we should do this.

Opened a draft MR with the idea from #28.

One option (instead of pre-hashing) is to force use argon2id for new passwords which are unsuitable for bcrypt.

One of the advantages of using standard password hashing algorithms instead of a custom one is that the resulting hashes are portable. With bcrypt and argon2, it is easier to migrate user records to and from external systems. This gets more complex and error prone with (selective) pre-hashing.

This is true, but the worst that happens with a non-portable hash is that the user can't log in and has to reset their password. If we enforce a length limit for existing accounts, we'll be doing that anyway. If we don't change the behaviour for existing passwords, then it doesn't feel like a complete fix (I guess they will eventually change their password naturally though). So if we only pre-hash passwords over 72 bytes it feels like the least worst of 2-3 bad options - all the other password hashes would remain portable.

Thinking about how we handle existing user accounts if we change the hashing behaviour...

If we start to enforce a limit on the password length, we'd need to decide whether we try to do something helpful / graceful in the UI e.g. tell users that supply a longer input that they should only provide the first 72 bytes of their password, or try to do that for them automatically (and tell the user this is what's happening?).

That may be easier said than done if e.g. the password includes multi-byte characters and the 72 byte threshold falls in the middle of a character. The maxlength attribute "is measured in UTF-16 code units, which is often but not always equal to the number of characters", but I'm not certain what that would mean in all cases where existing passwords exceed the limit in bytes.

I'd think we'd certainly want to avoid e.g. enforcing a limit in the login form which effectively makes it impossible for users with passwords that exceed the limit to authenticate (perhaps they'd be forced to do a password reset - but we can't know that's always available on all Drupal sites?)

If we start pre-hashing passwords (would we do that universally or only when the input is longer than a threshold?), we'd perhaps need to re-generate the stored password hash for existing user accounts (this has been done before in Drupal when the hashing algorithm changed).

Presumably that means:

• A user with a >72 byte password would be successfully authenticated based on the first 72 bytes of the password they supply matching the existing hash.
• We then generate a new hash based on the full password they've supplied and store that for future use.
• The password they've given may or may not match what they originally gave - we can only ever know that the first 72 bytes matched.
• From then onwards, the password they give would need to fully match the one used to generate the new hash.

No way around that AFAICS, but it's pretty ugly; doesn't change the fact we're only considering the first 72 bytes at the moment anyway.

I think I still prefer a length limit. It could be combined with a setting or container parameter, e.g., bcrypt_length_limit_mode: [warn, enforce]. That could default to warn for now (generate a log message when a long password is created / verified) and in a later release could be switched to enforce (throw an exception when a long password is created, still log when verified). Site owners then can choose whatever works best for their use case.

One of the advantages of using standard password hashing algorithms instead of a custom one is that the resulting hashes are portable. With bcrypt and argon2, it is easier to migrate user records to and from external systems. This gets more complex and error prone with (selective) pre-hashing.

Both Symfony and WordPress base64 encode the output of the pre-hash function, which is the mitigation for the NULL byte problem discussed in that article.

Need to verify whether this (still) actually applies, but here's a strong argument not to pre-hash (or at least to approach doing so with extreme caution):

https://blog.ircmaxell.com/2015/03/security-issue-combining-bcrypt-with....

In one way I think we should follow Symfony, given that we use other Symfony components it might make sense for us to switch to the Symfony PasswordHasher component one day instead of managing this ourselves; if we use compatible code then that helps us to switch in future. But that does not follow the best practice identified by @mcdruid in #20...

Symfony does SHA512 followed by base64:

base64_encode(hash('sha512', $password, true));

This is actually 88 bytes long, but the Symfony maintainers considered it secure enough for bcrypt to then truncate to 72 bytes.

WordPress does SHA384 with a fixed pepper of wp-sha384 followed by base64.

base64_encode(hash_hmac('sha384', trim($password), 'wp-sha384', true));

This results in a 64 byte string.

In both cases the base64 encoding is required because the raw hash may contain a NULL 0x00 character, which bcrypt cannot handle:

php > print password_hash(chr(0), PASSWORD_BCRYPT);
PHP Warning:  Uncaught ValueError: Bcrypt password must not contain null character in php shell code:1

There's an argument that it's best not to use just a plain hash in the pre- step before bcrypt - e.g. instead of:

bcrypt(sha512($password))

..you'd do:

bcrypt(sha512($salt + $password))

..or use a "pepper" instead of salt, or use a HMAC.

Some references:

• https://neilmadden.blog/2023/04/27/i-still-dont-really-get-hash-shucking/
• https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sh...

Given Symfony and WordPress pre-hash longer passwords then we could follow that? Then we avoid any UX issues; the only difficulty is backward compatibility in identifying pre-hashed vs non-pre-hashed for existing passwords over 72 bytes, but WordPress handles this with a known prefix on the hashed string.

Symfony hashes passwords longer than 72 bytes with sha512 before hashing with bcrypt: https://github.com/symfony/symfony/pull/40920

I think this would be my preference, can't really see how this would be less secure than forcing someone to choose a shorter password.

It looks like WP changed the algorithm to bcrypt in february and they seems to pre-hash passwords: https://core.trac.wordpress.org/changeset/59828 (line 2706).

If we go the way as proposed in the MR, then I think it will be better if we not fail silently in case the password will be longer - so agree with #14 that we should add at least some information for user.

Adding some other reference points:

Symfony hashes passwords longer than 72 bytes with sha512 before hashing with bcrypt: https://github.com/symfony/symfony/pull/40920

WordPress hashes passwords longer than 72 bytes with sha384 before hashing with bcrypt: https://github.com/WordPress/wordpress-develop/pull/7333

For context, discussion around the same thing in Laravel:

https://securinglaravel.com/security-tip-limiting-bcrypt-passwords-to-72...

In Laravel 12 they added a configurable flag to have the Hash class throw an exception when given input longer than the hashing algorithm properly supports.

There's some follow-up discussion about what approach to take in future e.g. adopting a different hash algo. by default.

Added a feature request for search_api to support boolean operators, it is in very early stages.

@pganore1@gmail.com regarding the highlighting I am working in another issue about that topic, @see #3031390, It may solve your issue if solr and the highlight processor is properly configured.

Re #4

We should however test the UX when bcrypt is in use and a password between 73 and 128 characters is attempted to be stored.

For DX it would be possible to customize the exception message thrown in PasswordItem::preSave() when PasswordInterface::hash() returns FALSE.

That would work for API requests i think (JSON:API, GraphQL, REST).

For UX it might additionally be possible to make the PasswordConfirm element aware of algorithm limitations. Another possibility would be to conditionally add '#maxlength' keys in AccountForm::form() → .

Added a draft MR which implements a limit when bcrypt is in use. I expect that PhpPasswordTest will fail (it is testing 512 byte passwords). So that test will need to be updated.

Since ✨ Add libargon2 and/or libsodium Active the test containers come with argon2 support and ✨ Introduce kernel parameters allowing to specify password hashing algorithm and options Active makes it easier for sites to change the password hashing algorithm. The latter issue also contains some test code targeting specifically bcrypt and argon2.

Nobody can type that with a single keystroke and even if they saw it displayed or printed they would be unlikely to be able to reproduce it, therefore to me they are not really comparable.

While we may not observe it on a desktop frequently, Emoji keyboards on mobile devices have been a feature for years.

doesn't that emoji have far more entropy

Knowing the keyboard a victim uses will likely significantly reduce the entropy size.

I'm not 100% sure of the math here. I do agree that a "single visible emoji" is going to have more entropy than "a single printable ASCII character" I'm not sure that logic extends when we begin counting multiple characters and encountering truncation.

Is my 3 input emoji ( I may have used 6 or 8 emojis however the remainder are truncated away) more secure than an 8 character ASCII password (gut instinct is yes), however what happens when let a password manager generate a significantly larger password such as 25 character? I suspect there is at least a bit of error here in assuming only 3 are accepted as the combination could reasonably be assumed to include a mix of long and short emojis making it closer to an actual '6 digit' password.

But in practical terms, doesn't that emoji have far more entropy and hence is much more secure than a single ASCII letter, surely? Nobody can type that with a single keystroke and even if they saw it displayed or printed they would be unlikely to be able to reproduce it, therefore to me they are not really comparable.

In PHP it turns out that this has been known about since 2013 -

Correct, this is a well known and well documented limitation disclosed in the API spec for the PHP Language. PHP as part of its API spec defers responsibility for this validation to callers ( I'm not sure I agree with that, however it is the status quo of the API) .

GitLab had the same problem reported several years ago, which is still open:

I'm not sure why they haven't taken this as seriously believing it to be a feature request, however I will note as contrast that Drupal <10.1.0 did fully support >72 byte passwords, making this a security impacting regression where users who upgraded from 10.0.11 had working >72 byte passwords. Users may have even tested this implementation at the time only to have it silently change once a site upgrade to 10.1.0.

then an alternative is to add a new password implementation

I will note there was some discussion regarding NIST 800-63b in the issue that caused this regression.

There is some contention around the PBKDF2 vs other algorithms (I do agree the analysis appear well founded it is on the weaker side). I have seen much less discussion regarding Balloon. In both cases a NIST 800-63b calls for approved algorithm is to be paired with the key derivation function.

Standard deployment guides can run behind the times as they are less frequently updated. I do not fault Drupal for considering its options and trying to choose a stronger hashing system than one standard suggested. This issue however equally shows why some algorithms are not "approved" given their inherent risks and becomes an example of the dangers present in going down a less defined path.

This is at least worth keeping in mind, especially if Drupal or Drupal CMS have plans to target US Government or corporations aligning themselves to the NIST standards.

I also don't think that in practice the 72 character limit is an issue for most users or sites, even with password managers in widespread use.

To reiterate, this is 72 bytes, not characters. UTF-8 4byte could make this as low as 18 characters.

The 18 character limit does not include what I could best describe as "iconography". 72 bytes of consumption becomes significantly easier to imagine in the terms of emojis. Engineers of my generation may never have even considered using an emoji in password, yet we now have them common in younger generation lexicon.

👨‍👩‍👧‍👦 appears on my devices as a single discreet 'character', it is in-fact 4 emojis combined with the "zero width joiner" for a total of 25 bytes consumption. On devices that do not render this out to a single graphical emoji it should (If I understand correctly) render as the individual man+woman+girl+boy emoji's with no (or very little) spacing between them.

3 entries of 👨‍👩‍👧‍👦 is sufficient to to exceed 72 bytes.

While researching this issue I found that GitLab had the same problem reported several years ago, which is still open:

https://gitlab.com/gitlab-org/gitlab/-/issues/220580

If we don't like #4 then an alternative is to add a new password implementation that hashes the password with e.g. sha256 before calling bcrypt (or perhaps we only hash if the password is over 72 characters?). Some sources recommend this and some applications have implemented it, while others recommend against it.

In PHP it turns out that this has been known about since 2013 - see https://stackoverflow.com/questions/16594613/how-to-hash-long-passwords-... - and phpass is suggested as a viable alternative!

Refusing to hash a password longer than 72 characters is what Spring decided to do for CVE-2025-22228: https://github.com/spring-projects/spring-security/commit/46f0dc6dfc8402...

Further discussion about a regression that this caused: https://github.com/spring-projects/spring-security/issues/16802

I don't think we should be changing password algorithms again. password_hash() is only guaranteed to implement bcrypt, Argon2 is a compile time option and we cannot guarantee it is available. Therefore to switch to another algorithm we would need a new dependency, or as a last resort, swap back to phpass.

I also don't think that in practice the 72 character limit is an issue for most users or sites, even with password managers in widespread use.

I do think the suggested proposal is acceptable, given that we have both hash() and check() methods on PasswordInterface.

Callers to hash() for newly stored passwords will reject the password if it doesn't fit the criteria. If we limit this check to bcrypt then users who have configured Argon, or any future password algorithm, will not be affected.

Callers to check() will retain backward compatibility because that will still accept longer passwords; only new passwords will be affected.

We currently enforce maxlength 128 on password elements. I don't think we should change this here. We should however test the UX when bcrypt is in use and a password between 73 and 128 characters is attempted to be stored.

I'm still relatively new to contributing to core, but I’ve been trying to understand this issue and the potential risks around unserialize() and serialized field storage.
Please feel free to correct or improve them:

1. Add a Warning in FieldItem Schema
Since 'serialize' => TRUE can be risky, we could document something like the following in FieldItemInterface::schema():

When using 'serialize' => TRUE, the FieldItem class must ensure only safe values (such as arrays or scalars) are stored. Arbitrary PHP objects should not be serialized.

2. Use allowed_classes in unserialize()
Wherever unserialize() is used (for example, in SqlContentEntityStorage::mapFromStorageRecords()), it might be safer to include:

unserialize($data, ['allowed_classes' => false]);

Alternatively, we could use a limited allowlist of known-safe classes to prevent PHP object injection vulnerabilities.

3. Deprecate Unrestricted unserialize() Usage
We could consider marking all usages of unserialize() without the allowed_classes parameter as @deprecated, and encourage the use of a safer wrapper function or utility.

4. Add a PHPCS Rule
A PHPCS rule could help identify unsafe uses of unserialize() during development—similar to how we detect unsafe render arrays.

5. Audit Use of 'serialize' => TRUE in Core and Contrib
It might be worth reviewing all field types in core and contrib that use 'serialize' => TRUE, to ensure they are not accepting object instances via setValue() or unsafe API data.

I propose to harden PhpPassword::hash() and refuse creating new password hashes for passwords with a length of over 72 bytes if bcrypt is in use.

I'm not sure about this one:

1. The password isn't 'more secure' if we prevent longer passwords that bcrypt can support, it's the same amount of security, we're just forcing people to input shorter ones. It'll also be really confusing if we enforce a length on the login form so that previously 'valid' passwords become invalid. I can see validating new/changed passwords though.

2. As things stand, when we change to a password hashing algorithm that supports longer passwords, existing accounts with passwords that exceed the 72 byte limit would get rehashed with their full password length on the new algorithm, if we've enforced a shorter password, they won't get the benefit of the algorithm supporting longer passwords until they change their password again.

However the options for not doing this aren't great either, we could:

1. Not do anything, and try to improve the password hashing algorithm again asap so passwords get rehashed.

2. Hash the password with a different hashing algorithm prior to sending it to bcrypt, only if it's over 72 bytes.

closing open tags. adding reference back to a private issue that reported this.