Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:72730
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain devilix.net designates 209.85.213.51 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAGa2bXZVNaYFMR13m8WBv+-S2Yojv5Smg4dxqgQoVmCy3PXoOg@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>
	<CAPhkiZx_sff8AcucRpHxb=f2w2DxKHt7SrOjQ1oTomrSa0oo-Q@mail.gmail.com>
	<CAGa2bXZVNaYFMR13m8WBv+-S2Yojv5Smg4dxqgQoVmCy3PXoOg@mail.gmail.com>
Date: Fri, 21 Feb 2014 13:25:24 +0200
Message-ID: <CAPhkiZy=c2a6d+0XuQ2Ypteb7UeA1Ue0gfC2SejKN4m_YpP_eQ@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>
Cc: "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 Yasuo,

On Thu, Feb 20, 2014 at 11:17 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
> Hi Andrey,
>
> On Thu, Feb 20, 2014 at 10:03 PM, Andrey Andreev <narf@devilix.net> wrote:
>>
>> > 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')
>
>
> I'm not trying to hide length at all. What I'm trying to do is "do not
> compare strings up to id_length, so that attacker cannot analyze secret
> session ID char by char using timing up to id_length".
>
> The rest of chars (chars larger than id_length) can be analyzed by timing.
> However, unless attacker could guess id_length part of secret, it's useless.

I feel like you're not reading a word of my arguments.

Nobody has to guess the id length, as my quote below says: an attacker
experienced enough to do timing attacks will always send a string with
the full length.
They do not have to know any secret, nor would they care if one
exists. Your "id length" check will only "protect" you from a 13-year
old using some brute-force script that they just found.

>> 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.
>
>
> The reason why it is not automatic is user could add prefix to session ID.
> When user uses prefix, automatic calculation may disable timing protection.
> However, your proposal of automatic calculation is better.
>
>>
>>
>> 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.
>
>
> We may be better to have "id_additional_chars" instead of "id_length(minimum
> ID length)", then automatic calculation can be done. This is better idea.

It's not, a proper name would be something like "id_check_min_length".

>> > 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.
>
>
> We cannot ignore valid user implementations. Session ID prefixing is useful
> for large sites to determine user of the session without additional storage
> look up, for example. Users do not have to have separate storage to keep
> session ID and user ID relation with this. Keeping them in sync is not
> needed, too. Sessions are deleted by GC. Keep them in sync is not trivial
> unless there is special session save handler.
>
> <user-id-hash>-<session-id-hash>
>
> Many databases can perform efficient search for string prefix.
>
> There are users who use this. They have millions of users for each.

You also can't build your whole logic around a single (and not as
widely used as you think) possible user implementation. And while I
can see how prefixing a session ID could be useful, a more effective
approach would be using the session name (cookie name) instead of a
prefix.

Also, the example you're giving is absurd. A user_id should be stored
either in session data or as a separate database field in the same
table and fetched at the time of session ID validation.

>> >> 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?
>
>
> ext/hash could be DL module. There is #if for this case.

This is a more serious flaw than the one you're trying to protect
against. I'd suggest one of the following:

 - Force ext/hash to be dynamically loaded when session wants to use it.
 - Not give the ability to disable the extension at all.
 - Emit E_WARNING when a user tries to use an unavailable algorithm.

Actually, the last one should be done anyway, IMO.

>>
>>
>> > - 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.
>
>
> Agreed. We should be better to have "id_additional_chars" (or better name),
> then problem is solved.

I'm confused, you can't both agree and suggest a different name.
Maybe I wasn't clear enough here, but the "the setting" I'm talking
about is hash_function+hash_bits_per_character.

>> 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.
>
>
> Yes. If user have trouble with new default, they should be able to use old
> setting of their own. Changing database schema is headache.
>
>>
>>
>> IMO, this just needs to be properly described in the RFC.
>
>
> I'll add this.
>
>>
>>
>> Btw, the "Backward Incompatible Changes" section currently says
>> use_SCRIPT_mode instead of 'strict' - needs a fix as well. :)
>
>
>
>  - hash_bits_per_character will be removed from the RFC.
>  - Possible issue with ID length change will be documented.
>  - "id_length"(Minimum session ID length) will be
> "id_additional_chars"(Session ID prefix length set by users. Default to 0)
> and session ID length is calculated by settings. Automatic hash function
> fallback is took into account.
>  - User may have variable length for prefix/postfix, the setting could be
> interval or minimum. Perhaps, id_additional_chars=16,32 for 16 to 32 chars?
> id_additional_chars=0 by default. This setting could negate timing
> protection. This will be documented.
>  - Spelling mistake will be fixed. If anyone notice any, please let me know
> and/or fixing them in wiki is appreciated. Thanks :)

You're again contradicting yourself.

> It improved a lot. Are we okay with session ID length issue now?
> Better proposals are appreciated.

No, you just both agreed to and then completely ignored my suggestions
in your "solution".
That's not an improvement, it's just confusion.

Cheers,
Andrey.