php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #71599 trans sid handling rework broke interaction with cookies
Submitted: 2016-02-15 15:33 UTC Modified: 2016-09-29 09:04 UTC
Votes:17
Avg. Score:5.0 ± 0.0
Reproduced:17 of 17 (100.0%)
Same Version:17 (100.0%)
Same OS:-17 (-100.0%)
From: phpbug at wisl dot de Assigned: yohgaki (profile)
Status: Closed Package: Session related
PHP Version: 7.0.3 OS: All
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: phpbug at wisl dot de
New email:
PHP Version: OS:

Further comment on this bug is unnecessary.

 

 [2016-02-15 15:33 UTC] phpbug at wisl dot de
Description:
------------
I am using session.use_trans_sid=1 as a transparent fallback for users who have cookies disabled. By setting both use_trans_sid and use_cookies until PHP7 the cookie users would silently be upgraded to using cookies, while users without cookies could still use the session related function due to PHPSESSID that was written automatically into the html.

Cause seems to be this commit: https://github.com/php/php-src/commit/f248df900300c5b2201d4cf634d58d413399e2eb
"Cleanup trans sid code. Behavior is unchanged."

But the behavior is changed, because the new macro APPLY_TRANS_SID only looks for the ini settings use_trans_sid and use_only_cookies, but the code wrt. cookies like lines 540+541 was removed.

Test script:
---------------
<?php
ini_set("session.use_cookies","1");
ini_set("session.use_only_cookies","0");
ini_set("session.use_trans_sid","1");
session_start();
?>
<a href="bugtest.php">Follow Me</a>


Expected result:
----------------
On first call to the supplied test script (wenn saved as bugtest.php) the use_trans_sid feature will insert an additional parameter PHPSESSID into the <a href>, but because of use_cookies the script will also send an Set-Cookie-Header.

After clicking the link, the same page will reopen again. But if cookies are allowed in the browser, the <a href> will now no longer contain a PHPSESSID parameter.

After clicking the link, the <a href> should only still contain a PHPSESSID parameter, if there was no cookie set.

This behavior works as expected as of PHP 5.6.18.

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
 * will still rewrite the <a href> even if a cookies is already set

While testing I tried to call the script under PHP7 without any PHPSESSID parameter or cookie and it even segfaulted:
(gdb) run /srv/www/htdocs/bugtest.php
Starting program: /usr/bin/php-cgi7.0 /srv/www/htdocs/bugtest.php
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".

Program received signal SIGSEGV, Segmentation fault.
0x00000000008b6507 in zend_hash_str_find ()
(gdb) bt
#0  0x00000000008b6507 in zend_hash_str_find ()
#1  0x00000000006975a0 in php_session_start ()
#2  0x0000000000698d25 in zif_session_start ()
#3  0x00000000008feeee in ZEND_DO_ICALL_SPEC_HANDLER ()
#4  0x00000000008ee59b in execute_ex ()
#5  0x0000000000971826 in zend_execute ()
#6  0x000000000089f901 in zend_execute_scripts ()
#7  0x000000000081bb18 in php_execute_script ()
#8  0x000000000048daa0 in main ()

I will try to add more details for the segfault later...

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-02-15 16:09 UTC] phpbug at wisl dot de
The segfault seems to be something different. It can only be seen in the CGI version of php, but the script to reproduce is minimal:
<?php
ini_set("session.use_only_cookies","0");
session_start();
?>

After building php with more debug info, I got the following, better backtrace:

Program received signal SIGSEGV, Segmentation fault.
zend_hash_str_find (ht=0x0, str=str@entry=0xe1fb4e "REQUEST_URI", len=len@entry=11) at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/Zend/zend_hash.c:1959
1959    /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/Zend/zend_hash.c: No such file or directory.
(gdb) bt
#0  zend_hash_str_find (ht=0x0, str=str@entry=0xe1fb4e "REQUEST_URI", len=len@entry=11) at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/Zend/zend_hash.c:1959
#1  0x00000000006975a0 in php_session_start () at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/ext/session/session.c:1613
#2  0x0000000000698d25 in zif_session_start (execute_data=<optimized out>, return_value=0x7ffff0612090) at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/ext/session/session.c:2312
#3  0x00000000008feeee in ZEND_DO_ICALL_SPEC_HANDLER () at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/Zend/zend_vm_execute.h:586
#4  0x00000000008ee59b in execute_ex (ex=<optimized out>) at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/Zend/zend_vm_execute.h:414
#5  0x0000000000971826 in zend_execute (op_array=<optimized out>, return_value=<optimized out>) at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/Zend/zend_vm_execute.h:458
#6  0x000000000089f901 in zend_execute_scripts (type=type@entry=8, retval=retval@entry=0x0, file_count=file_count@entry=3)
    at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/Zend/zend.c:1427
