Hi all, I've made the patch for trunk. I checked by using session module tests using valgrind.
https://gist.github.com/1379668 This patch adds - validate_sid() to ps_modules (Save handlers) - use_strict_session to php.ini (On by default, off for old behavior) - display that save handler supports strict session or not via phpinfo() (So that user could know behavior) - update PHP_SESSION_API version (So that save handler authors could write portable code) Compatibility issues are - save handlers that are currently working should also work with this patch, except ps modules using PS_MOD_SID and PS_MOD_FUNCS_SID macro. These ps modules should implement validate_sid(). Modules that are using PS_MOD/PS_FUNCS are not affected, they just marked as "adaptive" module. (e.g. pecl sqlite's ps module. You can see it via phpinfo()) NOTE: PS_MOD_SID() and PS_MOD_FUNCS_SID() are already defined, but it was the same as PS_MOD()/PS_MOD_FUNCS(). If old ps_module does not compile, just remove "_SID" from PS_MOD_SID()/PS_MOD_FUNCS_SID(). - session ID string is checked so that chars are alphanumeric + ',' '-' when use_strict_session=On. (mod_file.c checks session ID this way w/o this patch to prevent problems. Using restrictive session ID string is better for other ps modules. IMHO) - session read failure is not rescued to eliminate possible infinite loops. Bundled ps modules were not using this at least. You will see some tests are failing since they depend on adaptive session. By looking into failing test results, you can see this patch is generating new session ID if it's not a already initialized session. I'll modify these tests (set use_strict_session=Off) and add some tests for new feature (new validate_sid() function for user and use_class save handler) after commit. It removes session read failure retry code from php_session_initialize() . The retry code may cause infinite loop depending on save handler implementation. Some session save handlers may be using this to prevent adoption(?), but they should implement validate_sid() to prevent adoption from now on. With new code, there will never be infinite loops. Currently bundled save handlers are not using this feature. If there is no objection, I'll commit it to trunk. It will save much time if I don't have to write wiki for rfc. Comments are appreciated. -- Yasuo Ohgaki 2011/11/12 9:50 "Rui Hirokawa" <rui_hirok...@yahoo.co.jp>: > > Hi, > > I strongly recommend to submit the Strict session patch for > php-src(HEAD) because the vulnerability of PHP against the session > adoption/fixation attack is annoying issue of the PHP programmers for > many years. > > I also suggest to apply this patch for PHP_5_4 after PHP 5.4.0 is > released. For PHP 5.4.1, I suggest that the default value of > session.use_strict_mode should be 0 (Off) to maintain the backward > compatibility. > > Rui > > Yasuo Ohgaki wrote: > > Hi all, > > > > Few years ago, I have proposed strict session. > > It seems PHP 5.4 and php-src don't have protection against session > > adoption yet. > > > > Since there will be many TLDs, session adoption attack will be > > very easy for some domains until browsers support them. > > Even without new TLDs, attacker may place cookie file to attack > > session adoption or can exploit paths or domains, since there is no > > standard for precedence. > > > > e.g. Domain has more precedence over path on Chrome while > > path has greater precedence on IE. This is due to the order difference > > of sent cookies. (If there are multiple cookies are set for domain/path, > > only > > one became the outstanding cookie. I think PHP uses first IIRC while other > > implementation may use the last. Therefore, browser may not able to > > solve this issue, since it may destroy apps specific to browser) > > > > Even if a programmer sets path and domain for PHP session id cookie, > > attackers may exploit this to fix session id with session managers that are > > vulnerable to session adoption. > > > > If you don't get idea, play with cookie with/without domain/path is > > set/unset. > > You'll see how attacker may use session adoption. Default session module's > > configuration (domain not set, path set to /) is very easy to exploit > > anyway. > > > > I usually set both exact application domain/path, and unset all domain/path > > cookies for session id to prevent the attack. I guess this is not > > widely adopted. > > Even this is not enough. For example, if subdomain is added, Chrome has > > greater precedence for subdomain and attacker may exploit it. > > > > I pasted a patch for PHP 5.2 that rejects uninitialized session id. I > > think original > > patch was written by Stefan Esser. It is for PHP 5.2, but it's easy to port > > to > > current PHP. If one would like to old behavior, he/she can just turn off the > > strict session. > > > > There are too many ways to exploit session with session adoption vulnerable > > session manger. Simple solution is making session manager strict. > > > > Any comments? > > > > -- > > Yasuo Ohgaki > > yohg...@ohgaki.net > > > > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
