Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC][VOTE] Session ID without hashing »

[RFC][VOTE] Session ID without hashing

php.internals

Yasuo Ohgaki

10 years ago
Hi all, Currently session module uses obsolete MD5 for session ID. With CSPRNG, hashing is redundant and needless. It adds hash module dependency and inefficient (There is no reason to use hash for CSPRNG generated bytes). This proposal cleans up session code by removing hash. https://wiki.php.net/rfc/session-id-without-hashing I set vote requires 2/3 support. Please describe the reason why when you against this RFC. Reasons are important for improvements! Thank you!
-- Yasuo Ohgaki yohgaki@ohgaki.net

Leigh

10 years ago
On Sat, 2 Jul 2016 at 08:36 Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi all, > > Currently session module uses obsolete MD5 for session ID. With > CSPRNG, hashing is redundant and needless. It adds hash module > dependency and inefficient (There is no reason to use hash for CSPRNG > generated bytes). > > This proposal cleans up session code by removing hash. > > https://wiki.php.net/rfc/session-id-without-hashing > > I set vote requires 2/3 support. > Please describe the reason why when you against this RFC. Reasons are > important for improvements! >
I support the idea proposed here, but I have issues with the implementation. You've changed a lot of defaults that may break things for some users (especially those using custom session handlers). I'll add some notes on github.

Leigh

10 years ago
Actually decided to post so On Sat, 2 Jul 2016 at 09:16 Leigh <leight@gmail.com> wrote:
> On Sat, 2 Jul 2016 at 08:36 Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > >> Hi all, >> >> Currently session module uses obsolete MD5 for session ID. With >> CSPRNG, hashing is redundant and needless. It adds hash module >> dependency and inefficient (There is no reason to use hash for CSPRNG >> generated bytes). >> >> This proposal cleans up session code by removing hash. >> >> https://wiki.php.net/rfc/session-id-without-hashing >> >> I set vote requires 2/3 support. >> Please describe the reason why when you against this RFC. Reasons are >> important for improvements! >> > >
So I have a few issues that span the RFC and the implementation. Your RFC states
> hardcoded default and php.ini-* default values are the same.
This is not the case. Originally the session id length and character set were controlled by session.hash_function and/or session.hash_bits_per_character. These customisations to configuration will be lost when the user upgrades. You have provided a mechanism to control length and charset, but it will require new changes to the default settings. This needs to be noted as a breaking change. Your default for session.sid_length is 48. Up to 7.1 the session id length is 32. Your default for session.sid_bits_per_character is 5, up to 7.1 the session id uses 4 bits per character. This is a breaking change. (Imagine custom session handlers that validate session id character sets, or database schemas that will truncate after 32 characters) Your patch updates session.use_strict_mode from 0 to 1. I actually don't know what this changes, but it's an undocumented change. Overall your patch looks very similar to the one I was working on earlier in the year, although you appear to have deleted a bunch of tests that you could have just updated. You should probably put those back, and update them.

Markus Fischer

10 years ago
Everytime I see a thread mentioning session.use_strict_mode I'm wondering why we haven't got around to enable it by default (by means of php.ini-development/php.ini-production ). Maybe someone can step forward and propose this change for the next version (not 7.1 ...)? It could be documented as a breaking change, refer to the documentation and finally call it a day :-) - Markus On 02.07.16 10:39, Leigh wrote:

Yasuo Ohgaki

10 years ago
Hi Markus, On Sat, Jul 2, 2016 at 6:06 PM, Markus Fischer <markus@fischer.name> wrote:
> Everytime I see a thread mentioning session.use_strict_mode I'm > wondering why we haven't got around to enable it by default (by means of > php.ini-development/php.ini-production ). > > Maybe someone can step forward and propose this change for the next > version (not 7.1 ...)? > > It could be documented as a breaking change, refer to the documentation > and finally call it a day :-) > > - Markus
Thank you for reminder. I'll create new RFC only for this soon. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi Leigh, On Sat, Jul 2, 2016 at 5:39 PM, Leigh <leight@gmail.com> wrote:
> So I have a few issues that span the RFC and the implementation. > > Your RFC states > >> hardcoded default and php.ini-* default values are the same. > > This is not the case. > > Originally the session id length and character set were controlled by > session.hash_function and/or session.hash_bits_per_character. These > customisations to configuration will be lost when the user upgrades. You > have provided a mechanism to control length and charset, but it will require > new changes to the default settings. This needs to be noted as a breaking > change. > > Your default for session.sid_length is 48. Up to 7.1 the session id length > is 32. Your default for session.sid_bits_per_character is 5, up to 7.1 the > session id uses 4 bits per character. This is a breaking change. (Imagine > custom session handlers that validate session id character sets, or database > schemas that will truncate after 32 characters)
I'll update relevant part.
> Your patch updates session.use_strict_mode from 0 to 1. I actually don't > know what this changes, but it's an undocumented change.
This is unintentional. I'll remove this part.
> Overall your patch looks very similar to the one I was working on earlier in > the year, although you appear to have deleted a bunch of tests that you > could have just updated. You should probably put those back, and update > them.
It removes hashing, so irrelevant tests are simply removed. Thank you for point them out. I'll fix them now. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Pierre Joye

