argon hashing broken again on PHP 8.1

[GrahamCampbell]

Related to https://github.com/php/php-src/blob/master/UPGRADING#L296, possibly?

---

Last time: https://github.com/shivammathur/php-builder/issues/2.

[GrahamCampbell]

ValueError: A thread value other than 1 is not supported by this implementation

[shivammathur]

@GrahamCampbell It detects libargon2 correctly while configuring the build. So, I don't think it is related to the pkg-config change. https://github.com/shivammathur/php-builder/runs/2163085742?check_suite_focus=true#step:5:2167

[shivammathur]

@GrahamCampbell It works with default options. https://github.com/shivammathur/test-setup-php/actions/runs/677305948/workflow

As per my understanding, Libargon support multiple threads, but implementation in PHP only supports a single thread. https://github.com/php/php-src/blob/master/ext/sodium/sodium_pwhash.c#L66

This is because crypto_pwhash API in sodium library has no parameter for threads. https://github.com/jedisct1/libsodium/blob/master/src/libsodium/include/sodium/crypto_pwhash.h#L104 https://github.com/jedisct1/libsodium/issues/986#issuecomment-665533993

It worked before this patch was added in PHP.

[GrahamCampbell]

Oh, but this is working on 8.0.3?

[shivammathur]

@GrahamCampbell Yes, it does, as per the code it should not. Maybe I'm missing something.
Can you file a bug report here https://bugs.php.net as people working on internals would know more about this.

[GrahamCampbell]

That code you link to was what was causing the original issue on PHP 8.0, but then after we loaded the extension properly, the issue went away. That code that throws the value error is not being executed on PHP 8.0.x.

[shivammathur]

@GrahamCampbell ok. Is there a commit where this changed

[GrahamCampbell]

I can't see anything. How odd.

[GrahamCampbell]

Like you, I am confused how this ever worked. Also, if I remove the threads argument, it still works on 8.0, but still crashes on 8.1 with a different error:

$ php81 -r "echo password_hash('password', PASSWORD_ARGON2I, [
    'memory_cost' => 1024,
    'time_cost' => 2,
]);"
PHP Fatal error:  Uncaught ValueError: Unexpected failure hashing password in Command line code:2
Stack trace:
#0 Command line code(2): password_hash()
#1 {main}
  thrown in Command line code on line 2

[shivammathur]

@GrahamCampbell time_cost has to be minimum 3 for argon2i

• https://github.com/php/php-src/blob/master/ext/sodium/sodium_pwhash.c#L86
• https://github.com/jedisct1/libsodium/blob/6d566070b48efd2fa099bbe9822914455150aba9/src/libsodium/include/sodium/crypto_pwhash_argon2i.h#L49
• https://github.com/jedisct1/libsodium/blob/6d566070b48efd2fa099bbe9822914455150aba9/src/libsodium/crypto_pwhash/argon2/pwhash_argon2i.c#L160

[GrahamCampbell]

Thanks for looking into this. I guess Laravel's tests are broken, and need to be changed. :P

[GrahamCampbell]

I don't understand why they'd have these config options if they can't be used. https://www.php.net/manual/en/function.password-hash.php

[shivammathur]

Yes, these need to be documented better, as with PHP 8.1 it will throw exceptions for wrong options.

[GrahamCampbell]

Why would threads be included as an option if it's not configurable? There must be some other code which gets run instead?

[shivammathur]

It was implemented with this rfc https://wiki.php.net/rfc/sodium.argon.hash. My guess is the thread option was kept with hardcoded value of 1, so that if a future version of libsodium accepts threads, it can be implemented without any changes to PHP API by removing hardcode internally. This did not affect users until it started throwing exceptions. Now that it does, the documentation of password_hash function should have details about acceptable ranges for options. Please create a bug report on PHP, requesting to update the UPGRADING file regarding this.