Everything

php

[VOTE] Secure Session Module Options/Internal by Default

php.internals

Yasuo Ohgaki

12 years ago

Hi all,

This RFC changes default session settings and introduces a new
setting that disables possible timing attack against session ID. All
of them help to improve general session ID security except
hash_bits_per_character change.

NIST discourages use of SHA-1 years ago. It proposes to use
SHA-256 as the default hash function for session. To reduce
size of session ID string, hash_bits_per_character=6 is proposed.

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

Thank you for voting!

P.S. Although, the change is trivial, if anyone would like to see
patch for this, I'll prepare one ASAP.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Damien Tournoud

12 years ago

Hi there,

I remember that the `use_strict_mode=on` added in PHP 5.5 was broken and
would prevent the application from setting a session ID by itself. This is
actually fairly common, about 800k Drupal 7 websites do that.

Unless this is fixed, this setting just cannot be enabled by default.

Damien Tournoud

On Mon, Feb 17, 2014 at 5:27 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

Yasuo Ohgaki

12 years ago

Hi Damien,

On Tue, Feb 18, 2014 at 7:38 PM, Damien Tournoud <damz@damz.org> wrote:

> I remember that the `use_strict_mode=on` added in PHP 5.5 was broken and
> would prevent the application from setting a session ID by itself. This is
> actually fairly common, about 800k Drupal 7 websites do that.
>
> Unless this is fixed, this setting just cannot be enabled by default.
>

It was simple mistake during merge. It is fixed next release already. Do
not worry about it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Tjerk Meesters

12 years ago

On Tue, Feb 18, 2014 at 6:49 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi Damien,
>
> On Tue, Feb 18, 2014 at 7:38 PM, Damien Tournoud <damz@damz.org> wrote:
>
> > I remember that the `use_strict_mode=on` added in PHP 5.5 was broken and
> > would prevent the application from setting a session ID by itself. This
> is
> > actually fairly common, about 800k Drupal 7 websites do that.
> >
> > Unless this is fixed, this setting just cannot be enabled by default.
> >
>
> It was simple mistake during merge. It is fixed next release already. Do
> not worry about it.
>

It seemed to be part of the patch of the respective rfc:

https://gist.github.com/yohgaki/1379668#file-php-trunk-strict-session-patch-L602

How was this a merge issue?

>
> Regards,
>
> --
> Yasuo Ohgaki
> yohgaki@ohgaki.net
>

--
--
Tjerk

Yasuo Ohgaki

12 years ago

Hi Tjerk,

On Tue, Feb 18, 2014 at 7:56 PM, Tjerk Meesters <tjerk.meesters@gmail.com>wrote:

> It seemed to be part of the patch of the respective rfc:
>
>
> https://gist.github.com/yohgaki/1379668#file-php-trunk-strict-session-patch-L602
>
> How was this a merge issue?
>

Could you point out what's the exact issue here?
It's gist and it's not the patch merged.

tests/session_id_basic.phpt

var_dump(session_id());
var_dump(session_id("test"));
var_dump(session_id());
var_dump(session_id("1234567890"));
var_dump(session_id());
// Turn off strice mode, since it does not allow uninitialized session ID
ini_set('session.use_strict_mode',false);
var_dump(session_start());
var_dump(session_id());
var_dump(session_destroy());
var_dump(session_id());

echo "Done";
ob_end_flush();
?>
--EXPECTF--
*** Testing session_id() : basic functionality ***
string(0) ""
string(0) ""
string(4) "test"
string(4) "test"
string(10) "1234567890"
bool(true)
string(10) "1234567890"
bool(true)
string(0) ""
Done

It works like this. Is there issue with this behavior?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Tjerk Meesters

12 years ago

On Tue, Feb 18, 2014 at 7:09 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:

> Hi Tjerk,
>
> On Tue, Feb 18, 2014 at 7:56 PM, Tjerk Meesters <tjerk.meesters@gmail.com>wrote:
>
>> It seemed to be part of the patch of the respective rfc:
>>
>>
>> https://gist.github.com/yohgaki/1379668#file-php-trunk-strict-session-patch-L602
>>
>> How was this a merge issue?
>>
>
> Could you point out what's the exact issue here?
> It's gist and it's not the patch merged.
>

In that case, the RFC should be updated with the corresponding commit logs
(part of the process) when it was merged with the project; this makes it
easier to discuss historical commits based on it.