10 years ago
On Jul 3, 2016 7:04 AM, "Yasuo Ohgaki" <yohgaki@ohgaki.net> wrote:
> > Hi Leigh, > > On Sat, Jul 2, 2016 at 5:39 PM, Leigh <leight@gmail.com> wrote: > > So I have a few issues that span the RFC and the implementation. > > > > Your RFC states > > > >> hardcoded default and php.ini-* default values are the same. > > > > This is not the case. > > > > Originally the session id length and character set were controlled by > > session.hash_function and/or session.hash_bits_per_character. These > > customisations to configuration will be lost when the user upgrades. You > > have provided a mechanism to control length and charset, but it will
require
> > new changes to the default settings. This needs to be noted as a
breaking
> > change. > > > > Your default for session.sid_length is 48. Up to 7.1 the session id
length
> > is 32. Your default for session.sid_bits_per_character is 5, up to 7.1
the
> > session id uses 4 bits per character. This is a breaking change.
(Imagine
> > custom session handlers that validate session id character sets, or
database
> > schemas that will truncate after 32 characters) > > I'll update relevant part. > > > Your patch updates session.use_strict_mode from 0 to 1. I actually don't > > know what this changes, but it's an undocumented change. > > This is unintentional. I'll remove this part. > > > Overall your patch looks very similar to the one I was working on
earlier in
> > the year, although you appear to have deleted a bunch of tests that you > > could have just updated. You should probably put those back, and update > > them. > > It removes hashing, so irrelevant tests are simply removed. > > Thank you for point them out. > I'll fix them now.
Restart vote too please. Thanks Pierre

Yasuo Ohgaki

10 years ago
Hi Pierre, On Sun, Jul 3, 2016 at 2:16 PM, Pierre Joye <pierre.php@gmail.com> wrote:
> > On Jul 3, 2016 7:04 AM, "Yasuo Ohgaki" <yohgaki@ohgaki.net> wrote: >> >> Hi Leigh, >> >> On Sat, Jul 2, 2016 at 5:39 PM, Leigh <leight@gmail.com> wrote: >> > So I have a few issues that span the RFC and the implementation. >> > >> > Your RFC states >> > >> >> hardcoded default and php.ini-* default values are the same. >> > >> > This is not the case. >> > >> > Originally the session id length and character set were controlled by >> > session.hash_function and/or session.hash_bits_per_character. These >> > customisations to configuration will be lost when the user upgrades. You >> > have provided a mechanism to control length and charset, but it will >> > require >> > new changes to the default settings. This needs to be noted as a >> > breaking >> > change. >> > >> > Your default for session.sid_length is 48. Up to 7.1 the session id >> > length >> > is 32. Your default for session.sid_bits_per_character is 5, up to 7.1 >> > the >> > session id uses 4 bits per character. This is a breaking change. >> > (Imagine >> > custom session handlers that validate session id character sets, or >> > database >> > schemas that will truncate after 32 characters) >> >> I'll update relevant part. >> >> > Your patch updates session.use_strict_mode from 0 to 1. I actually don't >> > know what this changes, but it's an undocumented change. >> >> This is unintentional. I'll remove this part. >> >> > Overall your patch looks very similar to the one I was working on >> > earlier in >> > the year, although you appear to have deleted a bunch of tests that you >> > could have just updated. You should probably put those back, and update >> > them. >> >> It removes hashing, so irrelevant tests are simply removed. >> >> Thank you for point them out. >> I'll fix them now. > > Restart vote too please.
Sure. I extended vote period. Fix for RFC and patch is done. Please vote. https://wiki.php.net/rfc/session-id-without-hashing Thank you!
-- Yasuo Ohgaki yohgaki@ohgaki.net

Derick Rethans

10 years ago
On Sat, 2 Jul 2016, Leigh wrote:
> Your patch updates session.use_strict_mode from 0 to 1. I actually don't > know what this changes, but it's an undocumented change.
http://php.net/manual/en/session.configuration.php#ini.session.use-strict-mode session.use_strict_mode specifies whether the module will use strict session id mode. If this mode is enabled, the module does not accept uninitialized session ID. If uninitialized session ID is sent from browser, new session ID is sent to browser. Applications are protected from session fixation via session adoption with strict mode. Defaults to 0 (disabled).

Yasuo Ohgaki

10 years ago
Hi Stas and Danack On Sat, Jul 2, 2016 at 4:35 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> This proposal cleans up session code by removing hash. > > https://wiki.php.net/rfc/session-id-without-hashing > > I set vote requires 2/3 support. > Please describe the reason why when you against this RFC. Reasons are > important for improvements! > > Thank you!
Could you share the reason why against this change? Thank you!
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

10 years ago
Hi!
> Could you share the reason why against this change?
1. I'm not sure exporting raw generator state is a good practice. I may change my opinion on the subject if I hear from some security people (I'm no crypto expert) that this is ok, then I may change my opinion. 2. Due to (1), I do not think it makes sense to do this change, because we produce no benefit (session generation speed is not that important since nobody generates millions of sessions at once) and create potential problems.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

10 years ago
Hi Stas, Thank you for sharing opinion. Followings is mine. On Tue, Jul 5, 2016 at 7:23 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> Could you share the reason why against this change? > > 1. I'm not sure exporting raw generator state is a good practice. I may > change my opinion on the subject if I hear from some security people > (I'm no crypto expert) that this is ok, then I may change my opinion.
I think no one can guarantee security of CSPRNG on all platforms. "CS" means cryptographic safety and cryptographers recommends not to reinvent crypt related functions/features.
> 2. Due to (1), I do not think it makes sense to do this change, because > we produce no benefit (session generation speed is not that important > since nobody generates millions of sessions at once) and create > potential problems.
Current implementation is regenerating random hash string by using - PID - Time (Simple random function) - CSPRNG when it is available 10 years ago, I would agree with your opinion, since old PRNG is not as good as current one. Today, I don't see the reason why we should reinvent secure random bytes as we do now. I agree we should have some mitigations just in case a CSPRNG is broken badly. The patch uses 4 to 6 bits to generate readable string and read extra 60 bytes to hide raw PRNG state. This mitigation should be good enough for somewhat broken CSPRNG. In addition, even when CSPRNG generates the same random bytes, session module has collision detection and will not create the same session ID. I hope you and others are convinced by this explanation. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Pierre Joye

