Remove hash usage from session ID creation #1850

yohgaki · 2016-04-06T05:25:22Z

This is PR for "Session ID without hashing" RFC.
https://wiki.php.net/rfc/session-id-without-hashing

…n-without-hashing

weltling · 2016-05-02T16:29:20Z

Yasuo, seems you include the stuff planned in #1769, but also some more as per RFC. IMHO replacing the current parts with php_random_bytes() were already a good step into the direction, to unify the randomness source for all platforms. I guess this substep wouldn't even require an RFC.

Thanks.

yohgaki · 2016-05-03T08:54:36Z

I agree. php_random_bytes() should be used regardless of RFC vote result. I'll make sure it will be in PHP 7.1.

Danack · 2016-07-04T23:55:56Z

ext/session/session.c

-
-		return SUCCESS;
-	}
+	php_error_docref(NULL, E_WARNING, "session.configuration 'session.sid_length' must between 32 and 256.");


Is this a typo? The check above is val >= 22 && val <= PS_MAX_SID_LENGTH - so the minimum length is 22 not 32?

Thanks. Newer restriction should be 22!

Sobak · 2016-07-12T06:27:06Z

ext/session/session.c


-		php_error_docref(NULL, E_WARNING, "The ini setting hash_bits_per_character is out of range (should be 4, 5, or 6) - using 4 for now");
+	/* Read additiona PS_EXTRA_RAND_BYTES just in case CSPRNG is not safe enough */


There is a typo - should be "additional"

Thanks. I'll fix this!

session.sid_length = 32 session.sid_bits_per_charactor = 4 However, php.ini-development/production had session.hash_func = 0 (MD5 128) session.hash_bits_per_character = 5 <--- This differs from compiled default. I didn't notice this. This config results in shorter session ID length. (26 chars) Anyway, most PHP installation would have session ID length = 32. So normal(?) users who are using php.ini-production as php.ini would get - session.sid_length=32 - session.sid_bits_per_character=5 and bits in session ID is 160 bits. (Older config was 128 bits)

staabm · 2016-08-04T08:43:40Z

UPGRADING

+     . session.sid_bits_per_character (Bits used per byte. 4 to 6. Default: 4)
+  . If you were using session.hash_func=1(SHA1) and session.hash_bits_per_charactor=6
+    Use following INIs to achive the same or better session ID strength.
+     . session.sid_length=32


could you note how one needs to calculate the new settings based on the old ones?

yohgaki · 2016-08-12T03:34:50Z

merged.

Tobion · 2016-12-03T01:08:35Z

php.ini-production

@@ -1457,7 +1443,7 @@ session.hash_function = 0
 ; Development Value: 5
 ; Production Value: 5
 ; http://php.net/session.hash-bits-per-character


The link needs to get updated, but the shortcut http://php.net/session.sid-bits-per-character doesn't seem to exist yet.

Tobion · 2016-12-03T01:10:50Z

php.ini-production

+; Set session ID character length. This value could be between 22 to 256.
+; Shorter length than default is supported only for compatibility reason.
+; Users should use 32 or more chars.
+; http://php.net/session.sid_length


This URL doesn't exist and shouldn't it use a dash like all the other links?

Tobion · 2016-12-03T01:42:12Z

ext/session/session.c

