# [VOTE] Secure Session Module Options/Internal by Default - Thread: https://externals.io/message/72643 - Original archive: https://news-web.php.net/php.internals/72643 - Started by Yasuo Ohgaki on 2014-02-17T04:27:16+00:00 - Messages: 9 - 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 --- ## #72643 — Yasuo Ohgaki — 2014-02-17T04:27:16+00:00 URL: https://externals.io/message/72643#72643 Hi all, This RFC changes default session settings and introduces a new setting that disables possible timing attack against session ID. All of them help to improve general session ID security except hash_bits_per_character change. NIST discourages use of SHA-1 years ago. It proposes to use SHA-256 as the default hash function for session. To reduce size of session ID string, hash_bits_per_character=6 is proposed. https://wiki.php.net/rfc/secure-session-options-by-default Thank you for voting! P.S. Although, the change is trivial, if anyone would like to see patch for this, I'll prepare one ASAP. -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72675 — Damien Tournoud — 2014-02-18T10:38:33+00:00 URL: https://externals.io/message/72643#72675 · In reply to: #72643 Hi there, I remember that the `use_strict_mode=on` added in PHP 5.5 was broken and would prevent the application from setting a session ID by itself. This is actually fairly common, about 800k Drupal 7 websites do that. Unless this is fixed, this setting just cannot be enabled by default. Damien Tournoud > Hi all, > > This RFC changes default session settings and introduces a new > setting that disables possible timing attack against session ID. All > of them help to improve general session ID security except > hash_bits_per_character change. > > NIST discourages use of SHA-1 years ago. It proposes to use > SHA-256 as the default hash function for session. To reduce > size of session ID string, hash_bits_per_character=6 is proposed. > > https://wiki.php.net/rfc/secure-session-options-by-default > > Thank you for voting! > > P.S. Although, the change is trivial, if anyone would like to see > patch for this, I'll prepare one ASAP. > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #72678 — Yasuo Ohgaki — 2014-02-18T10:49:26+00:00 URL: https://externals.io/message/72643#72678 · In reply to: #72675 Hi Damien, > I remember that the `use_strict_mode=on` added in PHP 5.5 was broken and > would prevent the application from setting a session ID by itself. This is > actually fairly common, about 800k Drupal 7 websites do that. > > Unless this is fixed, this setting just cannot be enabled by default. > It was simple mistake during merge. It is fixed next release already. Do not worry about it. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72679 — Tjerk Meesters — 2014-02-18T10:56:14+00:00 URL: https://externals.io/message/72643#72679 · In reply to: #72678 > Hi Damien, > > > > > […] > is > > […] > > It was simple mistake during merge. It is fixed next release already. Do > not worry about it. > It seemed to be part of the patch of the respective rfc: https://gist.github.com/yohgaki/1379668#file-php-trunk-strict-session-patch-L602 How was this a merge issue? > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > -- -- Tjerk --- ## #72680 — Yasuo Ohgaki — 2014-02-18T11:09:52+00:00 URL: https://externals.io/message/72643#72680 · In reply to: #72679 Hi Tjerk, On Tue, Feb 18, 2014 at 7:56 PM, Tjerk Meesters wrote: > It seemed to be part of the patch of the respective rfc: > > > https://gist.github.com/yohgaki/1379668#file-php-trunk-strict-session-patch-L602 > > How was this a merge issue? > Could you point out what's the exact issue here? It's gist and it's not the patch merged. tests/session_id_basic.phpt var_dump(session_id()); var_dump(session_id("test")); var_dump(session_id()); var_dump(session_id("1234567890")); var_dump(session_id()); // Turn off strice mode, since it does not allow uninitialized session ID ini_set('session.use_strict_mode',false); var_dump(session_start()); var_dump(session_id()); var_dump(session_destroy()); var_dump(session_id()); echo "Done"; `ob_end_flush()`; ?> --EXPECTF-- *** Testing `session_id()` : basic functionality *** string(0) "" string(0) "" string(4) "test" string(4) "test" string(10) "1234567890" bool(true) string(10) "1234567890" bool(true) string(0) "" Done It works like this. Is there issue with this behavior? Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72681 — Zeev Suraski — 2014-02-18T11:46:05+00:00 URL: https://externals.io/message/72643#72681 · In reply to: #72643 Yasuo, This was previously discussed but I have to agree with Andrey (and maybe even go beyond what he said) - the hash_bits change doesn't belong in this RFC. First, it has no security implications and it's not entirely clear from the RFC. Second, I don't feel that the implications of that change are clear, beyond some mention that "this could not be an issue for almost all apps", which personally I don't think is accurate - but either way, we need some better analysis here. From my point of view I don't think a few extra characters per session going over the wire are worth the potential obscure BC break changing the default here will cause with certain session backends and/or apps, and we shouldn't include this change in this RFC. If we want to compact the session id's, proposing a change for this default can be done in a separate RFC that discusses the pros and cons of doing it, independent of security. Another thing that I think this RFC is missing is some clearer explanation on what kind of apps *don't* work with the proposed changes (most notably use_strict_mode=on and cookie_httponly). Even though my gut is that these two proposed changes are good - the RFC should include explanations of the code patterns and/or types of apps and/or modules that will be affected by this; "Most apps should not be affected" isn't enough in an RFC IMHO. Last - the voting period should be at least a week, right now it's 5 (or maybe 6, depending on your POV) days. Thanks! Zeev > -----Original Message----- > From: yohgaki@gmail.com [mailto:yohgaki@gmail.com] On Behalf Of Yasuo > Ohgaki > Sent: Monday, February 17, 2014 6:27 AM > To: internals@lists.php.net > Subject: [PHP-DEV] [VOTE] Secure Session Module Options/Internal by > Default > > Hi all, > > This RFC changes default session settings and introduces a new setting > that > disables possible timing attack against session ID. All of them help to > improve > general session ID security except hash_bits_per_character change. > > NIST discourages use of SHA-1 years ago. It proposes to use > SHA-256 as the default hash function for session. To reduce size of > session ID > string, hash_bits_per_character=6 is proposed. > > https://wiki.php.net/rfc/secure-session-options-by-default > > Thank you for voting! > > P.S. Although, the change is trivial, if anyone would like to see patch > for this, > I'll prepare one ASAP. > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net --- ## #72682 — Yasuo Ohgaki — 2014-02-18T11:57:23+00:00 URL: https://externals.io/message/72643#72682 · In reply to: #72681 Hi Zeev, > This was previously discussed but I have to agree with Andrey (and maybe > even go beyond what he said) - the hash_bits change doesn't belong in this > RFC. First, it has no security implications and it's not entirely clear > from the RFC. Second, I don't feel that the implications of that change > are > clear, beyond some mention that "this could not be an issue for almost all > apps", which personally I don't think is accurate - but either way, we need > some better analysis here. > > From my point of view I don't think a few extra characters per session > going > over the wire are worth the potential obscure BC break changing the default > here will cause with certain session backends and/or apps, and we shouldn't > include this change in this RFC. If we want to compact the session id's, > proposing a change for this default can be done in a separate RFC that > discusses the pros and cons of doing it, independent of security. > I agree. This would be removed. This means vote is closed and reopened again. I'm not sure why compiled and php.ini-* default differs now, but it's irrelevant issue here anyway. Another thing that I think this RFC is missing is some clearer explanation > on what kind of apps *don't* work with the proposed changes (most notably > use_strict_mode=on and cookie_httponly). Even though my gut is that these > two proposed changes are good - the RFC should include explanations of the > code patterns and/or types of apps and/or modules that will be affected by > this; "Most apps should not be affected" isn't enough in an RFC IMHO. > I'll add explanations and possible side effects. I think of few. > Last - the voting period should be at least a week, right now it's 5 (or > maybe 6, depending on your POV) days. > This is mistake. My apologies. Thank you for your comment. Regards, -- Yasuo Ohgaki yohgaki@ohgaki.net --- ## #72683 — Tjerk Meesters — 2014-02-18T11:58:58+00:00 URL: https://externals.io/message/72643#72683 · In reply to: #72680 > Hi Tjerk, > > On Tue, Feb 18, 2014 at 7:56 PM, Tjerk Meesters wrote: > > > […] > > Could you point out what's the exact issue here? > It's gist and it's not the patch merged. > In that case, the RFC should be updated with the corresponding commit logs (part of the process) when it was merged with the project; this makes it easier to discuss historical commits based on it. > tests/session_id_basic.phpt > > var_dump(session_id()); > var_dump(session_id("test")); > var_dump(session_id()); > var_dump(session_id("1234567890")); > var_dump(session_id()); > // Turn off strice mode, since it does not allow uninitialized session ID > ini_set('session.use_strict_mode',false); > var_dump(session_start()); > var_dump(session_id()); > var_dump(session_destroy()); > var_dump(session_id()); > > echo "Done"; > `ob_end_flush()`; > ?> > --EXPECTF-- > *** Testing `session_id()` : basic functionality *** > string(0) "" > string(0) "" > string(4) "test" > string(4) "test" > string(10) "1234567890" > bool(true) > string(10) "1234567890" > bool(true) > string(0) "" > Done > > It works like this. Is there issue with this behavior? > That looks okay, thanks for getting back to me :) > > Regards, > > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > > -- -- Tjerk --- ## #72727 — Yasuo Ohgaki — 2014-02-21T00:15:13+00:00 URL: https://externals.io/message/72643#72727 · In reply to: #72683 Hi Tjerk, On Tue, Feb 18, 2014 at 8:58 PM, Tjerk Meesters wrote: > In that case, the RFC should be updated with the corresponding commit logs > (part of the process) when it was merged with the project; this makes it > easier to discuss historical commits based on it. Thank you for point it out. I'll update the RFC. If document is not updated, I'll update it, too. Regarding `session_id()` behavior, it could be made to accept user specified session ID independent of "use_strict_mode". To accept user specified session ID when "use_strict_mode=on", there should be an API that generates secure session ID. Otherwise, user may create insecure ID and set it. Any call for RFC, anyone? I'm willing to propose this. -- Yasuo Ohgaki yohgaki@ohgaki.net