10 years ago
On Jul 5, 2016 6:14 AM, "Yasuo Ohgaki" <yohgaki@ohgaki.net> wrote:
> > Hi Stas, > > Thank you for sharing opinion. > Followings is mine. > > On Tue, Jul 5, 2016 at 7:23 AM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:
> >> Could you share the reason why against this change? > > > > 1. I'm not sure exporting raw generator state is a good practice. I may > > change my opinion on the subject if I hear from some security people > > (I'm no crypto expert) that this is ok, then I may change my opinion. > > I think no one can guarantee security of CSPRNG on all platforms. > > "CS" means cryptographic safety and cryptographers recommends > not to reinvent crypt related functions/features. > > > 2. Due to (1), I do not think it makes sense to do this change, because > > we produce no benefit (session generation speed is not that important > > since nobody generates millions of sessions at once) and create > > potential problems. > > Current implementation is regenerating random hash string by using > > - PID > - Time (Simple random function) > - CSPRNG when it is available
For clarification, it is always available. Php requires a valid one to be built. We can argue about the provided pnrng being CS but it is not php's job to decide.

Yasuo Ohgaki

10 years ago
Hi Pierre, On Tue, Jul 5, 2016 at 12:02 PM, Pierre Joye <pierre.php@gmail.com> wrote:
>> Current implementation is regenerating random hash string by using >> >> - PID >> - Time (Simple random function) >> - CSPRNG when it is available > > For clarification, it is always available. Php requires a valid one to be > built. > > We can argue about the provided pnrng being CS but it is not php's job to > decide.
Thank you for clarification! Modern systems that execute PHP should have CSPRNG always. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Leigh

10 years ago
On 5 July 2016 at 04:02, Pierre Joye <pierre.php@gmail.com> wrote:
> We can argue about the provided pnrng being CS but it is not php's job to > decide.
I think we need to drop the concerns about exposing "RNG state". A reminder of what php_random_bytes looks at (in order): * CryptGenRandom on Windows * arc4random_buf on modern BSD (where ChaCha20 is used) * Linux getrandom(2) syscall where available * /dev/urandom where available * Throws an exception if it cannot access one of the above If these are weak RNGs on your system, YOUR SYSTEM is broken. They are all designed to be cryptographic quality. If people are unconvinced we can temper the values with a secondary RNG, but there is absolutely no need to generate session IDs using a slow hashing algorithm. For the record, I am +1 on removing hashing, -1 on the _other_ changes in this RFC

Christoph Becker

10 years ago
On 05.07.2016 at 16:32, Leigh wrote:
> On 5 July 2016 at 04:02, Pierre Joye <pierre.php@gmail.com> wrote: >> We can argue about the provided pnrng being CS but it is not php's job to >> decide. > > I think we need to drop the concerns about exposing "RNG state". > > A reminder of what php_random_bytes looks at (in order): > * CryptGenRandom on Windows > * arc4random_buf on modern BSD (where ChaCha20 is used) > * Linux getrandom(2) syscall where available > * /dev/urandom where available > * Throws an exception if it cannot access one of the above
Would that imply that in this latter case sessions couldn't be used anymore? What would be the fallback in that case? From a quick glance at the current PR there appears to be none!
-- Christoph M. Becker

Tom Worster

10 years ago
On 7/5/16 11:37 AM, Christoph Becker wrote:
> On 05.07.2016 at 16:32, Leigh wrote: > >> On 5 July 2016 at 04:02, Pierre Joye <pierre.php@gmail.com> wrote: >>> We can argue about the provided pnrng being CS but it is not php's job to >>> decide. >> >> I think we need to drop the concerns about exposing "RNG state". >> >> A reminder of what php_random_bytes looks at (in order): >> * CryptGenRandom on Windows >> * arc4random_buf on modern BSD (where ChaCha20 is used) >> * Linux getrandom(2) syscall where available >> * /dev/urandom where available >> * Throws an exception if it cannot access one of the above > > Would that imply that in this latter case sessions couldn't be used > anymore?
I hope so. It's not safe to use sessions if PHP cannot get unpredictable randoms for session IDs. PHP should therefore error so that the sys op can be alerted and fix the problem. Tom

Yasuo Ohgaki

10 years ago
Hi Christoph, On Wed, Jul 6, 2016 at 12:37 AM, Christoph Becker <cmbecker69@gmx.de> wrote:
> On 05.07.2016 at 16:32, Leigh wrote: > >> On 5 July 2016 at 04:02, Pierre Joye <pierre.php@gmail.com> wrote: >>> We can argue about the provided pnrng being CS but it is not php's job to >>> decide. >> >> I think we need to drop the concerns about exposing "RNG state". >> >> A reminder of what php_random_bytes looks at (in order): >> * CryptGenRandom on Windows >> * arc4random_buf on modern BSD (where ChaCha20 is used) >> * Linux getrandom(2) syscall where available >> * /dev/urandom where available >> * Throws an exception if it cannot access one of the above > > Would that imply that in this latter case sessions couldn't be used > anymore? What would be the fallback in that case? From a quick glance > at the current PR there appears to be none!
It relies on php_random_bytes() defined in ext/standard/random.c Current PHP does not build without decent PRNG. The patch uses php_random_bytes() simply. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Christoph Becker