#7  0x000000000081bb18 in php_execute_script (primary_file=primary_file@entry=0x7fffffffd310) at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/main/main.c:2484
#8  0x000000000048daa0 in main (argc=<optimized out>, argv=<optimized out>) at /var/tmp/portage/dev-lang/php-7.0.3/work/sapis-build/cgi/sapi/cgi/cgi_main.c:2453


session.c:1613+1614 is:
if (PS(define_sid) && !PS(id) &&
                        (data = zend_hash_str_find(Z_ARRVAL(PG(http_globals)[TRACK_VARS_SERVER]), "REQUEST_URI", sizeof("REQUEST_URI") - 1)) &&

The commit linked above removed a "!Z_ISUNDEF(PG(http_globals)[TRACK_VARS_SERVER])", that might explain this additional segfault, but this breakage is unrelated to the fact that use_trans_sid can no longer be used as a simple fallback for users that forbid cookies, as it now no longer skips the url rewriting, when a cookie is available.
 [2016-03-10 10:11 UTC] phpbug at wisl dot de
Crash has been fixed as Bug #71754: https://bugs.php.net/bug.php?id=71754

But the problem that the transparent session id rework is broken still persists. session_regenerate_id() for example is also broken.

My application does a session_regenerate_id() when a new session is started to prevent session fixation attacks. Because of that on the first request all URLs had two PHPSESSID parameter added. The effect and that this is only broken in PHP7 can be seen with the following test script:

bugtest3.php:
<?php
ini_set("session.use_only_cookies","0");
ini_set("session.use_trans_sid","1");
session_start();
for($i=0;$i<$_REQUEST["count"];$i++) session_regenerate_id();
?>
<a href="bugtest3.php?count=0">0</a></br>
<a href="bugtest3.php?count=1">1</a></br>
<a href="bugtest3.php?count=2">2</a></br>
<a href="bugtest3.php?count=3">3</a></br>

Using the URL bugtest3.php?count=3 under PHP 5.5 without having a cookie set results in hrefs like "bugtest3.php?count=3&PHPSESSID=f94bqpvtgksdiro232qolclut6". That is the correct and expected behavior.

Under PHP 7.0.4 the URLs look like this: "bugtest3.php?count=3&PHPSESSID=pgvvh2d39jpr4vnubnq2r2m800&PHPSESSID=umpctfqdp4q0av74l6mtflmqp3&PHPSESSID=a94c7n4694n2rrkqc2tlnlt1k3&PHPSESSID=ek4m6ikt29po9oh6045m1fpke2"

Each call to session_regenerate_id() seems to add another rewriter and an additional PHPSESSID parameter.

Could somebody please look into this?
 [2016-03-10 18:12 UTC] cmb@php.net
-Assigned To: +Assigned To: yohgaki
 [2016-03-10 18:12 UTC] cmb@php.net
Yasuo, could you please have a look at this issue?
 [2016-03-11 01:33 UTC] yohgaki@php.net
-Status: Assigned +Status: Duplicate
 [2016-03-11 01:33 UTC] yohgaki@php.net
The cause is jit global change in PHP7.

Related to bug #71683
 [2016-03-11 08:08 UTC] phpbug at wisl dot de
This is not a duplicate of #71683.

The crash that I also noted might be a duplicate of #71683, but that got fixed as bug #71754.

The logic wrt. the transparent session id when a cookie is available and that multiple calls to session_regenerate_id() add multiple PHPSESSID parameters is broken in PHP7 compared to PHP5.X and this can't be fixed with the crash fix from #71683 or #71754.

Please reopen this bug.
 [2016-03-11 09:07 UTC] cmb@php.net
-Status: Duplicate +Status: Re-Opened
 [2016-03-11 09:07 UTC] cmb@php.net
> The logic wrt. the transparent session id when a cookie is
> available and that multiple calls to session_regenerate_id() add
> multiple PHPSESSID parameters is broken in PHP7 compared to
> PHP5.X […]

The latter might be caused by
<https://github.com/php/php-src/commit/f248df900300c5b2201d4cf634d58d413399e2eb#diff-52eb9eb7f9d5d9125fbb1337a6541c06R1491>.
 [2016-03-11 23:43 UTC] yohgaki@php.net
Automatic comment on behalf of yohgaki
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ca61f5954bf9e64072bfa31b4a7431e211a109e7
Log: Fixed Bug #71754 Regression in PHP7.0: trivial script segfaults php-cgi Fixed Bug #71683 Null pointer dereference in zend_hash_str_find_bucket Fixed Bug #71599 trans sid handling rework broke interaction with cookies
 [2016-03-11 23:43 UTC] yohgaki@php.net
-Status: Re-Opened +Status: Closed
 [2016-03-14 14:59 UTC] ab@php.net
Automatic comment on behalf of yohgaki
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ca61f5954bf9e64072bfa31b4a7431e211a109e7
Log: Fixed Bug #71754 Regression in PHP7.0: trivial script segfaults php-cgi Fixed Bug #71683 Null pointer dereference in zend_hash_str_find_bucket Fixed Bug #71599 trans sid handling rework broke interaction with cookies
 [2016-04-05 14:59 UTC] phpbug at wisl dot de
-Status: Closed +Status: Assigned
 [2016-04-05 14:59 UTC] 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.
 [2016-04-06 00:17 UTC] yohgaki@php.net
-Status: Assigned +Status: Closed
 [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.

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-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.
 [2016-04-06 10:22 UTC] 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 10:57 UTC] 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 11:52 UTC] phpbug at wisl dot de
