Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:95104
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain ohgaki.net designates 180.42.98.130 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <cc719c6c-598d-b103-96bf-eb3714a61365@thefsb.org>
References: <CAGa2bXaZhfXbU6N0-JGVPzkkxRdkcjEhkHP+YrQNH=XFX714-Q@mail.gmail.com>
 <CAPwsocFAMofROQE-OBfNiR4yrKKSss_owu0svu+Db66DmRbShw@mail.gmail.com>
 <CAGa2bXaxRA91q+ev-X3okKpNC=s3XEwC9deUYikTaUQPfOE-JQ@mail.gmail.com> <cc719c6c-598d-b103-96bf-eb3714a61365@thefsb.org>
Date: Sat, 13 Aug 2016 05:38:34 +0900
Message-ID: <CAGa2bXYNExeQLug=co8G1g+fdy6BXK9UbeyGkjss5s62R00Fdg@mail.gmail.com>
To: Tom Worster <fsb@thefsb.org>
Cc: Leigh <leight@gmail.com>, "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] [RFC][VOTE] Add session_create_id() function
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi Tom,

On Sat, Aug 13, 2016 at 3:11 AM, Tom Worster <fsb@thefsb.org> wrote:
> On 8/11/16 6:58 PM, Yasuo Ohgaki wrote:
>>
>> Hi Leigh,
>>
>> On Fri, Aug 12, 2016 at 3:25 AM, Leigh <leight@gmail.com> wrote:
>>>
>>> On Wed, 10 Aug 2016 at 10:15 Yasuo Ohgaki <yohgaki@ohgaki.net> wrote:
>>>>
>>>>
>>>> Hi all,
>>>>
>>>> This is RFC for adding session_create_id() function.
>>>>
>>>> Session ID string uses special binary to string conversion. Users
>>>> should write lengthy and slow code to have the same session ID string
>>>> as session module does.
>>>
>>>
>>>
>>> I disagree, this pretty much covers it:
>>>
>>> function session_create_id()
>>> {
>>> $encoded = base64_encode(random_bytes(random_bytes(32)));
>>> // Use same charset as PHP
>>> return rtrim(strtr($encoded, '+/', ',-'), '=');
>>> }
>>
>>
>> Thank you for insight!
>>
>> You've missed to set SID to proper length and SID validation.
>
>
> Replacing rtrim() with substr() is fixes that.

No.

Base 64 adds padding for extra bytes. The Padding char is "=" and it's
illegal char as SID. Therefore trims is mandatory.

Don't you think it's nice to make "PHP just works" without knowing such details?

>
>
>> function session_create_id(string $prefix)
>> {
>>     $encoded = base64_encode(ini_get('session.sid_length')*2);
>
>
> Did you omit random_bytes() in this line?

I don't why I did this :)
I thought I replaced "32" to ini setting.

>
>
>>     // Use same charset as PHP
>>     $sid = substr(rtrim(strtr($encoded, '+/', ',-'), '='), 0,
>>                           ini_get('session.sid_length');
>>
>>     $sid .= $prefix;
>>
>>     // Now validate SID so that it does not have collisions
>>     when session is active, connect to database and validate SID
>>       try to fetch sid
>>         if sid is not there
>>           try again to generate SID few times
>>       if SID validation failed
>>          fatal error
>>       return safe SID
>>    when session is inactive
>>       return unvalidated SID
>> }
>>
>> This is what proposed session_create_id() does.
>> I used pseudo, but it should be easy to imagine it would be lengthy code.
>
>
> You don't need to waste time checking for collisions if the SID has a random
> component of sufficient length. 32 random base-64 characters is sufficient.
>
> There are lots of purposes for random strings with negligible chance of
> collision. Hence some frameworks provide the function, e.g.
> http://www.yiiframework.com/doc-2.0/yii-base-security.html#generateRandomString()-detail
>
> Only the search of the the session database for collisions seems hard to me.
> But I don't understand why it is needed.

Session ID security is key factor of Web application security. There
is huge difference between 'cannot happen' and 'very rare and almost
cannot happen'. In addition, proving 'very rare and almost cannot
happen' is difficult.

NIST requires SHA2 or better hash for collision sensitive usage. SHA2
has 256 bits or more. We only use 128 bits, i.e. MD5 and
bits_per_character=5 now. (130 bits for 7.1 and later). Therefore, I
don't think collision check is needless. We should have 256 bits
session ID at least which requires 52 chars with 5 bits_per_character.

We know it's relatively easy to check if a PRNG meets NIST
requirement, but requirement fulfillment does not mean PRNG is
generating excellent random. Measuring quality of PRNG is difficult.
(This does not mean we should add low quality entropy such as time and
pid to create SID and hash it. We depends on PRNG security anyway, use
of hash and low quality entropy only makes SID weaker. Thus I proposed
raw PRNG usage for SID)

These things make me think collision detection is mandatory to say
"PHP session is secure".

Anyway, I'm not a cryptographer and just following their advices. I
suggest you do the same.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net