10 years ago
Hi Yasuo! On 06.07.2016 at 03:51, Yasuo Ohgaki wrote:
> > On Wed, Jul 6, 2016 at 12:37 AM, Christoph Becker <cmbecker69@gmx.de> wrote: >> On 05.07.2016 at 16:32, Leigh wrote: >> >>> On 5 July 2016 at 04:02, Pierre Joye <pierre.php@gmail.com> wrote: >>>> We can argue about the provided pnrng being CS but it is not php's job to >>>> decide. >>> >>> I think we need to drop the concerns about exposing "RNG state". >>> >>> A reminder of what php_random_bytes looks at (in order): >>> * CryptGenRandom on Windows >>> * arc4random_buf on modern BSD (where ChaCha20 is used) >>> * Linux getrandom(2) syscall where available >>> * /dev/urandom where available >>> * Throws an exception if it cannot access one of the above >> >> Would that imply that in this latter case sessions couldn't be used >> anymore? What would be the fallback in that case? From a quick glance >> at the current PR there appears to be none! > > It relies on php_random_bytes() defined in ext/standard/random.c > Current PHP does not build without decent PRNG. The patch uses > php_random_bytes() simply.
Yes, I am aware that the patch uses php_random_bytes(), but what happens when it fails, in which case php_session_create_id() returns null[1]? Would it be impossible to use a session in this case? [1] <https://github.com/php/php-src/pull/1850/files#diff-52eb9eb7f9d5d9125fbb1337a6541c06R315>
-- Christoph M. Becker

Leigh

10 years ago
On Wed, 6 Jul 2016 at 13:10 Christoph Becker <cmbecker69@gmx.de> wrote:
> > Yes, I am aware that the patch uses php_random_bytes(), but what happens > when it fails, in which case php_session_create_id() returns null[1]? > Would it be impossible to use a session in this case? > > [1] > < > https://github.com/php/php-src/pull/1850/files#diff-52eb9eb7f9d5d9125fbb1337a6541c06R315 > > > > -- > Christoph M. Becker >
The FAILURE check here is redundant because it is using the _throw variant of random_bytes, which means an exception is thrown if there isn't a good source of random available.

Yasuo Ohgaki

10 years ago
Hi Christoph, On Wed, Jul 6, 2016 at 9:10 PM, Christoph Becker <cmbecker69@gmx.de> wrote:
> > Yes, I am aware that the patch uses php_random_bytes(), but what happens > when it fails, in which case php_session_create_id() returns null[1]? > Would it be impossible to use a session in this case?
php_session_create_id() may return NULL. It's an usual error. Session module supports session ID creation save handler which may return anything valid for the type. Session module tries to call php_session_create_id() several places/times. If it could not get valid one, it raises E_RECOVERABLE_ERROR or E_ERROR eventually. Although it's not enabled yet, PHP_FUNCTION(session_create_id) returns FALSE it fails. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Dan Ackroyd

10 years ago
> I think we need to drop the concerns about exposing "RNG state". > > If these are weak RNGs on your system, YOUR SYSTEM is broken.
Telling people that their system is broken isn't going to be comforting to the people it happens to. There are always bugs in software and hardware. At some point it is almost inevitable that there will be some information leak through exposing the random numbers directly. Just telling people that their system is broken and they need to buy need hardware immediately, rather than just being able to re-enable hashing seems a bad choice. But even without accidental bugs, the scenario I am remain concerned with is when a piece of hardware or software is compromised by a sophisticated attacker, (hello NSA!) and the 'random' numbers generated have some subtle pattern to them. To almost everyone, the random numbers generated would still look random. But to the organisation that compromised the system, and so knows the algorithm that is leaving traces in the random sequences, exposing the random numbers directly to the outside world would be an obvious but almost untraceable line of attack on the system. Hashing should still obfuscate any subtle patterns in the random number generation, and so give some protection against comprised chip-set/firmware. If users choose to continue to want the extra security of hashing, at the cost of slightly slower session ID generation we shouldn't removed that from them. Yasuo wrote:
> Some of us worried about CSPRNG state exposure. I'm wondering how many > of you will vote in favor if I change the RFC to use hash functions > optionally.
Yes. Though as I said before, I think changing session.use_strict_mode should be a separate decision, and it's one I support doing. cheers Dan

Yasuo Ohgaki