>  - 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.
 [2016-04-06 21:05 UTC] yohgaki@php.net
Now I understand what is the issue. Do not report multiple issues at once next time.
 [2016-07-20 11:33 UTC] davey@php.net
Automatic comment on behalf of yohgaki
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ca61f5954bf9e64072bfa31b4a7431e211a109e7
Log: Fixed Bug #71754 Regression in PHP7.0: trivial script segfaults php-cgi Fixed Bug #71683 Null pointer dereference in zend_hash_str_find_bucket Fixed Bug #71599 trans sid handling rework broke interaction with cookies
 [2016-09-29 06:39 UTC] fashionretailshop01 at gmail dot com
http://www.cheapjerseysfree.us/ - wholesale nfl jerseys
http://www.wholesalerjersey.com/ - wholesale jerseys
http://www.wholesaleCheapJerseysshipping.com/ - wholesale Jerseys free shipping
http://www.cheapwholesalejerseysshipping.com/ - cheap jerseys free shipping
http://www.wholesalejerseyscheapforsale.com/ - wholesale jerseys
 [2016-09-29 06:40 UTC] fashionretailshop01 at gmail dot com
http://www.cheapjerseyswholesaleforsale.com/ - cheap jerseys
http://www.WholesaleJerseysCheaponline.com/ - Wholesale Jerseys
http://www.wholesalejerseyscheapfromchina.com/ - wholesale jerseys from china
http://www.wholesalecheapauthenticjerseys.com/ - wholesale authentic jerseys
http://www.cheapauthenticwholesalejerseys.com/ - cheap authentic jerseys
 [2016-09-29 06:41 UTC] fashionretailshop01 at gmail dot com
http://www.about-jerseys.com/ - wholesale jerseys
http://www.about-jerseys.com/default.aspx/Sitemap - Cheap Jerseys - Wholesale Jerseys
http://www.fakesunglasseswholesale.com/ - sunglasses
http://www.fakesunglasseswholesale.com/tag.php?Cheap_Ray_Ban_Sunglasses/ - Cheap Ray Ban Sunglasses
http://www.fakesunglasseswholesale.com/news.php?cheap_oakleys_holbrook_sunglasses/ - cheap oakley holbrook sunglasses
 [2016-09-29 06:42 UTC] fashionretailshop01 at gmail dot com
http://www.wholesalediscountjerseys.com/AuthenticJerseys/ - Authentic NFL Jerseys
http://www.cheapjerseyswhoesale.com/ - About Jerseys News
http://www.cheapshoesfreeshipping.com/ - Best Shoes news Website
http://www.cheapsunglassesfreeshipping.com/ - Fashion Sunglasses News Website
http://www.sphoter.com/ - Buy Wholesale Cheap Free Shipping China
 [2016-09-29 06:43 UTC] fashionretailshop01 at gmail dot com
http://www.cheapjewe11erystore.com/ - Cheap Pandora Store
http://www.gryfino.sr.gov.pl/discount/mlb-jerseys/ - mlb jerseys
http://www.gryfino.sr.gov.pl/discount/nba-jerseys/ - nba jerseys
http://www.gryfino.sr.gov.pl/discount/ncaa-jerseys/ - ncaa jerseys
 [2016-09-29 09:04 UTC] cmb@php.net
-Block user comment: No +Block user comment: Yes
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Wed Apr 24 02:01:30 2024 UTC