Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:90970
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
To: "'Yasuo Ohgaki'" <yohgaki@ohgaki.net>,
	"'Stanislav Malyshev'" <smalyshev@gmail.com>
Cc: "'Remi Collet'" <remi@fedoraproject.org>,
	<internals@lists.php.net>,
	"'Yasuo Ohgaki'" <yohgaki@php.net>
References: <03a501d15439$fcbf9ca0$f63ed5e0$@php.net> <56A1054A.5080102@fedoraproject.org> <CAGa2bXaKaoiSKLpodHAEKurSEwrs9rYO25uwqGdbwg_KKUkGbg@mail.gmail.com> <56A2069B.2050007@fedoraproject.org> <56A21D68.6030403@fedoraproject.org> <56A825A9.9020706@gmail.com> <CAGa2bXYf9JFtH_yqjYfL7UiqDCzxmeFy9s-Ja4YPF+UjxDs3=w@mail.gmail.com> <56A85967.4090603@gmail.com> <CAGa2bXb_3eM2EaM=Zt2Mf-pLnGDvgek2M87O_7mL5f6HLp3s2w@mail.gmail.com>
In-Reply-To: <CAGa2bXb_3eM2EaM=Zt2Mf-pLnGDvgek2M87O_7mL5f6HLp3s2w@mail.gmail.com>
Date: Wed, 27 Jan 2016 09:06:24 +0100
Message-ID: <01c101d158d9$a0533450$e0f99cf0$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQHsl++e30t/kLKxuC36hZ0/YzeBFQIq8uRuAYbF2n8BtZ3tHgEw+119AcPWaWACq7463QJG1BqXAfA9AHSeXoWxMA==
Content-Language: en-us
Subject: RE: [PHP-DEV] PHP 7.0.3 RC1 is available for testing - **** BC break ***
From: anatol.php@belski.net ("Anatol Belski")

Hi Yasuo,

> -----Original Message-----
> From: yohgaki@gmail.com [mailto:yohgaki@gmail.com] On Behalf Of Yasuo
> Ohgaki
> Sent: Wednesday, January 27, 2016 7:26 AM
> To: Stanislav Malyshev <smalyshev@gmail.com>
> Cc: Remi Collet <remi@fedoraproject.org>; internals@lists.php.net; =
Yasuo
> Ohgaki <yohgaki@php.net>
> Subject: Re: [PHP-DEV] PHP 7.0.3 RC1 is available for testing - **** =
BC break ***
>=20
> Hi Stas,
>=20
> On Wed, Jan 27, 2016 at 2:45 PM, Stanislav Malyshev =
<smalyshev@gmail.com>
> wrote:
> >> Note for this issue.
> >> The change does not breaks normal codes as PHP cannot set new =
session
> >> ID when header is already sent. The session is _not_ accessible
> >> anyway. Not writing orphaned session does not matter at all.
> >
> > So it looks like this particular breakage is because write does not
> > happen when cookies could not be sent. However, it is not =
necessarily
> > true that session is not accessible in this case - session ID can be
> > passed by other means, not only via cookies, and it also may be
> > existing session ID. The second part of this test opens session with
> > known ID, but write still does not happen.
> > I think assuming that if cookie sending failed then the whole =
session
> > failed is dangerous. Also, removing session write is a pretty =
serious
> > change, I'm not sure this should be happening in a stable version.
>=20
> It's correct. I ignored trasid because of severe security bug in it =
that you should
> know of. (This is for others. We've been talking about the fix off =
this list. Please
> don't use transid or use it with extreme care with URL outputs. It's =
broken for a
> long time.)
>=20
> So correct behavior would be if transid is enabled and =
use_only_cookies is
> disabled, write session data. So the code should be
>=20
> if (!PS(use_only_cookies)) {
>   php_session_abort()
> }
>=20
> Good point!
>=20
> >> If you understand exactly what is happening, it is understandable
> >> this is not a BC for production code, but only breaks test code =
that
> >> inspect session behaviors and it found out bogus write() is gone
> >> now.> Anyway, to fix existing app/framework unit test failures, =
(it's
> >> not fixing anything but leave bogus write(), though) removing
> >> php_session_abort() from php_session_cache_limitter() should be =
enough.
> >
> > Yes, this patch:
> > https://gist.github.com/smalyshev/4d8435b7993bef80c0ed
> > fixes it for me. Remi, could you check that it also fixes the =
failures
> > in the test suites?
>=20
> Thank you for verifying it.
>=20
> So question would be if we are going to revert part that breaks unit =
tests (or
> even discard all patch that returns correct session_start() return =
value and save
> handler abuse crashes), or add necessary "if"
> statement to return correct value. One could argue if cookie cannot be =
sent, it
> should fail, but I think it's OK to return TRUE if session could =
persist via transid.
>=20
Thanks for all the investigation (as well Remi, Stas and everyone). At =
first glance last week, as for me, it looked like OK to keep at least =
the 7.0 part, as the breach was only concerning the unit tests but =
unlikely the actual web functionality. While it is good to have =
improvements and hardening on the unclean behaviors, the area is =
critical and should be kept stable in stable branches. Right now, it =
seems that the patch can have unexposed impacts.

I would like also to remind that we're now quite short in time as finals =
are planned for the next week. With this in mind, IMHO it's better to =
play safe and revert 5.6 and 7.0 to the previous state before this =
change. Or at least, don't release these patches in the upcoming finals, =
but keep improving and fixing BC breaches in dev branch and re-evaluate =
the status in the next possible RC.

Regards

Anatol