10 years ago
Hi Dan, On Fri, Jul 8, 2016 at 5:33 AM, Dan Ackroyd <danack@basereality.com> wrote:
>> I think we need to drop the concerns about exposing "RNG state". >> >> If these are weak RNGs on your system, YOUR SYSTEM is broken. > > Telling people that their system is broken isn't going to be > comforting to the people it happens to. > > There are always bugs in software and hardware. At some point it is > almost inevitable that there will be some information leak through > exposing the random numbers directly. Just telling people that their > system is broken and they need to buy need hardware immediately, > rather than just being able to re-enable hashing seems a bad choice. > > But even without accidental bugs, the scenario I am remain concerned > with is when a piece of hardware or software is compromised by a > sophisticated attacker, (hello NSA!) and the 'random' numbers > generated have some subtle pattern to them. To almost everyone, the > random numbers generated would still look random. But to the > organisation that compromised the system, and so knows the algorithm > that is leaving traces in the random sequences, exposing the random > numbers directly to the outside world would be an obvious but almost > untraceable line of attack on the system. > > Hashing should still obfuscate any subtle patterns in the random > number generation, and so give some protection against comprised > chip-set/firmware. If users choose to continue to want the extra > security of hashing, at the cost of slightly slower session ID > generation we shouldn't removed that from them.
I understand concern and I would not argue against that hashing should still obfuscate PRNG state. The patch proposed/session module has mitigations - Read extra bytes from RPNG to hide raw PRNG state (This should be good enough mitigation for usable CSPRNG) - In case PRNG generates the same session IDs repeatedly, it terminate execution with E_ERROR/E_RECOVERABLE_ERROR. (This shouldn't happen with good CSPRNG) Above mitigations should be good enough for CSRPNG applications. System with seriously broken CSPRNG should not be used anyway. IMO. Vote requires 2/3 in favor. Current vote is 6 vs 3. If this RFC declined, I'll prepare patch that has optional hashing. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Niklas Keller

10 years ago
> > > I think we need to drop the concerns about exposing "RNG state". > > > > If these are weak RNGs on your system, YOUR SYSTEM is broken. > > Telling people that their system is broken isn't going to be > comforting to the people it happens to. >
Sure, but it's the right way. Just like random_bytes having no fallback. There are always bugs in software and hardware. At some point it is
> almost inevitable that there will be some information leak through > exposing the random numbers directly. Just telling people that their > system is broken and they need to buy need hardware immediately, > rather than just being able to re-enable hashing seems a bad choice. >
Nobody talks about buying new hardware. It usually happens due to bad system setup, such as blocking access to /dev/random. But even without accidental bugs, the scenario I am remain concerned

Leigh

10 years ago
On 7 July 2016 at 21:33, Dan Ackroyd <danack@basereality.com> wrote:
>> I think we need to drop the concerns about exposing "RNG state". >> >> If these are weak RNGs on your system, YOUR SYSTEM is broken. > > Telling people that their system is broken isn't going to be > comforting to the people it happens to.
Of course it isn't. If I find out suddenly that /dev/urandom is somehow predictable I would be incredibly uncomfortable. However the least of my worries would be PHPs session id generation.
> There are always bugs in software and hardware. At some point it is > almost inevitable that there will be some information leak through > exposing the random numbers directly. Just telling people that their > system is broken and they need to buy need hardware immediately, > rather than just being able to re-enable hashing seems a bad choice.
I feel like we're back to the discussions around random_bytes() again. The random number generators being used here are designated CS for a reason. They are not like deterministic RNGs where you can give them simple seed and produce the same set of outputs. They draw on environmental noise for additional entropy and where appropriate they reseed regularly, far before any useful statistical information can be gathered. In the case of urandom it actually performs a SHA1 on its pool, CryptGenRandom mixes in data that has been MD4 hashed, arc4random_buf is literally a stream cipher. These are well maintained and vetted systems. An extra round of hashing is simply unnecessary overhead.
> But even without accidental bugs, the scenario I am remain concerned > with is when a piece of hardware or software is compromised by a > sophisticated attacker, (hello NSA!) and the 'random' numbers > generated have some subtle pattern to them. To almost everyone, the > random numbers generated would still look random. But to the > organisation that compromised the system, and so knows the algorithm > that is leaving traces in the random sequences, exposing the random > numbers directly to the outside world would be an obvious but almost > untraceable line of attack on the system.
I know it's easy to say: If your system has been compromised you have bigger problems. But this is pretty much the fact of it. If someone has the ability to make your system CSPRNG predictable, they can almost certainly get anything else they want from the system anyway without resorting to that.

Dan Ackroyd

10 years ago
Hi Yasuo,
> Could you share the reason why against this change?
The RFC is doing separate things: * Using a proper random number generator - which is probably a good thing, and I probably would vote/support that change by itself. * Removing old stuff for performance reasons - probably a bad thing, as minor performance improvements don't justify changing the ini settings imo. * Slipping in a change to make 'session.use_strict_mode' be enbled by default. I actually agree with this change, but I don't like how it's been combined into an unrelated RFC. * Exposing information that could be useful to an attacker, from the RFC:
> If PRNG has vulnerability that generates predictable value, exposing > raw PRNG state helps attackers. Although, this is unlikely with modern PRNGs, but > risk is there.
I don't agree that faster session generation is worth exposing the random number generator in a completely known way to the outside world. Reading an extra 60 bytes almost certainly does nothing to mitigate that exposure.
> cryptographers recommends > not to reinvent crypt related functions/features.
They also recommend reducing your attack surface as much as possible, not opening it up to the point where you're relying on the CSPRNG to work perfectly to avoid having a security hole. But more importantly, as I wrote before (http://news.php.net/php.internals/90940) I don't think the session extension is in a state where incremental improvements are a good way to proceed:
> The problem is that you're trying to build on a foundation of sand. > The session handling works but is incredibly fragile.
> To me, there are two good ways to proceed:
> i) Develop a new session extension, that doesn't depend on magic > behaviour of globals and leave the current session handler as it is. > The new session extension could be shipped as a 'work in progress' when > it's good enough, before PHP 8. Then when it's stable, we could figure > out how to transition users from the old extension. > > ii) Develop a session handler in userland code only. PHP is powerful > enough to support this. Although obviously there are big benefits to > shipping a session handler with PHP, I don't see any need for it to be > done internally other than we don't currently have a way of shipping > userland code with an extension. I'm hoping that before PHP 8, the > ability to ship PHP code as part of extensions would be in place.
cheers Dan

Yasuo Ohgaki

