[RFC]: Add Argon2 to password_* #1997

charlesportwoodii · 2016-07-10T18:56:04Z

RFC: https://wiki.php.net/rfc/argon2_password_hash

PASSWORD_ARGON2_MEMORY_COST PASSWORD_ARGON2_TIME_COST PASSWORD_ARGON2_LANES

Added Windows config.w32 changes Updated constants in php_password.h

Indicating constants as defaults Minor coding standards change

nikic · 2016-07-10T20:08:44Z

Is libargon2 provided by distros? I don't fancy the idea of bundling another library, unless we actually need to patch something in it.

charlesportwoodii · 2016-07-10T22:19:09Z

@nikic,

No, not yet - this is something specifically mentioned in the RFC draft I'm working on.

libargon2 is in Debian stretch, but not stable yet. https://packages.debian.org/search?keywords=libargon2-0. I haven't seen it drop in RH/CentOS yet, hence whyy the necessary C files were added directly. I'm not too fond of including it directly either.

As an alternative to directly including the source (at least until a library drops in distros) to to compile the libargon2 reference library as part of PHP's make process. This is the process used by the extension I wrote and extensions for other languages such as Ruby and Go.

The libargon2 reference library compiles with little dependencies, going down that route however would significantly complicate the PHP build process, which I was hoping to avoid.

Let me know your thoughts. There's ways to make it work without including the C source, but I completely understand tabling a feature to wait for the library to become more wildly available.

jpauli · 2016-07-11T08:17:27Z

I'm 👎 to bundle yet another lib, and to have to maintain it.
This is a pain with libgd, libpcre and other libs we bundle, so please, rely on the OS distro.

Also, ext/standard should not depend on libpthread, as this is not (officialy) supported under Windows.
Please, consider Windows specific issues and provide some solutions if possible.

KalleZ · 2016-07-11T11:26:05Z

ext/standard/config.w32

 	'/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1');
 	PHP_INSTALL_HEADERS("", "ext/standard");
 if (PHP_MBREGEX != "no") {
 	CHECK_HEADER_ADD_INCLUDE("oniguruma.h", "CFLAGS_STANDARD", PHP_MBREGEX + ";ext\\mbstring\\oniguruma")   
 }
 PHP_INSTALL_HEADERS("", "ext/standard");
+PHP_INSTALL_HEADERS([ext/standard/argon2lib])


This should be PHP_INSTALL_HEADERS("", "ext/standard/argon2lib")

KalleZ · 2016-07-11T11:27:57Z

I agree with @jpauli here, bundling yet another library and the added Windows issues it causes is a huge -1, especially in ext/standard

nikic · 2016-07-11T11:46:06Z

On Jul 11, 2016 00:19, "Charles R. Portwood II" [email protected]
wrote:

@nikic,

No, not yet - this is something specifically mentioned in the RFC draft
I'm working on.

libargon2 is in Debian stretch, but not stable yet.
https://packages.debian.org/search?keywords=libargon2-0. I haven't seen it
drop in RH/CentOS yet, hence by the necessary C files were added directly.
I'm not too fond of including it directly either.

As an alternative to directly including the source (at least until a
library drops in distros) to to compile the libargon2 reference library as
part of PHP's make process. This is the process used by the extension I
wrote and extensions for other languages such as Ruby and Go.

The libargon2 reference library compiles with little dependencies, going
down that route however would significantly complicate the PHP build
process, which I was hoping to avoid.

Let me know your thoughts. There's ways to make it work without including
the C source, but I completely understand tabling a feature to wait for the
library to become more wildly available.

I would suggest to add libargon2 as an optional dependency for now, which
we link against if something like --with-argon2 is configured. This will
make the feature available to the people who need it in a convenient way.
If libargon2 becomes widely available, we can make it a default dependency,
though that is probably some years off.

From my limited understanding, there is no pressing need to replace bcrypt
with argon2 right now, argon2 is just slightly preferable. As such I thing
it's fine to only provide it optionally to start with.

jpauli · 2016-07-11T11:49:08Z

I totally agree with @nikic here.

charlesportwoodii · 2016-07-11T17:32:06Z

Great, thanks for your feedback everyone.

