# [RFC] Secure Session Module Options by Default

- Thread: https://externals.io/message/72049
- Original archive: https://news-web.php.net/php.internals/72049
- Started by Andrey Andreev on 2014-02-03T07:31:28+00:00
- Messages: 13
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #72049 — Andrey Andreev — 2014-02-03T07:31:28+00:00

URL: https://externals.io/message/72049#72049

Hey,

Just sharing my initial thoughts on this ...

'use_strict_mode' is supposed to generate a new session ID when the
application receives a session cookie with an invalid (non-existing)
session ID.
I don't think that it should've ever changed the behavior of
 `session_id()` and I'd rather consider it a bug if it doesn't allow it
to set the new session ID.

'hash_bits_per_character' has nothing to do with security and its only
useful purpose that I could think of is maintainig BC for
database-stored session data, where i.e. the ID used to be an md5 hash
and and switching to sha1 would require altering the field length to
40. Increasing hash_bit_per_character in that case would allow a
sha-1-sized hash to be stored in 32 characters.
Other than that (and probably some crazy traffic volume-related
micro-optimization), it's pretty useless.

That "allow an underscore when hash_bits_per_character=6" is also not
in the scope of security and the hash function itself wouldn't
generate an underscore, so ... what has it got to do with anything?

Cheers,
Andrey.

---

## #72070 — Yasuo Ohgaki — 2014-02-03T10:18:46+00:00

URL: https://externals.io/message/72049#72070 · In reply to: #72049

Hi Andrey,



> 'use_strict_mode' is supposed to generate a new session ID when the
> application receives a session cookie with an invalid (non-existing)
> session ID.
> I don't think that it should've ever changed the behavior of
> `session_id()` and I'd rather consider it a bug if it doesn't allow it
> to set the new session ID.
>

I can understand your opinion. Yet, it would be better for users to have
securely prefixed session ID. IMHO.

string session_id('PREFIX_', TRUE);

This does not set insecure session ID, since it only adds prefix to
generated session ID.


> 'hash_bits_per_character' has nothing to do with security and its only
> useful purpose that I could think of is maintainig BC for
> database-stored session data, where i.e. the ID used to be an md5 hash
> and and switching to sha1 would require altering the field length to
> 40. Increasing hash_bit_per_character in that case would allow a
> sha-1-sized hash to be stored in 32 characters.
> Other than that (and probably some crazy traffic volume-related
> micro-optimization), it's pretty useless.
>

Since the RFC is proposing stronger hash function as NIST suggests,
session ID needs to have longer hash bits. To reduce size of session ID,
larger hash bits is proposed. Similar reason you explained.

Some user may have limited storage size for session ID. However, it's
not a good excuse keeping weaker setting. Users need weaker settings
should set values for their own. IMHO.


> That "allow an underscore when hash_bits_per_character=6" is also not
> in the scope of security and the hash function itself wouldn't
> generate an underscore, so ... what has it got to do with anything?
>

If hash_bits_per_character=6, files save handler uses all chars for session
ID and users cannot have prefix delimiter char. User may use string offset,
though.  That's the reason why there is '_' proposal.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

---

## #72075 — Andrey Andreev — 2014-02-03T10:54:01+00:00

URL: https://externals.io/message/72049#72075 · In reply to: #72070

Hi,


> Hi Andrey,
>
> 
> > […]
>
>
> I can understand your opinion. Yet, it would be better for users to have
> securely prefixed session ID. IMHO.
>
> string session_id('PREFIX_', TRUE);
>
> This does not set insecure session ID, since it only adds prefix to
> generated session ID.

I was just pointing out that 'use_strict_mode' shouldn't change the
behavior of `session_id()`.
In other words, you don't need a "force" option, because passing a
custom made ID by itself already tells it to force something.

Don't know why you're talking about prefixes here ... nothing to do
with security.

> > […]
>
>
> Since the RFC is proposing stronger hash function as NIST suggests,
> session ID needs to have longer hash bits. To reduce size of session ID,
> larger hash bits is proposed. Similar reason you explained.
>
> Some user may have limited storage size for session ID. However, it's
> not a good excuse keeping weaker setting. Users need weaker settings
> should set values for their own. IMHO.