> tests/session_id_basic.phpt
>
> var_dump(session_id());
> var_dump(session_id("test"));
> var_dump(session_id());
> var_dump(session_id("1234567890"));
> var_dump(session_id());
> // Turn off strice mode, since it does not allow uninitialized session ID
> ini_set('session.use_strict_mode',false);
> var_dump(session_start());
> var_dump(session_id());
> var_dump(session_destroy());
> var_dump(session_id());
>
> echo "Done";
> ob_end_flush();
> ?>
> --EXPECTF--
> *** Testing session_id() : basic functionality ***
> string(0) ""
> string(0) ""
> string(4) "test"
> string(4) "test"
> string(10) "1234567890"
> bool(true)
> string(10) "1234567890"
> bool(true)
> string(0) ""
> Done
>
> It works like this. Is there issue with this behavior?
>

That looks okay, thanks for getting back to me :)

>
> Regards,
>
> --
> Yasuo Ohgaki
> yohgaki@ohgaki.net
>
>

--
--
Tjerk

Yasuo Ohgaki

12 years ago

Hi Tjerk,

On Tue, Feb 18, 2014 at 8:58 PM, Tjerk Meesters <tjerk.meesters@gmail.com>wrote:

> In that case, the RFC should be updated with the corresponding commit logs
> (part of the process) when it was merged with the project; this makes it
> easier to discuss historical commits based on it.

Thank you for point it out.
I'll update the RFC.  If document is not updated, I'll update it, too.

Regarding session_id() behavior, it could be made to accept user specified
session ID independent of "use_strict_mode". To accept user specified
session ID when "use_strict_mode=on", there should be an API that generates
secure session ID. Otherwise, user may create insecure ID and set it.

Any call for RFC, anyone? I'm willing to propose this.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Zeev Suraski

12 years ago

Yasuo,

This was previously discussed but I have to agree with Andrey (and maybe
even go beyond what he said) - the hash_bits change doesn't belong in this
RFC.  First, it has no security implications and it's not entirely clear
from the RFC.  Second, I don't feel that the implications of that change are
clear, beyond some mention that "this could not be an issue for almost all
apps", which personally I don't think is accurate - but either way, we need
some better analysis here.

From my point of view I don't think a few extra characters per session going
over the wire are worth the potential obscure BC break changing the default
here will cause with certain session backends and/or apps, and we shouldn't
include this change in this RFC.  If we want to compact the session id's,
proposing a change for this default can be done in a separate RFC that
discusses the pros and cons of doing it, independent of security.

Another thing that I think this RFC is missing is some clearer explanation
on what kind of apps *don't* work with the proposed changes (most notably
use_strict_mode=on and cookie_httponly).  Even though my gut is that these
two proposed changes are good - the RFC should include explanations of the
code patterns and/or types of apps and/or modules that will be affected by
this;  "Most apps should not be affected" isn't enough in an RFC IMHO.

Last - the voting period should be at least a week, right now it's 5 (or
maybe 6, depending on your POV) days.

Thanks!

Zeev

Yasuo Ohgaki

12 years ago

Hi Zeev,

On Tue, Feb 18, 2014 at 8:46 PM, Zeev Suraski <zeev@zend.com> wrote:

> This was previously discussed but I have to agree with Andrey (and maybe
> even go beyond what he said) - the hash_bits change doesn't belong in this
> RFC.  First, it has no security implications and it's not entirely clear
> from the RFC.  Second, I don't feel that the implications of that change
> are
> clear, beyond some mention that "this could not be an issue for almost all
> apps", which personally I don't think is accurate - but either way, we need
> some better analysis here.
>
> From my point of view I don't think a few extra characters per session
> going
> over the wire are worth the potential obscure BC break changing the default
> here will cause with certain session backends and/or apps, and we shouldn't
> include this change in this RFC.  If we want to compact the session id's,
> proposing a change for this default can be done in a separate RFC that
> discusses the pros and cons of doing it, independent of security.
>

I agree. This would be removed. This means vote is closed and reopened
again.

I'm not sure why compiled and php.ini-* default differs now, but it's
irrelevant issue here anyway.

Another thing that I think this RFC is missing is some clearer explanation

> on what kind of apps *don't* work with the proposed changes (most notably
> use_strict_mode=on and cookie_httponly).  Even though my gut is that these
> two proposed changes are good - the RFC should include explanations of the
> code patterns and/or types of apps and/or modules that will be affected by
> this;  "Most apps should not be affected" isn't enough in an RFC IMHO.
>

I'll add explanations and possible side effects. I think of few.

>  Last - the voting period should be at least a week, right now it's 5 (or
> maybe 6, depending on your POV) days.
>

This is mistake. My apologies.

Thank you for your comment.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net