Bug #71599 [Csd]: trans sid handling rework broke interaction with cookies

Edit report at https://bugs.php.net/bug.php?id=71599&edit=1 ID: 71599 User updated by: phpbug at wisl dot de Reported by: phpbug at wisl dot de Summary: trans sid handling rework broke interaction with cookies Status: Closed Type: Bug Package: Session related Operating System: All PHP Version: 7.0.3 Assigned To: yohgaki Block user comment: N Private report: N New Comment: > - use_trans_sid=1, use_cookies=1 and use_only_cookies=0 do not add PHPSESSID params in URL and > HTML form. I wrote in the original report: >Actual result: >-------------- >Under PHP7 (tested with 7.0.2 and 7.0.3) it will: > * no longer send a Set-Cookie Header, despite use_cookies=1 and >use_only_cookies=0 I can't reproduce this any longer. Possible I did not see this Header, because of previous tries already set a cookie and I failed to clear it for this test. But that was only a secondary concern, my main problem is: > * will still rewrite the <a href> even if a cookies is already set The problem is not, that PHP7 would fail to add the PHPSESSID parameter, but that it will still add it, even when a cookie is set and so is not needed. My usecase is as following: 1. Request: User has no Cookie, no Session * session_start() will start a new Session * I will call session_regenerate_id() to create a new, safe Session-ID * Set-Cookie-Header will be send * All URLs in this page will be rewritten to add PHPSESSID=... 2. Request: Case A) User has Cookies allowed, the via Set-Cookie generated Cookie will be send back to the server * session_start() will resume the Session * No Set-Cookie-Header, as there already was a Cookie available * No changes to any URLs, because a Cookie was available Case B) User did not allow Cookies * session_start() will resume the Session, because all URLs from the first Request had PHPSESSID as an normal URL parameter * Set-Cookie-Header will be send again, but that is no problem, the user will just ignore it * Because there was no Cookie, the URL-Rewriter for trans_sid will now also rewrite all URLs in this second page. Because in this case of a Cookie-Forbidder these parameter are still needed. This works fine with all versions of PHP5. With PHP7 I see the following changes: On 1. Request: Wenn I call session_regenerate_id() after session_start() I now have 2 PHPSESSIDs in every URL. That currently only looks ugly, but I'm not sure if this can result in any problems, if the wrong one gets used. That every URL gets an PHPSESSID added on this first request, even when a user has cookies allowed is ugly and has already caused problems, but I 100% understand that this is the *correct and intended behavior* of PHP. This is *not* what this bug report is about. (And this is the same in PHP5 and PHP7) On 2. Request: Case B) is still working as intended under PHP7. Case A) is the real problem: Under PHP7 the trans sid code will no longer silently turn off, when it detects that a cookie is available. This is my problem: Now the 90+% of my vistitors that enable at least session cookies will always get the ugliger URL version with the included PHPSESSID. Which will then be used in bookmarks or shared including a stale session id. These are the two things that I wanted to explain in https://bugs.php.net/bug.php?id=71599#1459868341 Under 1. the change removed the cookie check an now results in always adding PHPSESSID, even if not needed. Under 2. the change that caused multiple PHPSESSID to appear, and an explanation, why I think my use case (first session_start(), then session_regenerate_id()) is a normal usage of the session API. As you requested in: [2016-04-06 00:17 UTC] yohgaki@php.net > Please open new bug report for this and keep this one closed. I have rereported this bug as Bug #71974 . Should we continue there? If you want, I can also describe in more detail what I'm seeing with PHP5 / PHP7 with the small testscripts bugtest.php and bugtest3.php from my previous comments, if these description are missing something. Previous Comments: ------------------------------------------------------------------------ [2016-04-06 10:57:49] yohgaki@php.net OK. You've reported 2 issues on this report. - Crash is caused because PHP 7 changed auto global behavior. Fixed. - use_trans_sid=1, use_cookies=1 and use_only_cookies=0 do not add PHPSESSID params in URL and HTML form. I cannot reproduce 2nd issue. I verified that my browser stores/sends PHPSESSID cookie, displays PHPSESSID on URL. ------------------------------------------------------------------------ [2016-04-06 10:22:52] phpbug at wisl dot de [2016-04-06 00:17 UTC] yohgaki@php.net >The issue mentioned on this bug report is crash bug that is caused by missing >>zend_is_auto_global(). This is fixed. I am the original reporter. And the original report was about the cookie problems. That's why this bug has "trans sid handling rework broke interaction with cookies" as its subject. I only mentioned the crash, because when trying to debug this issue I suddenly only got HTTP 500 from my local server, because now the php-cgi was crashing. My Bug https://bugs.php.net/bug.php?id=71754 is a duplicate of Bug #71683, because I did not really search when filing this bug, because the main intention behind Bug #71754 was to get any attention, because this bug had been completely ignored for nearly a complete month. OTOH #71754 demonstrated that this crash did not only happen in cli, but also in the cgi version of PHP. >Please open new bug report for this and keep this one closed. I think I >understand what is >your problem, but please describe the issue in detail. >Thank you. I will report a new bug to disentangle this... [2016-04-06 00:33 UTC] yohgaki@php.net >BTW, issue you're describing sounds like known issue (at least for me) for a >long time. >To make sure it is known issue, please write short reproducible >code. It is a clear regression between PHP5 and PHP7, as described at various comments. And a single Test script, including "Expected result" and "Actual result" was provided in the original bug report. ------------------------------------------------------------------------ [2016-04-06 00:33:22] yohgaki@php.net BTW, issue you're describing sounds like known issue (at least for me) for a long time. To make sure it is known issue, please write short reproducible code. ------------------------------------------------------------------------ [2016-04-06 00:17:47] yohgaki@php.net The issue mentioned on this bug report is crash bug that is caused by missing zend_is_auto_global(). This is fixed. Please open new bug report for this and keep this one closed. I think I understand what is your problem, but please describe the issue in detail. Thank you. ------------------------------------------------------------------------ [2016-04-05 14:59:01] phpbug at wisl dot de PHP 7.0.5 just appeared in the Gentoo package tree and I retested this version. The main bug, the broken interaction between trans sid and cookies, is *NOT* fixed, it still has the same regressions. 1.: https://github.com/php/php-src/commit/f248df900300c5b2201d4cf634d58d413399e2eb#diff-52eb9eb7f9d5d9125fbb1337a6541c06L538 By removing the condition " && PS(send_cookie)" before apply_trans_sid was set 1, now a trans sid will always be transmitted, even if a correct session cookie is available. That prevents using the trans sid as a simple fallback, because as of PHP7 if it is enabled at all, it will always send an PHPSESSID rendering any cookie support superfluous despite using cookies for session ids is the better way. 2.: https://github.com/php/php-src/commit/f248df900300c5b2201d4cf634d58d413399e2eb#diff-52eb9eb7f9d5d9125fbb1337a6541c06L1494 By no longer resetting the rewriter each call to php_session_reset_id() will add another PHPSESSID parameter instead of replacing the old one. As session_regenerate_id() is calling into this function when switching to a new session id this means, that each call to session_regenerate_id() will add another PHPSESSID parameter. For me this means, that if a visitor request the first page of my site an new session will be generated. This will add a first PHPSESSID parameter to all URLs. This new empty session will be regarded as unsafe, by my code, because its ID might have been supplied by an attacker. So my code calls session_regenerate_id() to generate a new, guaranteed to be safe, session id. This will now add a second PHPSESSID parameter, while leaving the original (possible tampered with ID) in all URLs. PHP version before PHP7 would correctly only insert the correct new session id. Both of these changes are serious regression for my code, as it means I can no longer use the trans sid in PHP7 and now need to force all my visitors to enable cookies. As the commit promised "Behavior is unchanged", I see this as bugs and not changes that were needed to make PHP7 work, so please restore the trans sid behavior as it was in PHP5. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=71599 -- Edit this bug report at https://bugs.php.net/bug.php?id=71599&edit=1