[RFC][VOTE] Session ID without hashing

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

Actually decided to post so

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.

Hi Leigh,

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

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).

Hi Stas,

Thank you for sharing opinion.
Followings is mine.

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

Hi Dan,

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

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

Hi Derick,

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

Hi Derick,

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