10 years ago
Hi Dan, On Tue, Jul 5, 2016 at 9:36 AM, Dan Ackroyd <danack@basereality.com> wrote:
>> Could you share the reason why against this change? > > The RFC is doing separate things:
No. It simply follows best practice not to reinvent wheel and keep things simple.
> > * Using a proper random number generator - which is probably a good > thing, and I probably would vote/support that change by itself.
Use of not a really random values like pid and time is bad practice.
> > * Removing old stuff for performance reasons - probably a bad thing, > as minor performance improvements don't justify changing the ini > settings imo.
It's not an objective, but side effect improvement.
> > * Slipping in a change to make 'session.use_strict_mode' be enbled by > default. I actually agree with this change, but I don't like how it's > been combined into an unrelated RFC.
There is a request for inclusion, but I made separate RFC. Besides that making separate RFCs for this kind of simple mandatory change does not make much sense. I have too many RFCs already. I created RFC for it, though.
> * Exposing information that could be useful to an attacker, from the RFC:
I don't understand this part.
> >> If PRNG has vulnerability that generates predictable value, exposing >> raw PRNG state helps attackers. Although, this is unlikely with modern PRNGs, but >> risk is there. > > I don't agree that faster session generation is worth exposing the > random number generator in a completely known way to the outside > world. Reading an extra 60 bytes almost certainly does nothing to > mitigate that exposure.
Don't miss the basis of this RFC. It's not PRNG, but CSPRNG. Exposure of raw CSPRNG should not be a security issue at all by CSPRNG definition. Additional random data that read from CSPRNG should be good enough mitigation.
> >> cryptographers recommends >> not to reinvent crypt related functions/features. > > They also recommend reducing your attack surface as much as possible, > not opening it up to the point where you're relying on the CSPRNG to > work perfectly to avoid having a security hole.
Use of poor random sources like pid and time is bad practice. Old SSL used multiple hashes to mitigate hash functions' vulnerabilities, but this method is not used today for reason.
> > But more importantly, as I wrote before > (http://news.php.net/php.internals/90940) I don't think the session > extension is in a state where incremental improvements are a good way > to proceed: > >> The problem is that you're trying to build on a foundation of sand. >> The session handling works but is incredibly fragile. > >> To me, there are two good ways to proceed: > >> i) Develop a new session extension, that doesn't depend on magic >> behaviour of globals and leave the current session handler as it is. >> The new session extension could be shipped as a 'work in progress' when >> it's good enough, before PHP 8. Then when it's stable, we could figure >> out how to transition users from the old extension.
Basic design of session module is good enough, IMHO. Even if we rewrite session module, it will end up with similar implementation. Complexity of state(session) management cannot be reduced by rewrite. Object interface has/had lots of problems. This part could be improved one by one. If nobody objects, I can clean them up with a single RFC.
>> >> ii) Develop a session handler in userland code only. PHP is powerful >> enough to support this. Although obviously there are big benefits to >> shipping a session handler with PHP, I don't see any need for it to be >> done internally other than we don't currently have a way of shipping >> userland code with an extension. I'm hoping that before PHP 8, the >> ability to ship PHP code as part of extensions would be in place.
I think this is not good option. Session management is complex and difficult to understand what it should be. I've proposed both minimal/simple and complex/comprehensive timestamp based session management which is mandatory for session management, but both of RFCs are declined. I suppose this proved difficulty of proper session management. It's slow when it is implemented as PHP script also. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi all, On Sat, Jul 2, 2016 at 4:35 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Currently session module uses obsolete MD5 for session ID. With > CSPRNG, hashing is redundant and needless. It adds hash module > dependency and inefficient (There is no reason to use hash for CSPRNG > generated bytes). > > This proposal cleans up session code by removing hash. > > https://wiki.php.net/rfc/session-id-without-hashing > > I set vote requires 2/3 support. > Please describe the reason why when you against this RFC. Reasons are > important for improvements! > > Thank you!
Some of us worried about CSPRNG state exposure. I'm wondering how many of you will vote in favor if I change the RFC to use hash functions optionally. This means code and INI settings related to hash function selection will remain. Please note that ext/hash is not built always. If you against keeping hash related code, please let me know also. Thank you!
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

10 years ago
Hi!
> Some of us worried about CSPRNG state exposure. I'm wondering how many > of you will vote in favor if I change the RFC to use hash functions > optionally. This means code and INI settings related to hash function > selection will remain. Please note that ext/hash is not built always. > If you against keeping hash related code, please let me know also.
If there would be an option to not use hash, I would be completely fine with it.
-- Stas Malyshev smalyshev@gmail.com

Stas Malyshev

10 years ago
Hi!
> Some of us worried about CSPRNG state exposure. I'm wondering how many > of you will vote in favor if I change the RFC to use hash functions > optionally. This means code and INI settings related to hash function > selection will remain. Please note that ext/hash is not built always. > If you against keeping hash related code, please let me know also.
Re-reading RFC, I'd also propose this: - Make session.sid_bits_per_character take value from session.hash_bits_per_character if the latter is defined and the former is not - Remove use_strict_mode change, it is a different issue. With this, and possibility of keeping hash as non-default option, I'd be for this RFC.
-- Stas Malyshev smalyshev@gmail.com

Derick Rethans

10 years ago
On Sat, 2 Jul 2016, Yasuo Ohgaki wrote:
> Hi all, > > Currently session module uses obsolete MD5 for session ID. With > CSPRNG, hashing is redundant and needless. It adds hash module > dependency and inefficient (There is no reason to use hash for CSPRNG > generated bytes). > > This proposal cleans up session code by removing hash. > > https://wiki.php.net/rfc/session-id-without-hashing > > I set vote requires 2/3 support. > Please describe the reason why when you against this RFC. Reasons are > important for improvements!
I'm voting "no" bceause of session.use_strict_mode (0 to 1) - Changed as insurance of broken PRNG implementation. And it not being mentioned in BC breaking changes. It changes behaviour of session IDs, as it shown in the manual: session.use_strict_mode boolean session.use_strict_mode specifies whether the module will use strict session id mode. If this mode is enabled, the module does not accept uninitialized session ID. If uninitialized session ID is sent from browser, new session ID is sent to browser. Applications are protected from session fixation via session adoption with strict mode. Defaults to 0 (disabled). cheers, Derick