-			break;
-#endif /* HAVE_HASH_EXT */
+	/* Read additional PS_EXTRA_RAND_BYTES just in case CSPRNG is not safe enough */
+	if (php_random_bytes_throw(rbuf, PS(sid_length) + PS_EXTRA_RAND_BYTES) == FAILURE) {


It seems strange to me that it needs to read so many random bytes. In theory it should only require to read sid_length * sid_bits_per_character / 8 bytes. All the extra bytes read seem wasted. Could you explain why this is done this way?

yohgaki · 2016-12-03T05:05:10Z

@Tobion
Strictly speaking extra bytes are not needed. Extra bytes read from PRNG is intentional. It's to avoid exposing PRNG state as precaution.

php.ini-* and ini.list.xml is updated now.
Thank you for reporting.

Tobion · 2016-12-03T12:29:53Z

@yohgaki I've read the RFC and the discussion on the mailing list and I don't see how reading extra bytes should "mitigate risks of broken PRNG". If the random bytes are predictable, then reading 60 extra bytes does not make them less predictable. So what's the point?

yohgaki · 2016-12-03T20:43:49Z

@Tobion
Precaution for broken/predictable/weak PRNG is the whole point.
During RFC discussion, there are "worries" about PRNG state exposure. IMO, ignoring some bits (new bin_to_readable() uses at most 6 bits for PRNG generated byte) for each byte is good enough for hiding PRNG state, but extra 60 bytes are read in addition. There wouldn't be visible performance impact, so don't worry about it. It's faster than older code anyway.

BTW, magic number "60" is chosen because I would like to read more than 64 bytes addition data from PRNG. i.e. When sid_bits_per_character is 6 and sid_length is 22, (60 bytes + 5.5 bytes(22 bits * 2)) > 64 bytes.

Tobion · 2016-12-03T23:03:25Z

When sid_bits_per_character is 6 and sid_length is 22, (60 bytes + 5.5 bytes(22 bits * 2)) > 64 bytes.

As far as I see, you are reading 60 + 22 random bytes then. So I cannot follow your logic.

yohgaki · 2016-12-04T01:34:48Z

@Tobion

As far as I see, you are reading 60 + 22 random bytes then. So I cannot follow your logic.

It's 65.5 bytes (480 bits + 44 bits), not 88 bytes for that case.

Anyway, I don't insist to have "extra 60 bytes". It just the code proposed with the RFC and extra bytes are added according to RFC discussion with Stas. (Stas worried about PRNG state exposure)

I don't mind removing extra PRNG reads at all indeed. It may hurt performance on some architectures that have slow PRNG generation rate. New code does not use full bits from PRNG and it would be good enough mitigation. IMO.

Tobion · 2016-12-04T06:36:41Z

UPGRADING

+    Length of old session ID string is determined as follows
+     . Used hash function's bits.
+        . session.hash_function=0 - MD5 128 bits (This was default)
+        . session.hash_function=1 - SHA1 192 bits


sha1 is 160 bits. fixed in #2228

Tobion · 2016-12-04T06:39:13Z

ext/session/session.c

@@ -300,151 +295,24 @@ static char *bin_to_readable(char *in, size_t inlen, char *out, char nbits) /* {
 	}

 	*out = '\0';
-	return out;
+	return len;


why does the method return the length when it is constant? Seems useless.

Tobion · 2016-12-04T07:18:08Z

It's 65.5 bytes (480 bits + 44 bits), not 88 bytes for that case.

@yohgaki this php_random_bytes_throw(rbuf, PS(sid_length) + PS_EXTRA_RAND_BYTES) reads 88 random bytes. I don't see how you can end up with 65.5 bytes there...

yohgaki · 2016-12-04T07:22:27Z

@Tobion
I'm lost.
We are talking about "extra bytes(bits) read", aren't we?

Anyway, the code reads more than enough extra bytes from PRNG to avoid exposing PRNG state. If you would like to remove it, write RFC for it. I wouldn't oppose it.

Yasuo Ohgaki added 3 commits April 6, 2016 14:13

Remove hash usage from session ID creation

8de5200

Remove unneeded tests

55ca341

Merge remote-tracking branch 'upstream/master' into master-rfc-sessio…

62ff734

…n-without-hashing

Yasuo Ohgaki added 2 commits July 2, 2016 09:01

Merge branch 'master' into master-rfc-session-without-hashing

acfc084

Read extra 60 bytes. Change min sid len

cc328ff

laruence added the RFC label Jul 2, 2016

use_strict_mode will be changed by other RFC

1c369b7

Danack reviewed Jul 4, 2016
View reviewed changes

Fix possible overflow

9faf042

Sobak reviewed Jul 12, 2016
View reviewed changes

Yasuo Ohgaki added 11 commits July 12, 2016 18:08

Fix typo and error message

c6fb46f

Remove unneeded hash enum

399720e

Merge branch 'master' into master-rfc-session-without-hashing

65e0d5a

Update UPGRADING

79fbe49

Fix error message

106ee4d

Merge branch 'master' into master-rfc-session-without-hashing

9a40960

Add test. Relax validtion

4742cce

Use macro

49acf57

Merge branch 'master' into master-rfc-session-without-hashing

37a332e

Use compatible defaults in php.ini-*

13a26e7

staabm reviewed Aug 4, 2016
View reviewed changes

Yasuo Ohgaki added 2 commits August 9, 2016 19:23

Update UPGRADING

ab5b430

Merge branch 'master' into master-rfc-session-without-hashing

228eb6a

yohgaki closed this Aug 12, 2016

Tobion reviewed Dec 3, 2016

View reviewed changes

Tobion reviewed Dec 4, 2016

View reviewed changes


		php_error_docref(NULL, E_WARNING, "The ini setting hash_bits_per_character is out of range (should be 4, 5, or 6) - using 4 for now");
		/* Read additiona PS_EXTRA_RAND_BYTES just in case CSPRNG is not safe enough */

Remove hash usage from session ID creation #1850

Remove hash usage from session ID creation #1850

Uh oh!

Conversation

yohgaki commented Apr 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weltling commented May 2, 2016

Uh oh!

yohgaki commented May 3, 2016

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!

yohgaki commented Aug 12, 2016

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!

yohgaki commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tobion commented Dec 3, 2016

Uh oh!

yohgaki commented Dec 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tobion commented Dec 3, 2016

Uh oh!

yohgaki commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yohgaki commented Dec 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yohgaki commented Apr 6, 2016 •

edited

Loading

yohgaki commented Dec 3, 2016 •

edited

Loading

yohgaki commented Dec 3, 2016 •

edited

Loading

yohgaki commented Dec 4, 2016 •

edited

Loading

Tobion commented Dec 4, 2016 •

edited

Loading

yohgaki commented Dec 4, 2016 •

edited

Loading