Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:72705
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain devilix.net designates 209.85.213.68 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAGa2bXbeGGFHJe8t7=9yDpuODWRq9+eVSeQ339j=1gzd4x9wGA@mail.gmail.com>
References: <CAPhkiZxqmUVR3MhdMUBUuKxxkXndoTdoffbfuQ0oGc5JMvQ8dg@mail.gmail.com>
	<CAGa2bXbBnPQHqBUxe=S3HZtvQHQLdomEM4CEF3F+O0mf4oaktA@mail.gmail.com>
	<CAPhkiZx2sFqDmvdAsxOqqcrN6P09HW7MzFOA0cZbZ_2ruN4izw@mail.gmail.com>
	<CAGa2bXZbiTk_a=aJJY4iUPPaNQbv0yhyU2t6dXv2hAo0atiTdA@mail.gmail.com>
	<5302ABF6.60407@sugarcrm.com>
	<CAGa2bXbeGGFHJe8t7=9yDpuODWRq9+eVSeQ339j=1gzd4x9wGA@mail.gmail.com>
Date: Thu, 20 Feb 2014 15:03:49 +0200
Message-ID: <CAPhkiZx_sff8AcucRpHxb=f2w2DxKHt7SrOjQ1oTomrSa0oo-Q@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] [VOTE] Secure Session Module Options/Internal by Default
From: narf@devilix.net (Andrey Andreev)

Hi again,

> Length check is trivial, then why not check  and discard by module instead of accepting invalid session ID and let users check and discard?

It is trivial, but also arguably useful. The reasoning for any
addition should be "what's the benefit" instead of "it's trivial, so
why not".

> There is threat and it could be removed with length check.
> Let's have the check and forget about timing attack!

Consider this scenario:

 - Attacker accesses the site without sending a session cookie
 - Site responds, giving them a newly generated session ID (this is
where length becomes public).
 - Attacker doesn't care about your length check, because instead of
sending <first char(s)>, they can now send str_pad(<first char(s)>,
<received session ID length>, '0')

Anybody with the knowledge and resources to perform a successful
timing attack will bypass the length check without even knowing that
there is one. Bottom line is, length check will NOT prevent a timing
attack.
Also, you didn't actually answer to my other comments, this is what I said:

>> This still doesn't explain how it would work.

Rephrasing: the RFC barely describes what id_length does and the name
itself isn't descriptive.

>> And why is it necessary as a setting instead of auto-detecting it? The
>> valid length should be easy to calculate.

Rephrasing: Don't make it a setting. Decide the (exact) length based
on the hash_function and hash_bits_per_character settings. You simply
don't need this to be configurable, and exposing it as an ini setting
can only cause issues for inexperienced users who change it.

Stas raised the same concern:

> I could see it as an internal check - defensive coding and all - where length is pre-calculated and checked internally. This is fine - but as a user setting that inevitably will be set wrong and cause trouble - I just don't see the point.

> The reason why I proposed this as user setting is user may have prefixed session ID. If this is the case, internally set minimum length could be useless.

Why is it that you use this prefix as an argument for everything? This
isn't even a feature supported by PHP, it's a possible userland
implementation.

>> Why would they send short ID if they could send full-length ID and
>> easily defeat your checks?
>>
>
>3 reasons.
> - Hash used by session may fallback to SHA-1 from SHA-256

What?! Why would that happen?

> - User may set hash to SHA-1 or md5 by mistake or intentionally

Again, the length should be decided based on the setting. How the user
configures it is not your problem.

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.

After the lengthy discussion, I think the real problem is the lack of
clarity about this in the RFC.
If the hash_function is changed to one producing a longer length
string, then increasing hash_bits_per_character would complement that
change to somehow maintain BC.

For example, a custom session save handler using a database will most
likely have a varchar(40) field for the session_id. Switching to
sha256 and not changing hash_bits_per_character or the field's maximum
length will cause trouble.

IMO, this just needs to be properly described in the RFC.

Btw, the "Backward Incompatible Changes" section currently says
use_SCRIPT_mode instead of 'strict' - needs a fix as well. :)

Cheers,
Andrey.