Yasuo Ohgaki

10 years ago
Hi Derick, On Thu, Jul 7, 2016 at 9:29 PM, Derick Rethans <derick@php.net> wrote:
>> Hi all, >> >> Currently session module uses obsolete MD5 for session ID. With >> CSPRNG, hashing is redundant and needless. It adds hash module >> dependency and inefficient (There is no reason to use hash for CSPRNG >> generated bytes). >> >> This proposal cleans up session code by removing hash. >> >> https://wiki.php.net/rfc/session-id-without-hashing >> >> I set vote requires 2/3 support. >> Please describe the reason why when you against this RFC. Reasons are >> important for improvements! > > I'm voting "no" bceause of > > session.use_strict_mode (0 to 1) - Changed as insurance of broken PRNG implementation. > > And it not being mentioned in BC breaking changes. It changes behaviour > of session IDs, as it shown in the manual: > > session.use_strict_mode boolean > > session.use_strict_mode specifies whether the module will use strict > session id mode. If this mode is enabled, the module does not accept > uninitialized session ID. If uninitialized session ID is sent from > browser, new session ID is sent to browser. Applications are > protected from session fixation via session adoption with strict > mode. Defaults to 0 (disabled).
It was moved to other RFC. https://wiki.php.net/rfc/session-use-strict-mode Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi all, On Sat, Jul 2, 2016 at 4:35 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Currently session module uses obsolete MD5 for session ID. With > CSPRNG, hashing is redundant and needless. It adds hash module > dependency and inefficient (There is no reason to use hash for CSPRNG > generated bytes). > > This proposal cleans up session code by removing hash. > > https://wiki.php.net/rfc/session-id-without-hashing > > I set vote requires 2/3 support. > Please describe the reason why when you against this RFC. Reasons are > important for improvements! > > Thank you!
Thank you for voting and the RFC has passed 13 vs 5. I'll prepare documents and merge the change in a few days. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Derick Rethans

10 years ago
Hi, The voted-upon-RFC still has
> session.use_strict_mode (0 to 1) - Changed as insurance of broken PRNG implementation.
Although you said: It was moved to other RFC. https://wiki.php.net/rfc/session-use-strict-mode And neither did you restart voting after modifying the RFC - or writing down in the RFC's changes that it got changed. So what's the deal? cheers, Derick On Tue, 12 Jul 2016, Yasuo Ohgaki wrote:
> Hi all, > > On Sat, Jul 2, 2016 at 4:35 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > > Currently session module uses obsolete MD5 for session ID. With > > CSPRNG, hashing is redundant and needless. It adds hash module > > dependency and inefficient (There is no reason to use hash for CSPRNG > > generated bytes). > > > > This proposal cleans up session code by removing hash. > > > > https://wiki.php.net/rfc/session-id-without-hashing > > > > I set vote requires 2/3 support. > > Please describe the reason why when you against this RFC. Reasons are > > important for improvements! > > > > Thank you! > > Thank you for voting and the RFC has passed 13 vs 5. > I'll prepare documents and merge the change in a few days. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > >
-- https://derickrethans.nl | https://xdebug.org | https://dram.io Like Xdebug? Consider a donation: https://xdebug.org/donate.php twitter: @derickr and @xdebug

Davey

10 years ago
On Tue, Jul 12, 2016 at 3:25 AM, Derick Rethans <derick@php.net> wrote:
> Hi, > > The voted-upon-RFC still has > > > session.use_strict_mode (0 to 1) - Changed as insurance of broken > PRNG implementation. > > Although you said: > > It was moved to other RFC. > > https://wiki.php.net/rfc/session-use-strict-mode > > And neither did you restart voting after modifying the RFC - or writing > down in the RFC's changes that it got changed. > > So what's the deal? >
I'd like to see the vote re-run (1 week?) with the changes in place. I didn't vote because I expected it to be restarted. I would have voted -1 on the current proposal. Also, is it possible to add a notice/warning if any of the removed config settings are set to a non-default value? We should also have the defaults be the same as for older versions of PHP, otherwise it's a BC break. That is: session.sid_length=32 session.sid_bits_per_character=4 Better settings should be documented and in the default ini files, but not be changed till 8.0 IMO. I apologize for this feedback being so late. Thanks, - Davey

Yasuo Ohgaki

