Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.x] According to PHP Bug 78516 Argon2 requires at least 8KB #5097

Merged
merged 1 commit into from Sep 9, 2019

Conversation

@HepplerDotNet
Copy link
Contributor

commented Sep 9, 2019

https://bugs.php.net/bug.php?id=78516
Argon2 requires at least 8KB
On PHP 7.4 memory 1024 will throw:
password_hash(): Memory cost is outside of allowed memory range

According to PHP Bug 78516 Argon2 requires at least 8KB
https://bugs.php.net/bug.php?id=78516
Argon2 requires at least 8KB
On PHP 7.4 memory 1024 will throw:
password_hash(): Memory cost is outside of allowed memory range

@GrahamCampbell GrahamCampbell changed the title According to PHP Bug 78516 Argon2 requires at least 8KB [6.0] According to PHP Bug 78516 Argon2 requires at least 8KB Sep 9, 2019

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

We should maybe fix this on the 5.8 branch too?

@driesvints driesvints changed the title [6.0] According to PHP Bug 78516 Argon2 requires at least 8KB [6.x] According to PHP Bug 78516 Argon2 requires at least 8KB Sep 9, 2019

@HepplerDotNet

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

So, now it's clear!
PHP 7.2 and 7.3 use libargon, while PHP 7.4 uses libsodium.
https://bugs.php.net/bug.php?id=78269#1562751980

This means with PHP 7.2 and 7.3
memory_cost => 8192
will end up in 8MB!
But with PHP 7.4 it will be 8KB

@taylorotwell taylorotwell merged commit 74d84e9 into laravel:master Sep 9, 2019

1 check passed

continuous-integration/styleci/pr The analysis has passed
Details
@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Oh, in which case we should switch on the PHP version tbh. Like, setting 8KB memory cost is not really a strong default.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Laravel should come with strong, secure defaults out of the box.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@tillkruss

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@GrahamCampbell: What should we do about it dropping from 8MB to 8kb in November when we switch? Should this be an upgrade note, or maybe a docs PR?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

We should change the laravel default app to be correct, and not update the docs.

@tillkruss

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

With a PHP version check in the config, or a condition in the hashing manager?

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Oh, I guess, maybe the hashing manager is the best place for this.

@HepplerDotNet

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

Sorry for the back and forth. But it was a bit confusing on the php bug list.
If I understood right, it was indeed a bug in PHP 7.4 and is patched now.
https://bugs.php.net/bug.php?id=78516

So memory_cost = 1024 should be 1MB on ALL PHP Versions from 7.2 - 7.4

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Oh, so this PR should be reverted then?

@HepplerDotNet

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

As I read it, yes. All changes regarding this issue can be reverted because this will be fixed in next PHP 74-RC

taylorotwell added a commit that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.