I can definitely change this up to use a config flag and check for the library that way. Dynamic linking should resolve the issues surrounding pthread.

From my limited understanding, there is no pressing need to replace bcrypt
with argon2 right now, argon2 is just slightly preferable. As such I thing
it's fine to only provide it optionally to start with.

You're correct. Bcrypt is still a perfectly valid and secure hashing algorithm. Argon2 is just another option.

- Configure flag now accepts --with-argon2 for dynamic linking with libargon2. Argon2 will be enabled in password_* only if this flag is passed. - --with-argon2 config flag allows user passed directory for linking - Added Argon2 specific tests to ensure existing tests do not fail when argon2 is disable

- Added "Skipped:" flag so argon2 tests would be skipped when PHP is compiled without Argon2 support

KalleZ · 2016-07-12T12:13:51Z

ext/standard/config.w32

+	if (CHECK_LIB("Argon2Ref.lib", "argon2", PHP_ARGON2)
+	&& CHECK_HEADER_ADD_INCLUDE("argon2.h", "CFLAGS_ARGON2")) {
+		AC_DEFINE('HAVE_ARGON2LIB', 1);
+		EXTENSION("argon2", null, false);


This doesn't look right, simply drop the call to EXTENSION() here and leave the AC_DEFINE().

I was getting linker errors without the call to EXTENSION. Is there another macro I can use to get Argon2Ref.lib to be added to the linker? I wasn't seeing any other function for handling that.

Hi! It's been a while since I was tinkering around with this, but from vague memory this is how it works:

CHECK_LIB':

The second parameter is the target of where to link the Argon2Ref.lib to (as per extension), this can be set to empty ("") -- See win32\build\confutils.js

The third parameter is fine (as it uses PHP_ARGON2, which is the one from the --with-argon2=XXX)

CHECK_HEADER_ADD_INCLUDE

This sets the compiler flags for an extension (CFLAGS_ARGON2), and should simply be CFLAGS (we use this value for core things, like determining IPv6 support in win32\build\config.js).

TL;DR:
Line 7 should be: if (CHECK_LIB("Argon2Ref.lib", "", PHP_ARGON2)
Line 8 should be: && CHECK_HEADER_ADD_INCLUDE("argon2.h", "CFLAGS")) {

which should allow you to drop the EXTENSION() call.

(bear in mind I'm rusty, but else poke around the sources in win32\build)!

Hi! Thanks for pointing me in the right direction! I really appreciate it.

It looks like the trick is to set the second arguement of CHECK_LIB (target) to null instead of "". An empty string creates LIBS_ in the Makefile, whereas null correctly assigns it to LIBS.

Thanks again!

- Added Windows config script for building on Windows - Updated tests - Renamed constants from PASSWORD_ARGON* to ARGON*_PASSWORD to avoid potential conflicts with php/php-src#1997 - Bumping version for API change

nikic · 2016-07-18T16:59:26Z

ext/standard/password.c

@@ -61,7 +81,14 @@ static php_password_algo php_password_determine_algo(const char *hash, const siz
 {
 	if (len > 3 && hash[0] == '$' && hash[1] == '2' && hash[2] == 'y' && len == 60) {
 		return PHP_PASSWORD_BCRYPT;
+	} 
+#if HAVE_ARGON2LIB
+	if (hash[0] == '$' && strstr(hash, "argon2i")) {


This will check whether argon2i is contained anywhere in the string. Maybe doing something more precise like memcmp(hash + 1, "argon2i", sizeof("argon2i") - 1), with appropriate length check beforehand, would be preferable?

Because of the memory, time, and parallelism factors, Argon2 hashes can be of variable length. Using the default constants listed in php_password.h, this string will be 99 characters long, however the change of constants would result in different sized hashes.

The other alternative is to try to parse out all the available Argon2 options on the spot - there's still a chance that the string isn't an Argon2 hash though even verifying those options. At the end of the day the hash, will be password to password_verify. If the string isn't a valid Argon2 hash it'll be detected there and the password_verify will return false.

Thoughts?

The comment about the length check was only to avoid out of bounds memcmp, not ensure a particular value. I.e. something along the lines of:

if (hash_len >= sizeof("$argon2i$")-1 && !memcmp(hash, "$argon2i$", sizeof("$argon2i$")-1)) { return PHP_PASSWORD_ARGON2I; } // ...

You're right that it probably doesn't matter anyway. The only problem I can imagine is the string "argon2i" occurring in the salt of a non-bcrypt non-argon hash (iirc password_verify also accepts crypt hashes that password_hash cannot generate). But this is probably pretty far fetched ^^

I don't think it matters since bcrypt and crypt hashes are checked before hand, but I went ahead and converted it to your suggestion anyways. Your solution makes this a little more portable if new algorithms are added.

nikic · 2016-07-18T17:11:31Z

The implementation looks good to me (at least the C part, no idea about autoconf), nice work!

Using zend_string_alloc instead of char* for out and encoded variables

charlesportwoodii · 2016-07-18T18:18:51Z

@nikic Thanks, and thanks for the code review!

Argon2d is not suitable for password_hashing. To ensure best practices within password_*, Argon2d was removed. --with-argon2 implies the full feature set of Argon2, whereas this feature only implements Argon2i within password_*. Consequently the feature flag was renamed to --with-password-argon2

…SWORD_ARGON2

- Updating tests - Adjusting cost factors: - memory_cost = 1 MiB - time_cost = 2 - threads = 2

dkulyk · 2016-08-27T23:39:41Z

ext/standard/tests/password/password_verify_argon2.phpt

+Test normal operation of password_verify() with argon2
+--SKIPIF--
+<?php
+if (!defined('PASSWORD_ARGON2I')) die('Skipped: password_get_info not built with Argon2');


password_get_info not built with Argon2 in password_verify test

cmb69 · 2016-09-08T17:05:31Z

@charlesportwoodii ext/standard/tests/password/password_needs_rehash_argon2.phpt is failing for me on Windows x64 (I haven't checked other platforms yet) with the following diff:

001+ bool(true)
001- bool(false)

The other tests are running fine.

charlesportwoodii · 2016-09-08T17:12:00Z

@cmb69,

I'm pretty sure the test file is the problem. Just let me verify that is the case and I'll have a patch up for you momentarily. I'll also provide a separate PR for the upgrading docs since this was forked from 7.1 instead of 7.2.

charlesportwoodii · 2016-09-08T17:31:44Z

@cmb69,

Yeah, it's just the test file that needs to be updated after the constants were changed. The following diff should fix it. Git must've not picked up my last change set for this. Sorry for the trouble.

$ TEST_PHP_EXECUTABLE=./sapi/cli/php ./run-tests.php ext/standard/tests/password
=====================================================================
PHP         : ./sapi/cli/php 
PHP_SAPI    : cli
PHP_VERSION : 7.1.0-dev
ZEND_VERSION: 3.1.0-dev
PHP_OS      : Linux - Linux 4.4.0-36-generic #55-Ubuntu SMP Thu Aug 11 18:01:55 UTC 2016 x86_64
INI actual  : /home//Public/php-src
More .INIs  :   
CWD         : /home//Public/php-src
Extra dirs  : 
VALGRIND    : Not used
=====================================================================
Running selected tests.
PASS Test error operation of password_hash() with bcrypt hashing [ext/standard/tests/password/password_bcrypt_errors.phpt] 
PASS Test deprecated operation of password_hash() [ext/standard/tests/password/password_deprecated_salts.phpt] 
PASS Test normal operation of password_get_info() [ext/standard/tests/password/password_get_info.phpt] 
PASS Test normal operation of password_get_info() with Argon2 [ext/standard/tests/password/password_get_info_argon2.phpt] 
PASS Test error operation of password_get_info() [ext/standard/tests/password/password_get_info_error.phpt] 
PASS Test normal operation of password_hash() [ext/standard/tests/password/password_hash.phpt] 
PASS Test normal operation of password_hash() with argon2 [ext/standard/tests/password/password_hash_argon2.phpt] 
PASS Test error operation of password_hash() [ext/standard/tests/password/password_hash_error.phpt] 
PASS Test error operation of password_hash() with argon2 [ext/standard/tests/password/password_hash_error_argon2.phpt] 
PASS Test normal operation of password_needs_rehash() [ext/standard/tests/password/password_needs_rehash.phpt] 
PASS Test normal operation of password_needs_rehash() with argon2 [ext/standard/tests/password/password_needs_rehash_argon2.phpt] 
PASS Test error operation of password_needs_rehash() [ext/standard/tests/password/password_needs_rehash_error.phpt] 
PASS Test normal operation of password_verify) [ext/standard/tests/password/password_verify.phpt] 
PASS Test normal operation of password_verify() with argon2 [ext/standard/tests/password/password_verify_argon2.phpt] 
PASS Test error operation of password_verify() [ext/standard/tests/password/password_verify_error.phpt] 
=====================================================================
Number of tests :   15                15
Tests skipped   :    0 (  0.0%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    0 (  0.0%) (  0.0%)
Expected fail   :    0 (  0.0%) (  0.0%)
Tests passed    :   15 (100.0%) (100.0%)
---------------------------------------------------------------------
Time taken      :    2 seconds
=====================================================================

The patch is listed below.

diff --git a/ext/standard/tests/password/password_needs_rehash_argon2.phpt b/ext/standard/tests/password/password_needs_rehash_argon2.phpt
index 1fec77d..88b7c4b 100644
--- a/ext/standard/tests/password/password_needs_rehash_argon2.phpt
+++ b/ext/standard/tests/password/password_needs_rehash_argon2.phpt
@@ -7,11 +7,11 @@ if (!defined('PASSWORD_ARGON2I')) die('Skipped: password_needs_rehash not built
 --FILE--
 <?php

-$hash = '$argon2i$v=19$m=65536,t=3,p=1$YkprUktYN0lHQTd2bWRFeA$79aA+6IvgclpDAJVoezProlqzIPy7do/P0sBDXS9Nn0';
+$hash = password_hash('test', PASSWORD_ARGON2I);
 var_dump(password_needs_rehash($hash, PASSWORD_ARGON2I));
 var_dump(password_needs_rehash($hash, PASSWORD_ARGON2I, ['memory_cost' => 1<<17]));
-var_dump(password_needs_rehash($hash, PASSWORD_ARGON2I, ['time_cost' => 2]));
-var_dump(password_needs_rehash($hash, PASSWORD_ARGON2I, ['threads' => 2]));
+var_dump(password_needs_rehash($hash, PASSWORD_ARGON2I, ['time_cost' => 4]));
+var_dump(password_needs_rehash($hash, PASSWORD_ARGON2I, ['threads' => 4]));
 echo "OK!";
 ?>
 --EXPECT--

For the UPGRADING I'll just provide an extra diff here instead of a new PR.

charlesportwoodii · 2016-09-08T17:41:27Z

@cmb69,

And a patch for the UPGRADING docs. Let me know if you're looking for more, or if I am missing anything.

diff --git a/UPGRADING b/UPGRADING
index d61280a..d15c4c9 100644
--- a/UPGRADING
+++ b/UPGRADING
@@ -32,6 +32,10 @@ PHP 7.2 UPGRADE NOTES
 - PCRE:
   . Added `J` modifier for setting PCRE_DUPNAMES.

+- Standard:
+  . Simplified password hashing API updated to support Argon2i hashes when PHP is compiled with libargon2
+    (https://wiki.php.net/rfc/argon2_password_hash).
+
 ========================================
 3. Changes in SAPI modules
 ========================================
@@ -44,6 +48,14 @@ PHP 7.2 UPGRADE NOTES
 5. Changed Functions
 ========================================

+- Standard:
+  . password_hash() can generate Argon2i hashes when the algorithm is set to PASSWORD_ARGON2I. 
+    When using PASSWORD_ARGON2I, the following cost factors may be set: 'memory_cost', 'time_cost', 
+    and 'threads'. These cost factors will default to 'PASSWORD_ARGON2_DEFAULT_MEMORY_COST', 
+    'PASSWORD_ARGON2_DEFAULT_TIME_COST', and 'PASSWORD_ARGON2_DEFAULT_THREADS' respectively if not set.
+  . password_verify() can verify Argon2i hashes.
+  . password_get_info() and password_needs_rehash() can accept Argon2i hashes.
+
 ========================================
 6. New Functions
 ========================================
@@ -82,6 +94,12 @@ PHP 7.2 UPGRADE NOTES
 10. New Global Constants
 ========================================

+- Standard:
+  . PASSWORD_ARGON2_DEFAULT_MEMORY_COST
+  . PASSWORD_ARGON2_DEFAULT_TIME_COST
+  . PASSWORD_ARGON2_DEFAULT_THREADS
+  . PASSWORD_ARGON2I
+
 ========================================
 11. Changes to INI File Handling
 ========================================

cmb69 · 2016-09-08T17:55:06Z

Thanks, that looks fine! I'm going to commit.

koenhoeijmakers · 2017-07-25T12:37:31Z

So i know i'm late to the party, But couldn't libsodium (which will be core) handle the Argon2 hashing? (in which case the argument of "not willing to bundle another lib" becomes obsolete), Or does libsodium also rely on libargon2?

Just curious and speculating ofcourse, but couldn't really find an answer to this, enlight me! :)

denji · 2017-07-25T13:09:15Z

Hi, what about support for the Argon2id (Argon2i + Argon2d)? https://github.com/P-H-C/phc-winner-argon2#argon2

charlesportwoodii · 2017-07-25T13:51:57Z

@koenhoeijmakers

The bundling issue was resolved by using the libargon2 shared library 20161029, so that's no longer a concern. While sodium-ext is now in core, it's still dependent upon the libsodium library.

This was merged in nearly a year before sodium-ext was able to be merged in. The driving factors at the time were: not wanting to link this RFC to the Sodium RFC and have a bunch of last minute changes going on, and wanting clear separation of concerns between what Sodium does and the functionality available in password_hash/verify().

While I agree there could be some efficiencies gained by using crypto_pwhash() function from libsodium instead of directly calling the libargon2 functions, that would link password_hash/verify() to sodium-ext, and obscure what --with-sodium and --with-password-argon2 does. While I think most people would agree having these two items completely separated is a good idea, this is honestly a larger mailing list discussion.

@denji

I believe 7.2 is feature frozen, and the implemented version of libargon2 (20161029) didn't include Argon2id at the time. While #2582 enables this as a future possibility, it would require a mailing list discussion and a RFC to add a PASSWORD_ARGON2ID constant and implement that as another algorithm choice within password_hash/verify().

denji · 2017-07-29T17:01:37Z

https://github.com/yandex/argon2#benchmark-results

charlesportwoodii added 6 commits July 8, 2016 15:27

Implementing password_verify and password_get_info for Argon2

de85c2e

Introducing Argon2 memory, time, and lanes constants

3c7fb71

PASSWORD_ARGON2_MEMORY_COST PASSWORD_ARGON2_TIME_COST PASSWORD_ARGON2_LANES

Working implementation with password_hash, password_verify

c2551a7

Adding test cases for Argon2i and Argon2d

0a1274f

Added Windows config.w32 changes Updated constants in php_password.h

Fixing spacing in php_password.h

deba2b4

Reverting PASSWORD_DEFAULT to PASSWORD_BCRYPT

1bc3818

Indicating constants as defaults Minor coding standards change

jpauli added the RFC label Jul 11, 2016

KalleZ reviewed Jul 11, 2016
View reviewed changes

charlesportwoodii added 4 commits July 11, 2016 16:44

Fixing failing tests for Argon2

9f37be5

- Added "Skipped:" flag so argon2 tests would be skipped when PHP is compiled without Argon2 support

Adding Windows build flag for --with-argon2

d398657

Fixing linker issue on linux when DIR is specified on --with-argon2

f4aa3a4

KalleZ reviewed Jul 12, 2016
View reviewed changes

charlesportwoodii added 3 commits July 12, 2016 10:24

Adding to PHP library includes

9bedcb7

Untouching old tests

1c954c9

Updating config.w32 to correctly add Argon2RefLib to LIBS flag

9872208

nikic reviewed Jul 18, 2016
View reviewed changes

Fixing potential memory leak with encoded in password_hash

ab837a6

Using zend_string_alloc instead of char* for out and encoded variables

charlesportwoodii added 3 commits August 1, 2016 18:53

Fixing issue with config.m4 script not correctly checking for PHP_PAS…

d883f65

…SWORD_ARGON2

Changing m_cost and t_cost to memory_cost and time_cost

0e3b3b0

- Updating tests - Adjusting cost factors: - memory_cost = 1 MiB - time_cost = 2 - threads = 2

dkulyk reviewed Aug 27, 2016
View reviewed changes

Fixing typo in tests

35a74b9

php-pulls merged commit 35a74b9 into php:master Sep 8, 2016

denji mentioned this pull request May 15, 2017

Add libsodium extension facebook/hhvm#7827

Closed

charlesportwoodii mentioned this pull request Jun 18, 2017

Compatibility with libargon2 versions 20161029 and 20160821 #2582

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC]: Add Argon2 to password_* #1997

[RFC]: Add Argon2 to password_* #1997

charlesportwoodii commented Jul 10, 2016

nikic commented Jul 10, 2016

charlesportwoodii commented Jul 10, 2016 •

edited

Loading

jpauli commented Jul 11, 2016

KalleZ Jul 11, 2016

KalleZ commented Jul 11, 2016

nikic commented Jul 11, 2016 •

edited

Loading

jpauli commented Jul 11, 2016

charlesportwoodii commented Jul 11, 2016

KalleZ Jul 12, 2016

charlesportwoodii Jul 12, 2016 •

edited

Loading

KalleZ Jul 13, 2016

charlesportwoodii Jul 13, 2016

nikic Jul 18, 2016

charlesportwoodii Jul 18, 2016

nikic Jul 18, 2016

charlesportwoodii Jul 18, 2016

nikic commented Jul 18, 2016

charlesportwoodii commented Jul 18, 2016

dkulyk Aug 27, 2016

cmb69 commented Sep 8, 2016

charlesportwoodii commented Sep 8, 2016

charlesportwoodii commented Sep 8, 2016 •

edited

Loading

charlesportwoodii commented Sep 8, 2016 •

edited

Loading

cmb69 commented Sep 8, 2016

koenhoeijmakers commented Jul 25, 2017 •

edited

Loading

denji commented Jul 25, 2017 •

edited

Loading

charlesportwoodii commented Jul 25, 2017 •

edited

Loading

denji commented Jul 29, 2017

[RFC]: Add Argon2 to password_* #1997

[RFC]: Add Argon2 to password_* #1997

Conversation

charlesportwoodii commented Jul 10, 2016

nikic commented Jul 10, 2016

charlesportwoodii commented Jul 10, 2016 • edited Loading

jpauli commented Jul 11, 2016

Choose a reason for hiding this comment

KalleZ commented Jul 11, 2016

nikic commented Jul 11, 2016 • edited Loading

jpauli commented Jul 11, 2016

charlesportwoodii commented Jul 11, 2016

Choose a reason for hiding this comment

charlesportwoodii Jul 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic commented Jul 18, 2016

charlesportwoodii commented Jul 18, 2016

Choose a reason for hiding this comment

cmb69 commented Sep 8, 2016

charlesportwoodii commented Sep 8, 2016

charlesportwoodii commented Sep 8, 2016 • edited Loading

charlesportwoodii commented Sep 8, 2016 • edited Loading

cmb69 commented Sep 8, 2016

koenhoeijmakers commented Jul 25, 2017 • edited Loading

denji commented Jul 25, 2017 • edited Loading

charlesportwoodii commented Jul 25, 2017 • edited Loading

denji commented Jul 29, 2017

charlesportwoodii commented Jul 10, 2016 •

edited

Loading

nikic commented Jul 11, 2016 •

edited

Loading

charlesportwoodii Jul 12, 2016 •

edited

Loading

charlesportwoodii commented Sep 8, 2016 •

edited

Loading

charlesportwoodii commented Sep 8, 2016 •

edited

Loading

koenhoeijmakers commented Jul 25, 2017 •

edited

Loading

denji commented Jul 25, 2017 •

edited

Loading

charlesportwoodii commented Jul 25, 2017 •

edited

Loading