[Discussion] Password hashing API View as Markdown

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).

2. 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=<num>). 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.

3. 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.

4. 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.

5. 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=<num> 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=<num> 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.