Hi Andrey,
On Thu, Feb 20, 2014 at 10:03 PM, Andrey Andreev <[email protected]> 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.
>
> 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.
>
> > 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.
>
> >> 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.
>
> > - 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.
>
> On Tue, Feb 18, 2014 at 8:46 PM, Zeev Suraski <[email protected]> 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 :)
It improved a lot. Are we okay with session ID length issue now?
Better proposals are appreciated.
Regards,
--
Yasuo Ohgaki
[email protected]