Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:72785
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.175 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAPhkiZy=c2a6d+0XuQ2Ypteb7UeA1Ue0gfC2SejKN4m_YpP_eQ@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>
	<CAPhkiZy=c2a6d+0XuQ2Ypteb7UeA1Ue0gfC2SejKN4m_YpP_eQ@mail.gmail.com>
Date: Mon, 24 Feb 2014 10:31:57 +0100
Message-ID: <CAH-PCH64bqu6_igvLvMsedC7j1J0HEJPrdMGG-eD4JaLO9vaGw@mail.gmail.com>
To: Yasuo Ohgaki <yohgaki@ohgaki.net>, Andrey Andreev <narf@devilix.net>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c2fcca95cbb404f323a4dd
Subject: Re: [PHP-DEV] [VOTE] Secure Session Module Options/Internal by Default
From: tyra3l@gmail.com (Ferenc Kovacs)

--001a11c2fcca95cbb404f323a4dd
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 21, 2014 at 12:25 PM, Andrey Andreev <narf@devilix.net> wrote:

> 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 - whe=
re
> >> > 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 ide=
a.
>
> 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 kee=
p
> > 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 trivi=
al
> > 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 i=
n
> >> > 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 almo=
st
> >> > 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 functio=
n
> > fallback is took into account.
> >  - User may have variable length for prefix/postfix, the setting could =
be
> > interval or minimum. Perhaps, id_additional_chars=3D16,32 for 16 to 32
> chars?
> > id_additional_chars=3D0 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.
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
Hi,

any update on this?
today would be the last day of the voting, but given that there are still
active discussion/open issues and given the fact that nobody voted yet, I
think it would be nice to either move back the RFC to under discussion, or
at least extend the voting phase.

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--001a11c2fcca95cbb404f323a4dd--