I'm not sure I understand what you're saying.
If it is that you want to maintain BC on the session ID length - sure,
I see how that would be nice to have.

Not that I have any authority around here, but I'd recommend updating
the RFC to properly explain this. As it stands, it makes it almost
makes it look like incrementing the 'hash_bits_per_character' value by
itself increases security, and it doesn't. :)


> > […]
>
>
> If hash_bits_per_character=6, files save handler uses all chars for session
> ID and users cannot have prefix delimiter char. User may use string offset,
> though.  That's the reason why there is '_' proposal.

This is hard to follow, I really don't understand why we're talking
about prefixing here at all and how does it relate to
'hash_bits_per_character' or security.

Cheers,
Andrey.

---

## #72132 — Yasuo Ohgaki — 2014-02-03T21:48:17+00:00

URL: https://externals.io/message/72049#72132 · In reply to: #72075

Hi Andrey,



> 
> > […]
>
> I was just pointing out that 'use_strict_mode' shouldn't change the
> behavior of `session_id()`.
> In other words, you don't need a "force" option, because passing a
> custom made ID by itself already tells it to force something.
>

Change has been done already. It does not accept uninitialized ID when
use_strict_mode=On since 5.5.


>
> Don't know why you're talking about prefixes here ... nothing to do
> with security.
>

I see many codes that uses insecure session ID. The main reason why
they write such bad code is 'lack of secure ID generation feature'.

It's security matter, IMHO.

> > […]
>
> I'm not sure I understand what you're saying.
> If it is that you want to maintain BC on the session ID length - sure,
> I see how that would be nice to have.
>

If users need to use weaker settings, these users should set weaker options
by themselves. Forcing other users to use weaker setting does not make
sense to me.

Not that I have any authority around here, but I'd recommend updating
> the RFC to properly explain this. As it stands, it makes it almost
> makes it look like incrementing the 'hash_bits_per_character' value by
> itself increases security, and it doesn't. :)


It does not improve security, but it reduces session ID length changed by
this RFC. Are you suggesting another RFC for changing
hash_bits_per_character?
Or are you suggesting I should change the default without a RFC?


> > […]
> session
> > […]
> offset,
> > […]
>
> This is hard to follow, I really don't understand why we're talking
> about prefixing here at all and how does it relate to
> 'hash_bits_per_character' or security.


To improve security, I proposed larger hash and hash bits to decrease
number
of chars. I know there are users that use prefix and delimiter. Why
shouldn't I
address these affected users in the same RFC?

This security related changes may affect users and if it does, we should
provide
work round for it at the same time if it is feasible. IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

---

## #72152 — Andrey Andreev — 2014-02-03T22:29:52+00:00

URL: https://externals.io/message/72049#72152 · In reply to: #72132

Hey,

> > […]
>
>
> Change has been done already. It does not accept uninitialized ID when
> use_strict_mode=On since 5.5.

I know that. I'm saying it should work like you suggested without the
need of another option.

session_id(<user input>); // changes session ID to the user-supplied
string, regardless of use_strict_mode

> > […]
>
>
> I see many codes that uses insecure session ID. The main reason why
> they write such bad code is 'lack of secure ID generation feature'.
>
> It's security matter, IMHO.

Why is that necessary at all? `session_regenerate_id()` not sufficient?
Either way, this shouldn't be a part of this RFC.

It's your RFC, but still - it's about changing default settings for
improved security, not new features.

> > […]
>
>
> It does not improve security, but it reduces session ID length changed by
> this RFC. Are you suggesting another RFC for changing
> hash_bits_per_character?
> Or are you suggesting I should change the default without a RFC?

We've obviously got a language barrier here.
All I'm suggesting is that you improve the *wording* in your RFC, so
that it is clear why hash_bits_per_character is changed.

> > […]
>
>
> To improve security, I proposed larger hash and hash bits to decrease number
> of chars. I know there are users that use prefix and delimiter. Why
> shouldn't I
> address these affected users in the same RFC?
>
> This security related changes may affect users and if it does, we should
> provide
> work round for it at the same time if it is feasible. IMHO.

