[PHP-DEV] [Discussion] Password hashing API

Mailing Lists php.internals
1

Hi all,

While working on the bcrypt-wrap RFC and a couple of minor patches, I noticed some oddities in how the password hashing API handles error conditions and edge cases.
No vulnerabilities, but things that may hide userspace bugs or go against user assumptions.

  1. For unrecognized hashes, password_needs_rehash() always returns TRUE. For the logic behind this to be consistent, it is likely relying on password_verify() being able to verify passwords by blind proxy to crypt(), but not necessarily a good assumption if that’s not possible.
  • An unintentional consequence might be 2b → 2y rehashes in language-agnostic tech stacks.
  • There are various edge cases for which it doesn’t matter anyway, but because malformed hashes of supported algorithms also count as unrecognized, it’s also silencing that error condition (which can be malicious).
  1. For valid hashes matching the requested $algo, password_needs_rehash() evaluation is based on $options difference alone.
  • Right now only relevant for Argon2, but it’s clearly the intended behavior to ignore hash version (encoded v=). There are scenarios for which this is preferred (e.g. canary deployments not causing damage), but also depending on validation logic goes against problem 1 (always return TRUE for unrecognized hashes) and may even cause conflicting behavior.
  • In a contrived/inverted way to the previous point, 2b upgrade wouldn’t be triggered in this branch of logic if only it wasn’t treated as unrecognized.
  • Unlike password_hash(), password_needs_rehash() ignores erroneous $options and will silently return TRUE even for an invalid bcrypt cost.
  1. The userspace-provided salt option warning is triggered even for Argon2, even though it was never supported for it.
  • This is most likely a bug, as I’m also fixing a related issue stemming from the original Bcrypt-only code being reused (https://github.com/php/php-src/pull/24281). However, it might also be seen as intentional “salt” option treatment at the global scope and I’ve been working around that to avoid a BC break.
  • Since the salt option is ignored since PHP 8.0, it’s probably time to think of dropping the warning entirely.

  1. The $algo parameter in all functions accepts the internal integer enums (Bcrypt = 1, Argon2 variants 2/3). This is a remnant from the PHP 5.5-7.3 behavior where the $algo constants did have integer values, but today it’s more of a leaky abstraction and a pain to work around because Argon2 isn’t guaranteed.

  2. password_get_info() has inconsistent output, related to problem 4, but also to problems 1,2.

  • “algo” is “2y” and “algoName” is “bcrypt” for Bcrypt, but only “2y” is recognized.
  • “algo” and “algoName” always match (the “as is” Argon2 variant, functional equivalent to “2y” in the Bcrypt scenario), but with the added inconvenience of the libargon2 implementation returning “unknown” (missing implementation just wasn’t flagged during code review)
  • Encoded v= versions are not accounted for.

Some of these are trivial to fix, others require more care, but I do have a plan to fix all of them and this would be my project for the next few months. However, some changes naturally require more explicit consensus on the correct behavior, and RFC(s) for my bolder ideas. Here’s my ideal target behavior aside from trivial bugfixes:

  • password_needs_rehash() to trigger exceptions for outright malformed hashes and erroneous options; at first likely with an opt-in flag for it.
  • password_needs_rehash() gets some sort of “strategy” option to determine how algorithm versions and/or unrecognized hashes are treated.
  • Non-2y Bcrypt variants are recognized and handled accordingly.
  • “Known unknowns” like md5_crypt and sha256_crypt are also recognized and validated, not having an entirely blind proxy to crypt() (shield it from computing on hashes we know it doesn’t recognize).
  • password_get_info() gets an “algoVersion” output element, at least to provide for the encoded v= in modern algorithms, but Bcrypt variants can also go in there.
  • $algo integer values are phased out; this is probably the only one that can’t land in PHP 8.7 as more than just a deprecation warning.
  • Drop the “salt” option being ignored warning entirely, but when exactly? Is PHP 9 realistically close in the future?

Perhaps this escalated way too quickly from “some oddities” into major plans. Unfortunately, if I were to “trickle in” smaller parts there would be a lot more questions and doubts on why I’d be pushing for something. I wanted to put all of the issues up at once as to make it obvious that ad-hoc patches can only go so far. Thoughts?

Cheers,
Andrey.