Re: [VOTE] Secure Session Module Options/Internal by Default

On Fri, Feb 21, 2014 at 12:25 PM, Andrey Andreev <narf@devilix.net> wrote: > Hi Yasuo, > > On Thu, Feb 20, 2014 at 11:17 PM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrote: > > Hi Andrey, > > > > On Thu, Feb 20, 2014 at 10:03 PM, Andrey Andreev <narf@devilix.net> > wrote: > >> > >> > Length check is trivial, then why not check and discard by module > >> > instead of accepting invalid session ID and let users check and > discard? > >> > >> It is trivial, but also arguably useful. The reasoning for any > >> addition should be "what's the benefit" instead of "it's > >> trivial, so > >> why not". > >> > >> > >> > There is threat and it could be removed with length check. > >> > Let's have the check and forget about timing attack! > >> > >> Consider this scenario: > >> > >> - Attacker accesses the site without sending a session cookie > >> - Site responds, giving them a newly generated session ID (this is > >> where length becomes public). > >> - Attacker doesn't care about your length check, because instead of > >> sending <first char(s)>, they can now send str_pad(<first char(s)>, > >> <received session ID length>, '0') > > > > > > I'm not trying to hide length at all. What I'm trying to do is "do not > > compare strings up to id_length, so that attacker cannot analyze secret > > session ID char by char using timing up to id_length". > > > > The rest of chars (chars larger than id_length) can be analyzed by > timing. > > However, unless attacker could guess id_length part of secret, it's > useless. > > I feel like you're not reading a word of my arguments. > > Nobody has to guess the id length, as my quote below says: an attacker > experienced enough to do timing attacks will always send a string with > the full length. > They do not have to know any secret, nor would they care if one > exists. Your "id length" check will only "protect" you from a 13-year > old using some brute-force script that they just found. > > >> Anybody with the knowledge and resources to perform a successful > >> timing attack will bypass the length check without even knowing that > >> there is one. Bottom line is, length check will NOT prevent a timing > >> attack. > >> Also, you didn't actually answer to my other comments, this is what I > >> said: > >> > >> >> This still doesn't explain how it would work. > >> > >> Rephrasing: the RFC barely describes what id_length does and the name > >> itself isn't descriptive. > >> > >> > >> >> And why is it necessary as a setting instead of auto-detecting it? > The > >> >> valid length should be easy to calculate. > >> > >> Rephrasing: Don't make it a setting. Decide the (exact) length based > >> on the hash_function and hash_bits_per_character settings. You simply > >> don't need this to be configurable, and exposing it as an ini setting > >> can only cause issues for inexperienced users who change it. > > > > > > The reason why it is not automatic is user could add prefix to session > ID. > > When user uses prefix, automatic calculation may disable timing > protection. > > However, your proposal of automatic calculation is better. > > > >> > >> > >> Stas raised the same concern: > >> > >> > I could see it as an internal check - defensive coding and all - where > >> > length is pre-calculated and checked internally. This is fine - but > as a > >> > user setting that inevitably will be set wrong and cause trouble - I > just > >> > don't see the point. > > > > > > We may be better to have "id_additional_chars" instead of > "id_length(minimum > > ID length)", then automatic calculation can be done. This is better idea. > > It's not, a proper name would be something like "id_check_min_length". > > >> > The reason why I proposed this as user setting is user may have > prefixed > >> > session ID. If this is the case, internally set minimum length could > be > >> > useless. > >> > >> Why is it that you use this prefix as an argument for everything? This > >> isn't even a feature supported by PHP, it's a possible userland > >> implementation. > > > > > > We cannot ignore valid user implementations. Session ID prefixing is > useful > > for large sites to determine user of the session without additional > storage > > look up, for example. Users do not have to have separate storage to keep > > session ID and user ID relation with this. Keeping them in sync is not > > needed, too. Sessions are deleted by GC. Keep them in sync is not trivial > > unless there is special session save handler. > > > > <user-id-hash>-<session-id-hash> > > > > Many databases can perform efficient search for string prefix. > > > > There are users who use this. They have millions of users for each. > > You also can't build your whole logic around a single (and not as > widely used as you think) possible user implementation. And while I > can see how prefixing a session ID could be useful, a more effective > approach would be using the session name (cookie name) instead of a > prefix. > > Also, the example you're giving is absurd. A user_id should be stored > either in session data or as a separate database field in the same > table and fetched at the time of session ID validation. > > >> >> Why would they send short ID if they could send full-length ID and > >> >> easily defeat your checks? > >> >> > >> > > >> >3 reasons. > >> > - Hash used by session may fallback to SHA-1 from SHA-256 > >> > >> What?! Why would that happen? > > > > > > ext/hash could be DL module. There is #if for this case. > > This is a more serious flaw than the one you're trying to protect > against. I'd suggest one of the following: > > - Force ext/hash to be dynamically loaded when session wants to use it. > - Not give the ability to disable the extension at all. > - Emit E_WARNING when a user tries to use an unavailable algorithm. > > Actually, the last one should be done anyway, IMO. > > >> > >> > >> > - User may set hash to SHA-1 or md5 by mistake or intentionally > >> > >> Again, the length should be decided based on the setting. How the user > >> configures it is not your problem. > > > > > > Agreed. We should be better to have "id_additional_chars" (or better > name), > > then problem is solved. > > I'm confused, you can't both agree and suggest a different name. > Maybe I wasn't clear enough here, but the "the setting" I'm talking > about is hash_function+hash_bits_per_character. > > >> On Tue, Feb 18, 2014 at 8:46 PM, Zeev Suraski <zeev@zend.com> wrote: > >> > >> > 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. > >> > >> After the lengthy discussion, I think the real problem is the lack of > >> clarity about this in the RFC. > >> If the hash_function is changed to one producing a longer length > >> string, then increasing hash_bits_per_character would complement that > >> change to somehow maintain BC. > >> > >> For example, a custom session save handler using a database will most > >> likely have a varchar(40) field for the session_id. Switching to > >> sha256 and not changing hash_bits_per_character or the field's maximum > >> length will cause trouble. > > > > > > Yes. If user have trouble with new default, they should be able to use > old > > setting of their own. Changing database schema is headache. > > > >> > >> > >> IMO, this just needs to be properly described in the RFC. > > > > > > I'll add this. > > > >> > >> > >> Btw, the "Backward Incompatible Changes" section currently says > >> use_SCRIPT_mode instead of 'strict' - needs a fix as well. :) > > > > > > > > - hash_bits_per_character will be removed from the RFC. > > - Possible issue with ID length change will be documented. > > - "id_length"(Minimum session ID length) will be > > "id_additional_chars"(Session ID prefix length set by users. Default to > 0) > > and session ID length is calculated by settings. Automatic hash function > > fallback is took into account. > > - User may have variable length for prefix/postfix, the setting could be > > interval or minimum. Perhaps, id_additional_chars=16,32 for 16 to 32 > chars? > > id_additional_chars=0 by default. This setting could negate timing > > protection. This will be documented. > > - Spelling mistake will be fixed. If anyone notice any, please let me > know > > and/or fixing them in wiki is appreciated. Thanks :) > > You're again contradicting yourself. > > > It improved a lot. Are we okay with session ID length issue now? > > Better proposals are appreciated. > > No, you just both agreed to and then completely ignored my suggestions > in your "solution". > That's not an improvement, it's just confusion. > > Cheers, > Andrey. > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > Hi, any update on this? today would be the last day of the voting, but given that there are still active discussion/open issues and given the fact that nobody voted yet, I think it would be nice to either move back the RFC to under discussion, or at least extend the voting phase. -- Ferenc Kovács @Tyr43l - http://tyrael.hu