Because, as I said above - I thought this RFC was about changing
default settings and nothing else.
And again, an underscore *is not* a part of the hash, regardless of
hash_bits_per_character.

I personally don't care about the prefix. It's just that it's a
completely new feature that has nothing to do with the default
settings and it's just confusing that you mention it in the same
context. :)

Cheers,
Andrey.

---

## #72155 — Yasuo Ohgaki — 2014-02-03T23:05:06+00:00

URL: https://externals.io/message/72049#72155 · In reply to: #72152

Hi Andrey,



> > […]
>
> I know that. I'm saying it should work like you suggested without the
> need of another option.
>
> session_id(<user input>); // changes session ID to the user-supplied
> string, regardless of use_strict_mode
>
>
You may be referring old RFC. Current RFC is

Add optional $force_id parameter for `session_id()` to allow any id
regardless of this mode.

  bool session_id(string $new_id [, bool $force_id=FALSE]);

This was changed as Stas(?) suggested since I agree to him. It would be
more secure
than old one.


> > […]
>
> Why is that necessary at all? `session_regenerate_id()` not sufficient?
> Either way, this shouldn't be a part of this RFC.
>
> It's your RFC, but still - it's about changing default settings for
> improved security, not new features.
>

You are ignoring the fact that there is no  'default' function that is
securely generates random bytes/ID.

There are code that requires prefix for much efficient session data
handling.
For example, user may want to know session creation time/owner/etc
*without reading* session data. With prefixed session ID, creation
time/owner/etc
is known just selecting session ID names. For databases like PostgreSQL
which
supports function index, searching specific session is done in msec order
even
when there are millions of session IDs.


>
> > […]
>
> We've obviously got a language barrier here.
> All I'm suggesting is that you improve the *wording* in your RFC, so
> that it is clear why hash_bits_per_character is changed.
>

Thank you for the suggestion.
I'll try to improve.


>
> > […]
> number
> > […]
>
> Because, as I said above - I thought this RFC was about changing
> default settings and nothing else.
> And again, an underscore *is not* a part of the hash, regardless of
> hash_bits_per_character.
>
> I personally don't care about the prefix. It's just that it's a
> completely new feature that has nothing to do with the default
> settings and it's just confusing that you mention it in the same
> context. :)
>

It's new for PHP, but there are codes do this and many of them are
insecure...
I would like to eliminate insecure code by introducing easy to use API also.

I might have chosen wrong name for the RFC.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

---

## #72194 — Andrey Andreev — 2014-02-04T12:59:33+00:00

URL: https://externals.io/message/72049#72194 · In reply to: #72155

> > […]
>
> You may be referring old RFC. Current RFC is
>
> Add optional $force_id parameter for `session_id()` to allow any id regardless
> of this mode.
>
>   bool session_id(string $new_id [, bool $force_id=FALSE]);
>
> This was changed as Stas(?) suggested since I agree to him. It would be more
> secure
> than old one.

I am not referring to an old RFC, I'm saying this parameter is pointless.
What difference does it make at all? How does `session_id()` work with
and without it?

> > […]
>
>
> You are ignoring the fact that there is no  'default' function that is
> securely generates random bytes/ID.
>
> There are code that requires prefix for much efficient session data
> handling.
> For example, user may want to know session creation time/owner/etc
> *without reading* session data. With prefixed session ID, creation
> time/owner/etc
> is known just selecting session ID names. For databases like PostgreSQL
> which
> supports function index, searching specific session is done in msec order
> even
> when there are millions of session IDs.

I'm not ignoring anything, I'm just saying this should be in a separate RFC.

> > […]
>
>
> It's new for PHP, but there are codes do this and many of them are
> insecure...
> I would like to eliminate insecure code by introducing easy to use API also.
>
> I might have chosen wrong name for the RFC.

See my previous comment above.
You didn't chose a wrong name, you seem to have done a 2-in-1 RFC and
that's not the right approach IMO. At the very least - it's obviously
confusing and most of what you've said in this conversation isn't
described in the RFC at all.

Cheers,
Andrey.

---

## #72225 — Yasuo Ohgaki — 2014-02-05T00:13:42+00:00

