# Session Id Collisions - Thread: https://externals.io/message/62414 - Original archive: https://news-web.php.net/php.internals/62414 - Started by Raymond Irving on 2012-08-23T04:48:12+00:00 - Messages: 37 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/62414.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #62414 — Raymond Irving — 2012-08-23T04:48:12+00:00 URL: https://externals.io/message/62414#62414 Hello Everyone, I've been reading that it's possible to encounter session id collisions with the default php configuration. It's also been said that PHP utilizes a cryptographically weak random number generator to produce session ID information. I know it's possible to change the hash function and entropy used in the generation of the id but after looking at the php_session_create_id() function in the source code, I am wondering if adding the User Agent string to the default setup would improve the uniqueness of the id. What do you think? __ Raymond --- ## #62424 — Rasmus Lerdorf — 2012-08-23T15:03:32+00:00 URL: https://externals.io/message/62414#62424 · In reply to: #62414 > Hello Everyone, > > I've been reading that it's possible to encounter session id collisions > with the default php configuration. It's also been said that PHP utilizes a > cryptographically weak random number generator to > produce session ID information. > > > I know it's possible to change the hash function and entropy used in the > generation of the id but after looking at the php_session_create_id() > function in the source code, I am wondering if adding the User Agent > string to the default setup would improve the uniqueness of the id. > > What do you think? Adding a widely-known string adds very little entropy. As of PHP 5.4 we default to using session.entropy_file set to /dev/urandom or /dev/random (if it is available) so there is no entropy issue with the default config as of 5.4. Before 5.4 users had to be aware enough to add that to their php.ini themselves. -Rasmus --- ## #62425 — Raymond Irving — 2012-08-23T15:26:14+00:00 URL: https://externals.io/message/62414#62425 · In reply to: #62424 Hi Rasmus, Many thanks for the information. It would be great if this information can be added to the docs: http://www.php.net/manual/en/session.configuration.php#ini.session.entropy-file __ Raymond > > > […] > utilizes a > > […] > > Adding a widely-known string adds very little entropy. As of PHP 5.4 we > default to using session.entropy_file set to /dev/urandom or /dev/random > (if it is available) so there is no entropy issue with the default > config as of 5.4. Before 5.4 users had to be aware enough to add that to > their php.ini themselves. > > -Rasmus --- ## #62427 — Sherif Ramadan — 2012-08-23T15:53:28+00:00 URL: https://externals.io/message/62414#62427 · In reply to: #62425 > Hi Rasmus, > > Many thanks for the information. > > It would be great if this information can be added to the docs: > > http://www.php.net/manual/en/session.configuration.php#ini.session.entropy-file > > > Please open a documentation bug at https://bugs.php.net/ for this so that we have a record of the issue and I'll be sure to follow up on the ticket. I can confirm the default configuration directives from 5.4: http://lxr.php.net/xref/PHP_5_4/ext/session/session.c#725 and 5.3: http://lxr.php.net/xref/PHP_5_3/ext/session/session.c#807 So I will update the docs accordingly to reflect the change. --- ## #62468 — Yasuo Ohgaki — 2012-08-25T02:47:16+00:00 URL: https://externals.io/message/62414#62468 · In reply to: #62427 Hi, I was willing to add collision detection to session module after session adoption patch is merged. What's the status of session adoption patch? I've created patches for all 3 versions and I think Stats is going to merge it to master and PHP 5.4. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net 2012/8/24 Sherif Ramadan : > > […] > > > Please open a documentation bug at https://bugs.php.net/ for this so > that we have a record of the issue and I'll be sure to follow up on > the ticket. > > I can confirm the default configuration directives from 5.4: > > http://lxr.php.net/xref/PHP_5_4/ext/session/session.c#725 > > and 5.3: > > http://lxr.php.net/xref/PHP_5_3/ext/session/session.c#807 > > So I will update the docs accordingly to reflect the change. --- ## #62490 — Ferenc Kovacs — 2012-08-25T16:40:05+00:00 URL: https://externals.io/message/62414#62490 · In reply to: #62468 > Hi, > > I was willing to add collision detection to session module > after session adoption patch is merged. > > What's the status of session adoption patch? > I've created patches for all 3 versions and I think Stats > is going to merge it to master and PHP 5.4. > > Please don't top post. What is this session adoption patch? Is it (part of) the Strict session rfc/patch from you? -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #62499 — Yasuo Ohgaki — 2012-08-25T22:02:29+00:00 URL: https://externals.io/message/62414#62499 · In reply to: #62490 2012/8/26 Ferenc Kovacs : > > > > > […] > > Please don't top post. > What is this session adoption patch? > Is it (part of) the Strict session rfc/patch from you? Yes. Strict session patch reject uninitialized session ID, thus it prevents session adoption/fixation. I know session ID collision will not happen most likely, but there are few people who worries collision. We can check session ID collision when it is generated. It's easy patch, but I didn't include the patch to focus on adoption. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #62507 — Stas Malyshev — 2012-08-26T07:37:45+00:00 URL: https://externals.io/message/62414#62507 · In reply to: #62468 Hi! > What's the status of session adoption patch? > I've created patches for all 3 versions and I think Stats > is going to merge it to master and PHP 5.4. As far as I remember there were some things that needed to be refactored/changed and I didn't see the updates since then, but if you could point me to the latest version I could check it out. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #62508 — Stas Malyshev — 2012-08-26T08:57:30+00:00 URL: https://externals.io/message/62414#62508 · In reply to: #62499 Hi! > I know session ID collision will not happen most likely, but > there are few people who worries collision. We can check > session ID collision when it is generated. You mean two randomly generated session IDs colliding? I think the probability of it is pretty low. I mean it'd take PHP's random number generator function to generate two equal random numbers in the same microsecond. And these are random 64-bit numbers, so unless you're generating billions of sessions per microsecond I don't think it's a very real concern. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #62533 — Yasuo Ohgaki — 2012-08-26T20:49:20+00:00 URL: https://externals.io/message/62414#62533 · In reply to: #62508 Hi, 2012/8/26 Stas Malyshev : > Hi! > > > […] > > You mean two randomly generated session IDs colliding? I think the > probability of it is pretty low. I mean it'd take PHP's random number > generator function to generate two equal random numbers in the same > microsecond. And these are random 64-bit numbers, so unless you're > generating billions of sessions per microsecond I don't think it's a > very real concern. Right, Statistically, session ID collision will not happen. It's very low even when we consider birthday paradox. We could also use stronger hash (SHA-1 etc) rather than MD5. However, collision detection is easy and makes collision impossible. That's the whole point of the patch. Collision is most unlikely, but unique session ID the basis of security. It's worth to have, IMHO. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #62534 — Yasuo Ohgaki — 2012-08-26T20:55:40+00:00 URL: https://externals.io/message/62414#62534 · In reply to: #62507 Hi, 2012/8/26 Stas Malyshev : > Hi! > > > […] > > As far as I remember there were some things that needed to be > refactored/changed and I didn't see the updates since then, but if you > could point me to the latest version I could check it out. We agreed not to introduce new session save handler API so that there would be no barrier for adoption. I've done it months ago, I guess you missed a mail. These are the patch. master https://gist.github.com/1379668 5.4 https://gist.github.com/2224196 5.3 https://gist.github.com/2224360 There might be conflict since it's made at 3/28, I'll fix it if there is. (I'll check it myself within a few days) > if you > could point me to the latest version I could check it out. Thank you for your time. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #62535 — Stas Malyshev — 2012-08-26T21:30:29+00:00 URL: https://externals.io/message/62414#62535 · In reply to: #62534 Hi! Thank you for the links! > master > https://gist.github.com/1379668 > > 5.4 > https://gist.github.com/2224196 > > 5.3 > https://gist.github.com/2224360 I think patch of this magnitude is not a good idea for 5.3. As for the rest, it'd be much easier to track and comment on if you could create a pull request. I'll read it through and see if it's OK for 5.4, but either 5.4 or master pull would be great. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #62536 — Yasuo Ohgaki — 2012-08-26T21:31:25+00:00 URL: https://externals.io/message/62414#62536 · In reply to: #62534 Hi, 2012/8/27 Yasuo Ohgaki : > Hi, > > 2012/8/26 Stas Malyshev : > > […] > > We agreed not to introduce new session save handler API so that > there would be no barrier for adoption. > > I've done it months ago, I guess you missed a mail. > These are the patch. > > master > https://gist.github.com/1379668 > > 5.4 > https://gist.github.com/2224196 > > 5.3 > https://gist.github.com/2224360 > > There might be conflict since it's made at 3/28, I'll > fix it if there is. (I'll check it myself within a few days) > > > […] > > Thank you for your time. > > Regards, It seems I've already added collision detection when I last updated :) It tries to generate new session ID a few times and if it fails, it does not initialize session. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #62537 — Yasuo Ohgaki — 2012-08-26T21:33:25+00:00 URL: https://externals.io/message/62414#62537 · In reply to: #62535 Hi, 2012/8/27 Stas Malyshev : > Hi! > > Thank you for the links! > > > […] > > I think patch of this magnitude is not a good idea for 5.3. As for the > rest, it'd be much easier to track and comment on if you could create a > pull request. I'll read it through and see if it's OK for 5.4, but > either 5.4 or master pull would be great. OK, I'll review my patch again and send pull request for master and 5.4 soon. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #62538 — Stas Malyshev — 2012-08-26T21:35:06+00:00 URL: https://externals.io/message/62414#62538 · In reply to: #62536 Hi! > It seems I've already added collision detection when I > last updated :) > > It tries to generate new session ID a few times and > if it fails, it does not initialize session. It'd be nice if we could keep it separate. Could you create a pull that includes only the strict session functionality? You could add the collision part in separate pull and we could discuss it separately but it'd be nice to not mix these things. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #62539 — Yasuo Ohgaki — 2012-08-26T21:36:52+00:00 URL: https://externals.io/message/62414#62539 · In reply to: #62538 Hi, 2012/8/27 Stas Malyshev : > Hi! > > > […] > > It'd be nice if we could keep it separate. Could you create a pull that > includes only the strict session functionality? You could add the > collision part in separate pull and we could discuss it separately but > it'd be nice to not mix these things. > No problem. I'll make separate requests for it. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #64421 — Yasuo Ohgaki — 2012-12-24T06:32:17+00:00 URL: https://externals.io/message/62414#64421 · In reply to: #62539 Hi stats and others, Sorry for the delay. I've finally updated the strict session patch. Following diff is against PHP-5.3, but 5.4 and others will be mostly the same. https://github.com/yohgaki/php-src/commit/42dcd8ef7cd2f9f2071b16586822dadd647c96ef I was promised to create separate patch for session id collision detection, but the patch is also in the diff. I left comments for the "if" statement handles collision. If you still prefer to have separate patch, I'll do it when I send pull requests. If nobody has comment, I'll create patch for 5.4. If you read this patch closely, you'll see that this patch accesses PS(id) global directly due to limitation of current API. It would be good to have new API for 5.5, I think. BTW, this patch fixes bug #60634 as a side effect also. https://bugs.php.net/bug.php?id=60634 Comments are appreciated. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #67913 — Yasuo Ohgaki — 2013-06-27T00:36:24+00:00 URL: https://externals.io/message/62414#67913 · In reply to: #64421 Hi, Sorry for the long delay, I've sent pull requests https://github.com/php/php-src/pull/368 https://github.com/php/php-src/pull/367 https://github.com/php/php-src/pull/366 Thank you for your time. -- Yasuo Ohgaki yohgaki@ohgaki.net 2012/12/24 Yasuo Ohgaki > Hi stats and others, > > Sorry for the delay. > I've finally updated the strict session patch. > > Following diff is against PHP-5.3, but 5.4 and others will be mostly the > same. > > > https://github.com/yohgaki/php-src/commit/42dcd8ef7cd2f9f2071b16586822dadd647c96ef > > I was promised to create separate patch for session id collision > detection, but > the patch is also in the diff. I left comments for the "if" statement > handles > collision. If you still prefer to have separate patch, I'll do it when > I send pull requests. > > If nobody has comment, I'll create patch for 5.4. > > If you read this patch closely, you'll see that this patch accesses > PS(id) global directly due to limitation of current API. It would be > good to have new API for 5.5, I think. > > BTW, this patch fixes bug #60634 as a side effect also. > https://bugs.php.net/bug.php?id=60634 > > Comments are appreciated. > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #67926 — Arpad Ray — 2013-06-27T09:02:53+00:00 URL: https://externals.io/message/62414#67926 · In reply to: #67913 > Hi, > > Sorry for the long delay, I've sent pull requests > > https://github.com/php/php-src/pull/368 > https://github.com/php/php-src/pull/367 > https://github.com/php/php-src/pull/366 > > Hi, I see the strict mode check is now implemented in the handlers and not session.c, presumably to keep ABI, but this means code is duplicated and the setting only actually works if the handler supports it. It's unfortunate timing since 5.5 has just gone, but I think it would make much more sense to have a new function in the structure (as in your original patch) and do this only in PHP.next. Having such an ini setting which quietly fails if using an unsupported handler is not good. I guess you could keep a whitelist of supported handlers but that's also obviously far from ideal. Regards, Arpad > Thank you for your time. > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > > > 2012/12/24 Yasuo Ohgaki > > > […] > https://github.com/yohgaki/php-src/commit/42dcd8ef7cd2f9f2071b16586822dadd647c96ef > > […] --- ## #67935 — Yasuo Ohgaki — 2013-06-27T10:51:42+00:00 URL: https://externals.io/message/62414#67935 · In reply to: #67926 Hi Arpad, 2013/6/27 Arpad Ray > I see the strict mode check is now implemented in the handlers and not > session.c, presumably to keep ABI, but this means code is duplicated and > the setting only actually works if the handler supports it. It's > unfortunate timing since 5.5 has just gone, but I think it would make much > more sense to have a new function in the structure (as in your original > patch) and do this only in PHP.next. > > Having such an ini setting which quietly fails if using an unsupported > handler is not good. I guess you could keep a whitelist of supported > handlers but that's also obviously far from ideal. > Thank you for comment. There are plenty of time before 5.6 release, I would like to fix ps_module API also. Current implementation requires ps_modules to access PS(id) to prevent rare case of crash. I'll try to implement a little better API for ps_modules. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68011 — Stas Malyshev — 2013-06-28T21:06:59+00:00 URL: https://externals.io/message/62414#68011 · In reply to: #64421 Hi! > Sorry for the delay. > I've finally updated the strict session patch. I'll review it ASAP, probably on the weekend. Unfortunately, we've missed the window for 5.5 API changes, if there's any API change, but we can still do it in master. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #68368 — Stas Malyshev — 2013-08-05T00:15:43+00:00 URL: https://externals.io/message/62414#68368 · In reply to: #67913 Hi! > Sorry for the long delay, I've sent pull requests > > https://github.com/php/php-src/pull/368 > https://github.com/php/php-src/pull/367 > https://github.com/php/php-src/pull/366 I'm looking at the pulls, and I see these issues: 1. session_id is still banned in strict mode. Can we not ban it? I think it detracts significantly from usefulness of the patch, as it would break a lot of legit code that takes session IDs not from cookies but from other sources and thus have to use session_id. 2. I got segfault when running it on test session_set_save_handler_class_005. Here's the backtrace: 0x003a4c6d in ps_files_path_create (buf=0xbfffdae4 "", buflen=1024, data=0x0, key=0x286f95c "ba3c966548c65fb9dd0ad4d30972f2fa") at /Users/smalyshev/php-5.5/ext/session/mod_files.c:76 76 if (key_len <= data->dirdepth || (gdb) bt #0 0x003a4c6d in ps_files_path_create (buf=0xbfffdae4 "", buflen=1024, data=0x0, key=0x286f95c "ba3c966548c65fb9dd0ad4d30972f2fa") at /Users/smalyshev/php-5.5/ext/session/mod_files.c:76 #1 0x003a47f9 in ps_files_key_exists (data=0x0, key=0x286f95c "ba3c966548c65fb9dd0ad4d30972f2fa") at /Users/smalyshev/php-5.5/ext/session/mod_files.c:230 #2 0x003a46c8 in ps_create_sid_files (mod_data=0xc9ae94, newlen=0x0) at /Users/smalyshev/php-5.5/ext/session/mod_files.c:468 #3 0x003960ef in zim_SessionHandler_create_sid (ht=0, return_value=0x286f880, return_value_ptr=0xbfffe228, this_ptr=0x2868260, return_value_used=1) at /Users/smalyshev/php-5.5/ext/session/mod_user_class.c:155 Looks like module data is not initialized properly (which is the point of the test) but the checks on ps_create_sid_files do not check it. 3. Also got some other test failures: session_save_path_variation5 session_set_save_handler_class_012 session_set_save_handler_class_016 Didn't look into those yet. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #68369 — Stas Malyshev — 2013-08-05T00:45:30+00:00 URL: https://externals.io/message/62414#68369 · In reply to: #68368 Hi! > > […] I've amended your patch slightly: https://github.com/php/php-src/pull/401 and it looks fine to me now. What do you think, is it OK with you? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #68370 — Yasuo Ohgaki — 2013-08-05T01:01:31+00:00 URL: https://externals.io/message/62414#68370 · In reply to: #68369 Hi Stas, On Mon, Aug 5, 2013 at 9:45 AM, Stas Malyshev wrote: > > […] > > I've amended your patch slightly: > https://github.com/php/php-src/pull/401 > > and it looks fine to me now. What do you think, is it OK with you? Thank you for noticing crash. Data can be null, so the fix is OK. Removing the limitation that prohibits setting session ID is fine for me, too. Please, apply your patch. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #68372 — Arpad Ray — 2013-08-05T09:22:45+00:00 URL: https://externals.io/message/62414#68372 · In reply to: #68370 Hi, > Thank you for noticing crash. Data can be null, so the fix is OK. > Removing the limitation that prohibits setting session ID is fine for me, > too. > > Please, apply your patch. I thought we were in agreement about doing this properly in PHP.next? My arguments against this version of the patch still stand: > Hi Arpad, > > 2013/6/27 Arpad Ray > > > […] > > Thank you for comment. > > There are plenty of time before 5.6 release, I would like to fix ps_module > API also. Current implementation requires ps_modules to access PS(id) to > prevent rare case of crash. I'll try to implement a little better API for > ps_modules. > Arpad --- Next page: https://externals.io/message/62414.md?page=2