10 years ago
Hi Davey, On Wed, Jul 13, 2016 at 6:59 AM, Davey Shafik <davey@php.net> wrote:
> On Tue, Jul 12, 2016 at 3:25 AM, Derick Rethans <derick@php.net> wrote: >> >> Hi, >> >> The voted-upon-RFC still has >> >> > session.use_strict_mode (0 to 1) - Changed as insurance of broken >> > PRNG implementation. >> >> Although you said: >> >> It was moved to other RFC. >> >> https://wiki.php.net/rfc/session-use-strict-mode >> >> And neither did you restart voting after modifying the RFC - or writing >> down in the RFC's changes that it got changed. >> >> So what's the deal? > > > I'd like to see the vote re-run (1 week?) with the changes in place. I > didn't vote because I expected it to be restarted. I would have voted -1 on > the current proposal. > > Also, is it possible to add a notice/warning if any of the removed config > settings are set to a non-default value? > > We should also have the defaults be the same as for older versions of PHP, > otherwise it's a BC break. That is: > > session.sid_length=32 > session.sid_bits_per_character=4 > > Better settings should be documented and in the default ini files, but not > be changed till 8.0 IMO. > > I apologize for this feedback being so late.
Thank you for the comment! I think it's not good idea to change the default from now. I'll document compatibility issue well in UPGRADING. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi Derick, On Tue, Jul 12, 2016 at 7:25 PM, Derick Rethans <derick@php.net> wrote:
> Hi, > > The voted-upon-RFC still has > >> session.use_strict_mode (0 to 1) - Changed as insurance of broken PRNG implementation. > > Although you said: > > It was moved to other RFC. > > https://wiki.php.net/rfc/session-use-strict-mode > > And neither did you restart voting after modifying the RFC - or writing > down in the RFC's changes that it got changed. > > So what's the deal?
session.use_strict_mode change in the patch is included by mistake. Someone pointed it out immediately after starting vote, RFC does not refer use_strice_mode change. I removed the change and extended vote period a day. RFC itself does not change (It didn't mention use_strict_mode change at all) and I didn't get any comments on this, but now I got one. Any suggestions? Re-opening vote few days or a week is fine with me. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi Derick, On Tue, Jul 12, 2016 at 7:25 PM, Derick Rethans <derick@php.net> wrote:
> The voted-upon-RFC still has > >> session.use_strict_mode (0 to 1) - Changed as insurance of broken PRNG implementation. > > Although you said: > > It was moved to other RFC. > > https://wiki.php.net/rfc/session-use-strict-mode > > And neither did you restart voting after modifying the RFC - or writing > down in the RFC's changes that it got changed. > > So what's the deal?
I missed to remove the related lines in RFC. I marked the line by <del>. I don't mind reopen the vote few days. Any objections? If no objections, I'll reopen vote few days. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Stas Malyshev

10 years ago
Hi!
> I missed to remove the related lines in RFC. I marked the line by <del>. > I don't mind reopen the vote few days. Any objections? > If no objections, I'll reopen vote few days.
We already had a vote, at it was completed. Having another vote on the same subject, slightly modified, is highly irregular and contrary to voting RFC, which mandates 6 month period or *substantial* changes (with assumed new discussion period I imagine, since past discussion can't really count for substantially changed proposal) to schedule a new vote on a rejected proposal. This also gives pretty bad example - on failed vote, tweak a little issue and issue immediate revote, repeat until one of the votes succeeds. I understand that this is not at all your intent here, but it's the pattern that we do not want to enable. I voted yes for it, and it is a pity that it failed, as it seems, because of a miscommunication (maybe not, I don't know), but going against our own agreed process I think is not a good outcome either.
-- Stas Malyshev smalyshev@gmail.com

Stas Malyshev

10 years ago
Hi!
> We already had a vote, at it was completed. Having another vote on the > same subject, slightly modified, is highly irregular and contrary to > voting RFC, which mandates 6 month period or *substantial* changes (with > assumed new discussion period I imagine, since past discussion can't > really count for substantially changed proposal) to schedule a new vote > on a rejected proposal. > > This also gives pretty bad example - on failed vote, tweak a little > issue and issue immediate revote, repeat until one of the votes > succeeds. I understand that this is not at all your intent here, but > it's the pattern that we do not want to enable. > > I voted yes for it, and it is a pity that it failed, as it seems, > because of a miscommunication (maybe not, I don't know), but going > against our own agreed process I think is not a good outcome either.
Oops, I thought I was talking about another RFC vote that failed. Sorry for the confusion. This one seems to have succeeded. This is weirder case then... I'd say just implement the uncontroversial part then and submit the controversial part as a new RFC?
-- Stas Malyshev smalyshev@gmail.com

Davey

10 years ago
Stas, The issue is that changes were made once the voting started, and some of us were waiting for the vote to restart:
> I'd like to see the vote re-run (1 week?) with the changes in place. I
didn't vote because I expected it to be restarted. I would have voted -1 on the current proposal. Changing the RFC during voting requires a _restart_ not an extension. The vote must be re-run. I will not put this in 7.1 without a new vote. - Davey On Sat, Jul 23, 2016 at 12:50 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:

Stas Malyshev

10 years ago
Hi!
> Changing the RFC during voting requires a _restart_ not an extension. > The vote must be re-run. I will not put this in 7.1 without a new vote.
OK, looks like I was underestimating the magnitude of the messiness that this change (or lack of it) brought to a vote. Let's re-run the vote then, I think that would be the best solution given the confusion that apparently happened.
-- Stas Malyshev smalyshev@gmail.com

Yasuo Ohgaki

10 years ago
Hi all, On Sun, Jul 24, 2016 at 6:13 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> Changing the RFC during voting requires a _restart_ not an extension. >> The vote must be re-run. I will not put this in 7.1 without a new vote. > > OK, looks like I was underestimating the magnitude of the messiness that > this change (or lack of it) brought to a vote. Let's re-run the vote > then, I think that would be the best solution given the confusion that > apparently happened.
I'll open reopen the vote a week. Is this too long? Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Yasuo Ohgaki

10 years ago
Hi all and RM, On Sat, Jul 2, 2016 at 4:35 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Currently session module uses obsolete MD5 for session ID. With > CSPRNG, hashing is redundant and needless. It adds hash module > dependency and inefficient (There is no reason to use hash for CSPRNG > generated bytes). > > This proposal cleans up session code by removing hash. > > https://wiki.php.net/rfc/session-id-without-hashing > > I set vote requires 2/3 support. > Please describe the reason why when you against this RFC. Reasons are > important for improvements!
The changes in the manual is committed. UPGRADING is written, PR branch is synced for merge. https://github.com/php/php-src/pull/1850 Should I merge or just telling "please merge" is OK? Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net