Provide argon2i(d) password hashing from sodium when needed by sgolemon · Pull Request #4012 · php/php-src

sgolemon · 2019-04-05T23:01:06Z

Implementation for https://wiki.php.net/rfc/sodium.argon.hash

remicollet · 2019-04-06T07:29:08Z

Great thanks for this.
Especially useful on distro where libargon is not available
(ex RHEL which also don't have libsodium, but shared ext can be provided on some alt repo)

ext/sodium/sodium_pwhash.c

nikic · 2019-04-08T10:34:12Z

ext/sodium/tests/php_password_hash_argon2i.phpt

+Argon2 provider: string(%d) "%s"
 Using password: string(44) "%s"
-Hash: string(95) "$argon2i$v=19$m=1024,t=3,p=2$%s"
+Hash: string(%d) "$argon2i$%s"


What's the reason for these test changes? What output does libsodium produce?

m, t, and p represent the values for memory_cost, time_cost, and threads, respectively. In the original version of the test, I included the expected values because they were only coming from the standard/libargon implementation where we had PHP-defined defaults (1024/3/2 as base values). If the implementations are coming from libsodium, then the defaults are coming from libsodium as well and are different (more aggressive/secure) values.

Therefore, I changed the expected hash strings to simply verify that the produced hash contains a valid looking hash for the protocol, without being quite so strict about what's in it.

I could have kept the v=19 portion since they happen to both produce v19 hashes, but I'm future-proofing newer versions of libargon/libsodium producing v20 hashes. They'll still potentially fail if one produces a version not supported by the other, but that's a different problem than is actually being tested for here and will only show in the argon-not-available case.

I think we should be using the same defaults independently of which implementation is used. It would be surprising if the behavior of password_hash differs depending on how exactly your PHP is compiled/loaded. Would there be any problem with aligning the ext/standard defaults to match what libsodium recommends (or whichever is stricter)?

For "threads" (p=# in the hash output), the current PHP default is 2, while libsodium is hardcoded at 1, we can't override that.

For the other two, I'm using the "MODERATE" values (crypto_pwhash_OPSLIMIT_MODERATE and crypto_pwhash_MEMLIMIT_MODERATE). What that means for differences from our defaults in ext/standard:

For "time_cost" we both use 3.

For "memory_cost", there's a BIG difference. 1024 for standard, 268435456 (256k) for libsodium.

Upping ext/standard's memory_cost default to should probably be pretty safe, though it should be noted that this would NOT be allocated from the per-request "engine" memory allocator. I'm not clear what the effect of having a different "threads" number is.

More to the point, there's no hard guarantee that libsodium won't change their recommended defaults at some arbitrary time in the future. In fact, we can probably safely assume they will.

btw, I should also note these tests I'm changing are brand new tests that I just wrote a week or two ago. Prior to that, there was nothing validating that we had particular defaults.

IMHO we should not be using the MODERATE constants provided by libsodium and instead specify the values explicitly. The behavior of password_hash() should be predictable across a PHP version and be independent of whether libargon2 or libsodium is used, and maybe more importantly, which libsodium version is used. Otherwise you may end up with surprises when migrating code from one environment to another and hashing taking either much more resources than anticipated or conversely being weaker than intended. If we think that 256k is a reasonable memory_cost default for PHP (independently of what libsodium specifies), then we should use that for both ext/standard and ext/sodium implementations.

It's not about the test changes for me here, but about having consistent behavior (at least as far as possible) independent of the backing implementation.

I'm fine using PHP's constants where possible. But as I said, sodium doesn't let us specify thread, and sodium uses a different number than us (currently), so if consistency is important, perhaps we should lower PHP's value? Maybe raise memory at the same since we're changing stuff.

So: 1/3/256k for all?

Current best practices suggest much higher defaults. According to libsodium's docs, INTERACTIVE uses 64mb of ram, not 256kb. and moderate uses 256mb...

Currently, it's significantly weaker than the default bcrypt: https://twitter.com/solardiz/status/1112665842922205184

Also, memory_cost for the options array should be in kilobytes, not bytes. 1024 means 1024 kilobytes, where libsodium expects it in bytes. So you should multiply the memory cost by 1024 to keep it consistent between libraries.

According to libsodium's docs, INTERACTIVE uses 64mb of ram, not 256kb. and moderate uses 256mb...
Also, memory_cost for the options array should be in kilobytes, not bytes...

Yeah, the "256k" I was referring to was the 256MB of MODERATE as described here:

For "memory_cost", there's a BIG difference. 1024 for standard, 268435456 (256k) for libsodium.

I expressed it a bit poorly by not describing ext/standard as 1048576 (1024), but never mind all that. My question stands:

What numbers do we want to use at the end of the day? 1/3/256k(interpreted as 256m when passing to sodium) still seems like the best compromise between the two.

What numbers do we want to use at the end of the day? 1/3/256k(interpreted as 256m when passing to sodium) still seems like the best compromise between the two.

Sounds good to me.

MaxSem · 2019-06-11T21:37:45Z

If I understood it correctly and this change indeed introduces an inability to verify some hashes produced by older PHP versions, this is extremely problematic as it leaves no path for migration. Not being able to generate new hashes with complexity values not considered safe anymore is fine, failing to verify is not. That's how you get users locked out of their accounts. For example, Wikipedia doesn't require users to provide an email during registration, and we have plans to migrate to Argon2 in the future - we may want to avoid doing that if this PR is merged.

sgolemon · 2019-06-23T15:45:34Z

If I understood it correctly and this change indeed introduces an inability to verify some hashes produced by older PHP versions

Only for very specific cases. Cases which are ALREADY broken.

If a site is using libargon to produce argon2i(d) hashes currently (say on PHP 7.3), and upgrades to PHP 7.4 with libargon still enabled, then verification will work fine. This is true with or without this patch.

If they upgrade to 7.4 and don't build in libargon support, then those hashes will fail to verify. This is also true with or without this patch.

This is because the libargon implementation supersedes the implementation made available by sodium. This patch only makes a broken situation slightly less broken.

MaxSem · 2019-07-02T02:33:45Z

Most people don't build PHP themselves, they just use what packages provide. So now people will be at mercy of their OS vendors or AMP packagers to build with libargon instead of libsodium, otherwise their hashes are a toast. Furthermore, a significant share of users can't even compile PHP or install packages, therefore they will not be able to fix

php > echo password_hash('password', PASSWORD_ARGON2I);
$argon2i$v=19$m=1024,t=2,p=2$U1VjYlY1TlF1b2dHblAySQ$kEl02TGiY/bK7okaUHe+c4Dq8projmFpjjtCE8Ubv7U

^^^ This hash, generated with PHP defaults, will not work with Sodium.

Pleasepleaseplease, it's much better to have no Argon2 in some cases rather than have 2 implementations, one of which is backwards incompatible. For now, I'm having to do this: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/520154/

nikic · 2019-07-03T11:03:51Z

@MaxSem Do you know which distros actually ship PHP linked against libargon2? I would have expected for it to be approximately "none" to avoid having an extra dep on the core package.

remicollet · 2019-07-03T12:08:57Z

@nikic Fedora have / use lilbargon2, while RHEL don't

remicollet · 2019-07-05T09:15:09Z

@sgolemon can you please add the constant definition for PASSWORD_ARGON2I and PASSWORD_ARGON2ID which are missing (tested using --with-sodium --without-password-argon2)

BTW, I don't understand which case are broken ?

$ php passargon.php gen
PHP_VERSION: 7.3.8-dev
..............
..............
..............
$ php passargon.php check
PHP_VERSION: 7.4.0-dev
PASSWORD_ARGON2_PROVIDER: sodium
..............
..............
..............
$ php passargon.php gen
PHP_VERSION: 7.4.0-dev
PASSWORD_ARGON2_PROVIDER: sodium
..............
..............
..............
$ php passargon.php check
PHP_VERSION: 7.3.8-dev
..............
..............
..............

1 line per algo, dot means "ok", using https://gist.github.com/remicollet/9adb2f5bfe670f1fc02806ec7aec5376

sgolemon · 2019-07-05T15:16:39Z

Pleasepleaseplease, it's much better to have no Argon2 in some cases rather than have 2 implementations, one of which is backwards incompatible.

Is it better to have two versions of PHP, one of which has any argon2 support at all, and one of which doesn't? Because that's the same in terms of brokenness. If this patch didn't go in, you'd still have the same problem because it's possible for someone to create an Argon2id hash on one install, and have a PHP that doesn't verify it on another.

WITH this diff, you as a mediawiki developer, at least have the option of enforcing "time cost must be greater than 2" so that you know you'll never risk generating a hash that can't be verified on a version of PHP that at least has /some/ argon support, regardless of where it comes from.

You're asking me to make your life harder and I'm not going to do that.

For now, I'm having to do this: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/520154/

Do you have a code review process? Because your commit contains half a comment: /* Argon2 support is disabled because PHP 7.4 enforces what does it enforce? I'm not sure you even know.

sgolemon · 2019-07-05T15:22:22Z

@remicollet What version of libsodium do you have installed? :/
Can you paste a few of the generated hashes you have?

sgolemon · 2019-07-05T15:40:02Z

@remicollet Huh.... seems sodium is more accepting in what it'll verify than in what it will generate.
This whole thing may be an entirely non-issue created from my misunderstanding.

@MaxSem You're still wrong about your use case though. I encourage you to think it through more carefully, even if the question no longer matters to you.

remicollet · 2019-07-06T04:59:47Z

What version of libsodium do you have installed? :/

Latest ;)
Version in Fedora / EPEL: https://rpms.remirepo.net/rpmphp/zoom.php?rpm=libsodium (so 1.0.18)

Huh.... seems sodium is more accepting in what it'll verify than in what it will generate.

+1

Can you paste a few of the generated hashes you have?

Generated by 7.3.8-dev

    "2": {
        "secret": "$argon2i$v=19$m=1024,t=2,p=2$SzBKU255c0wvVmZTVHRhdQ$3uD846D70ecS4M0sudrCFLTulZqFwOJhORtwO2YRqFs",
        "coucou": "$argon2i$v=19$m=1024,t=2,p=2$RThENXlhTDhrR3pUZEIzYg$I\/xVrU75X1szyscVbhwnFSV6hjRjn1msxEbVXvmn0xQ",
        "LexjfvSDK": "$argon2i$v=19$m=1024,t=2,p=2$TXNRSTFKRUxOR2hoVWo1TA$nFF4fn8uTpT6pzcTOEWxjwl8ABAkfqDER06oNLvllcY",
        "password": "$argon2i$v=19$m=1024,t=2,p=2$Zkp6VXRzNjB0SzJ3L25oaA$Njv4BuPwY2\/auJ1ToKh2wF7pJh\/5VrwwYwbmXMcvDKU",

    "3": {
        "secret": "$argon2id$v=19$m=1024,t=2,p=2$dXpuaDFDNThEWEtYLnFwWg$jUYY3Dds+YM93VICRtyD50w\/KvGstPUmpIuLusk7cFQ",
        "coucou": "$argon2id$v=19$m=1024,t=2,p=2$OG9zOTJOLzRVZDhzZEk0MQ$QRoxPiRNmhh\/cvN1goTGHKP8NwKYXUBj6A2LBw+wbAs",
        "LexjfvSDK": "$argon2id$v=19$m=1024,t=2,p=2$SEJBQkpLcEdYZnIxVWlobA$NfohVw0sSJOp2n5iDzVsOfas4YwOieWA4WMm5LLteW8",
        "password": "$argon2id$v=19$m=1024,t=2,p=2$SGpIeG1RSUhGUERZS1oxaA$cU5jVkPfwJiZo8WTJA+74ZQyKzEwMlN9nIX+fmOCeQo",

Generated by 7.4.0-dev

    "argon2i": {
        "secret": "$argon2i$v=19$m=262144,t=3,p=1$hK22ikQdn1VUckzskAgdcg$2eLZmK9MRZjPk4VHVrB6h9RWMaIjrSnxwcL4uYLKuak",
        "coucou": "$argon2i$v=19$m=262144,t=3,p=1$9Bvi+S1WjtOIkOUvee50RQ$nLzAxZT5CBTXbZpCeFKgpveW\/8DGAvvO4YQuI7Tah+E",
        "LexjfvSDK": "$argon2i$v=19$m=262144,t=3,p=1$a4Qpf3bX8RNSdZAcVlTiUg$9N4l4gqluzuGpuQZSNGWNHapCqBPrjK5mvodUj\/8\/iI",
        "password": "$argon2i$v=19$m=262144,t=3,p=1$r7HeSHzzI9UDr8\/Gw57Pyg$WA4MIbnWoiJfJhpXHftE0DFqIGUxCkcTjHJWwZrXO\/M",

    "argon2id": {
        "secret": "$argon2id$v=19$m=262144,t=3,p=1$L8XRFkImcF45Y5GYG5VNZQ$CIbPn\/sFQVnHtbEWjtWcm0VOjHiNr739eb+Ee1JH66c",
        "coucou": "$argon2id$v=19$m=262144,t=3,p=1$iQSRv\/t+fA46\/TtdKKCqww$zq+kwoHTsDVA\/8cMstQqwgwFOGT998kRWImxevT174k",
        "LexjfvSDK": "$argon2id$v=19$m=262144,t=3,p=1$u\/mmjxOx33WKCu5Sk\/hzNA$bct\/goPD\/3wflDoLlxCn4ag8fGXbyiqNKGkvNhL06YY",
        "password": "$argon2id$v=19$m=262144,t=3,p=1$QGqI4D1p5Q3TsphSG7t6cw$lRnrpaMoSXA5Q\/pU\/1IK545jXYhCLw8BekynLXdIXcw",

remicollet · 2019-07-08T05:38:04Z

Closing as merged

Thanks @sgolemon

From the RFC: > New Constants > > Just in case an application wants to know where their argon2 support is coming from, I'd propose a new constant to be declared by whichever module is providing the support. > > PASSWORD_ARGON2_PROVIDER == 'standard' || 'sodium' Refs: * https://wiki.php.net/rfc/sodium.argon.hash * php/php-src#4012 * php/php-src@0ba1db7

krakjoe added the RFC label Apr 5, 2019

remicollet reviewed Apr 6, 2019

View reviewed changes

ext/sodium/sodium_pwhash.c Show resolved Hide resolved

nikic reviewed Apr 8, 2019

View reviewed changes

sgolemon force-pushed the sodium-argon branch from 0a04de9 to 02f0bf6 Compare June 23, 2019 17:16

Provide argon2i(d) password hashing from sodium when needed

08e634e

sgolemon force-pushed the sodium-argon branch from 02f0bf6 to 08e634e Compare July 5, 2019 15:42

remicollet closed this Jul 8, 2019

jrfnl mentioned this pull request Aug 28, 2019

PHP 7.4/NewConstants: more new PHP 7.4 constants PHPCompatibility/PHPCompatibility#887

Merged

Conversation

sgolemon commented Apr 5, 2019

Uh oh!

remicollet commented Apr 6, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgolemon Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxSem commented Jun 11, 2019

Uh oh!

sgolemon commented Jun 23, 2019

Uh oh!

MaxSem commented Jul 2, 2019

Uh oh!

nikic commented Jul 3, 2019

Uh oh!

remicollet commented Jul 3, 2019

Uh oh!

remicollet commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgolemon commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgolemon commented Jul 5, 2019

Uh oh!

sgolemon commented Jul 5, 2019

Uh oh!

remicollet commented Jul 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

remicollet commented Jul 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sgolemon Apr 9, 2019 •

edited

Loading

remicollet commented Jul 5, 2019 •

edited

Loading

sgolemon commented Jul 5, 2019 •

edited

Loading

remicollet commented Jul 6, 2019 •

edited

Loading