URL: https://externals.io/message/72049#72225 · In reply to: #72194

Hi Andrey,



> > […]
> regardless
> > […]
> more
> > […]
>
> I am not referring to an old RFC, I'm saying this parameter is pointless.
> What difference does it make at all? How does `session_id()` work with
> and without it?
>

Original proposal was

session_id('SOME_USEFUL_PREFIX' . `session_create_id()`);

and allow to set ID. New one should be

  bool session_id(string $new_id_or_prefix [, bool $use_prefix=FALSE]);

(I thought I've updated the signature. It was old...)
It supposed to used like

session_id('PREFIX_', TRUE) // retunrs PREFIX_SECURE-SID

New API would not allow insecure session ID when use_strict_mode=on while
allowing user defined prefix for session ID.


>
> > […]
>
> I'm not ignoring anything, I'm just saying this should be in a separate
> RFC.
>
>
I have too many RFCs already...
I would like to reduce number of RFCs.


> > […]
> also.
> > […]
>
> See my previous comment above.
> You didn't chose a wrong name, you seem to have done a 2-in-1 RFC and
> that's not the right approach IMO. At the very least - it's obviously
> confusing and most of what you've said in this conversation isn't
> described in the RFC at all.


To me, it's the same objective that make session usage as secure as
possible.
I don't see reason to make 2 patches/RFC for making session module secure.
Perhaps, better title would be

"Make session options/API as secure as possible"

Regarding "_" addition to files save handler, it may not be RFC issue as it
does not break anything at all. Just an simple addition of safe char that
is needed for new safe prefixed session ID with hash bits=6. It may apply
even prefixed session. I think there are many changes like this w/o RFC.

I tried to write RFC to be minimum and sufficient. I should add more
description
if it is not. Or add link of this thread. I think it's preferred way.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

---

## #72288 — Andrey Andreev — 2014-02-05T20:00:52+00:00

URL: https://externals.io/message/72049#72288 · In reply to: #72225

Hi again,

> > […]
>
>
> Original proposal was
>
> session_id('SOME_USEFUL_PREFIX' . `session_create_id()`);
>
> and allow to set ID. New one should be
>
>   bool session_id(string $new_id_or_prefix [, bool $use_prefix=FALSE]);
>
> (I thought I've updated the signature. It was old...)
> It supposed to used like
>
> session_id('PREFIX_', TRUE) // retunrs PREFIX_SECURE-SID
>
> New API would not allow insecure session ID when use_strict_mode=on while
> allowing user defined prefix for session ID.

You're not getting what I mean at all.

use_strict_mode should only validate cookie input, that's what it is meant for:

> > […]
>
> I have too many RFCs already...
> I would like to reduce number of RFCs.

Would you like it if people vote 'No' on this RFC because they like
the setting changes, but not the proposed `session_id()` parameter?
That's how I would vote if I could. :)

> > […]
>
>
> To me, it's the same objective that make session usage as secure as
> possible.
> I don't see reason to make 2 patches/RFC for making session module secure.
> Perhaps, better title would be
>
> "Make session options/API as secure as possible"
>
> Regarding "_" addition to files save handler, it may not be RFC issue as it
> does not break anything at all. Just an simple addition of safe char that
> is needed for new safe prefixed session ID with hash bits=6. It may apply
> even prefixed session. I think there are many changes like this w/o RFC.
>
> I tried to write RFC to be minimum and sufficient. I should add more
> description
> if it is not. Or add link of this thread. I think it's preferred way.

    Changing default settings in the proposed way makes ext/session
more secure by default.

    Adding a new parameter to `session_id()` only gives users an easier
way to do complete a task that they otherwise *could* do the wrong
way.

The first has real, straight-forward impact on security and doesn't
change existing functionality.
The second only *might* lead to some userland code being more secure
and it is questionable if that's the proper tool for the job. I for
one would like more tools that allow me to change a session's
behavior, but a prefix is not one of them.

It's not the same thing, not even the same category.

On the '_' character:

Any character passed to `session_id()` should be allowed, depending on
hash_bits_per_character and just for the underscore doesn't sound good
to me.

Cheers,
Andrey.

---

## #72299 — Yasuo Ohgaki — 2014-02-05T21:58:44+00:00

URL: https://externals.io/message/72049#72299 · In reply to: #72288

Hi Andrey,



> > […]
> secure.
> > […]
> it
> > […]
>
>     Changing default settings in the proposed way makes ext/session
> more secure by default.
>
>     Adding a new parameter to `session_id()` only gives users an easier
> way to do complete a task that they otherwise *could* do the wrong
> way.
>
> The first has real, straight-forward impact on security and doesn't
> change existing functionality.
> The second only *might* lead to some userland code being more secure
> and it is questionable if that's the proper tool for the job. I for
> one would like more tools that allow me to change a session's
> behavior, but a prefix is not one of them.
>

If you handle millions of sessions and would like to find specific
active sessions with marginal overhead, prefixing is the way to
go. Many users may not need it, but there are users who need.

Currently, there is no easy way to get secure random bytes. Even if
there is, session module is better to have it's own secure ID generator.


> It's not the same thing, not even the same category.
>
> On the '_' character:
>
> Any character passed to `session_id()` should be allowed, depending on
> hash_bits_per_character and just for the underscore doesn't sound good
> to me.


Since we have transid, some chars are not allow by session module

    /* Finally check session id for dangarous characters
     * Security note: session id may be embedded in HTML pages.*/
    if (PS(id) && strpbrk(PS(id), "\r\n\t <>'\"\\")) {

Save handlers may have stricter limitations. Files is the one, since it
saves
data as file, only safe chars(white list) are allowed.

User may set any ID via `session_id()`, but session module does not accept
any dangerous session ID and creates new secure ID with use_strict_mode=On.

Anyway, I don't mind removing things that aren't related to INI.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

---

## #73554 — Andrey Andreev — 2014-04-03T09:31:58+00:00

URL: https://externals.io/message/72049#73554 · In reply to: #72299

Hi,

> > […]
>
>
> If you handle millions of sessions and would like to find specific
> active sessions with marginal overhead, prefixing is the way to
> go. Many users may not need it, but there are users who need.

Or, you could prefix (or add another field to check against) it in
storage, but leave the session ID itself untouched. That's not the
point though ... this just isn't a security feature and the RFC is
about improving security.

Can we move this forward now? I don't think there's anything more to discuss.
Btw, I'm still a proponent of changing hash_bits_per_character as
well, but IMO that may be done separately, without an RFC.

Cheers,
Andrey.

---

## #73592 — Yasuo Ohgaki — 2014-04-04T09:14:07+00:00

URL: https://externals.io/message/72049#73592 · In reply to: #73554

Hi all,



>
> > […]
> as
> > […]
> RFC.
> > […]
>
> Or, you could prefix (or add another field to check against) it in
> storage, but leave the session ID itself untouched. That's not the
> point though ... this just isn't a security feature and the RFC is
> about improving security.
>
> Can we move this forward now? I don't think there's anything more to
> discuss.
> Btw, I'm still a proponent of changing hash_bits_per_character as
> well, but IMO that may be done separately, without an RFC.


Sure.
These are simple changes for better session security.
I have to update RFC so that everyone understand side effects of
these changes.

hash_bits_per_characters may stay the same and additional char to
files save handler could be added simply.
I'll update the RFC weekend, hopefully.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

---

## #74127 — Yasuo Ohgaki — 2014-05-12T08:13:33+00:00

URL: https://externals.io/message/72049#74127 · In reply to: #73592

Hi all,



> Sure.
> These are simple changes for better session security.
> I have to update RFC so that everyone understand side effects of
> these changes.
>
> hash_bits_per_characters may stay the same and additional char to
> files save handler could be added simply.
> I'll update the RFC weekend, hopefully.
>

I updated the RFC. Sorry, it took so long.
I modified the RFC so that it only proposes INI value changes.
i.e. Removed behavior modifications "hash function fall back" and
"session ID collision detection in session module rather than save handler".

https://wiki.php.net/rfc/secure-session-options-by-default

Which version should include these?

Any comments?

--
Yasuo Ohgaki
yohgaki@ohgaki.net