Hey,
I didn't want to interrupt a vote, but this 'id_length' setting was
not initially a part of the RFC and it's just now that I see it.
I don't see how it relates to timing attacks.
If it is about comparing at least N characters of the session ID
before rejecting one, then why not just compare all of them? ID length
is public information, anybody can see what it is by simply looking at
what the application gives them.
And finally, the setting name itself is misleading - it doesn't make
it clear that it's about minimum length.
Cheers,
Andrey.
Hi Andrey,
I didn't want to interrupt a vote, but this 'id_length' setting was
not initially a part of the RFC and it's just now that I see it.
I noticed possible attack during timing attack discussion. Therefore,
I've added this. User defined, 3rd party session or certain system's
save handler could be vulnerable.
Simplest one would be file system with liner directory search. It would
be possible to get 1st session ID on the system with timing attack.
I suppose this scenario is possible with embedded systems. If it is
affected, it would be very easy to attack by timing. We cannot assure
system, 3rd party or user save handler is timing safe because we
cannot control implementation. Setting minimum length removes
possibility of the attack.
I might post mail in different thread, sorry.
I don't see how it relates to timing attacks.
If it is about comparing at least N characters of the session ID
before rejecting one, then why not just compare all of them? ID length
is public information, anybody can see what it is by simply looking at
what the application gives them.
And finally, the setting name itself is misleading - it doesn't make
it clear that it's about minimum length.
I agree. Java has similar name setting with different semantics.
Name could be anything. Better name would be appreciated.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Hi Andrey,
I didn't want to interrupt a vote, but this 'id_length' setting was
not initially a part of the RFC and it's just now that I see it.I noticed possible attack during timing attack discussion. Therefore,
I've added this. User defined, 3rd party session or certain system's
save handler could be vulnerable.Simplest one would be file system with liner directory search. It would
be possible to get 1st session ID on the system with timing attack.
I suppose this scenario is possible with embedded systems. If it is
affected, it would be very easy to attack by timing. We cannot assure
system, 3rd party or user save handler is timing safe because we
cannot control implementation. Setting minimum length removes
possibility of the attack.I might post mail in different thread, sorry.
I don't see how it relates to timing attacks.
If it is about comparing at least N characters of the session ID
before rejecting one, then why not just compare all of them? ID length
is public information, anybody can see what it is by simply looking at
what the application gives them.
And finally, the setting name itself is misleading - it doesn't make
it clear that it's about minimum length.I agree. Java has similar name setting with different semantics.
Name could be anything. Better name would be appreciated.
This still doesn't explain how it would work.
And why is it necessary as a setting instead of auto-detecting it? The
valid length should be easy to calculate.
Regards,
Andrey.
Hi Andrey,
This still doesn't explain how it would work.
And why is it necessary as a setting instead of auto-detecting it? The
valid length should be easy to calculate.
Valid length is easy to detect as well as valid chars.
Current session module only detects and discards "if session has invalid
chars"
i.e. Invalid chars are only few chars. Stricter check is up to session save
handlers.
http://lxr.php.net/xref/PHP_5_6/ext/session/session.c#1605
(It would be better to have stricter check in module. IMO. It's BC...)
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 addresses
bug
like null session ID raises annoying error also.
Timing attack could be done with liner directory search. With liner
directory
search, file names are compared one by one, character by character.
Therefore,
attacker can exploit timing if sent session ID matches or not.
Modern file systems like ext4 have htree that hashes file names. Timing
cannot be
used such file systems because it is the same as "hash strings, then
compare".
Characters are not compared one by one.
XFS uses B+tree. B+tree is one by one comparison basically. B+tree would be
harder to exploit since its structure may change a lot during attack, but
it might
be vulnerable if attacker considers tree structure change during attack.
i.e. Tree
structure may change after GC or new sessions are added. (This is also
applicable to liner directory search file system. Structure change is more
obvious
and easy to handle with simple liner search.)
There is threat and it could be removed with length check.
Let's have the check and forget about timing attack!
Regards,
P.S. The reason why Python adopts "hash comparison" with ==/=== is the
same reason, I suppose. PHP 5.6 may do the same or similar, and forget
about timing attack.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
Length check is trivial, then why not check and discard by module instead
I don't really understand - what is the point of checking length? If you
have session adoption problem, checking length won't help since the
length is public. If you don't have this problem, checking length adds
nothing since the session won't be valid anyway. So why add code that
contributes nothing? Maybe I do not see some contribution, if so, please
explain.
There is threat and it could be removed with length check.
Let's have the check and forget about timing attack!
But there's no actual real-world threat - there is an imagined scenario
which requires use of antique filesystems with very specific and
unobvious properties (namely, directories stored sorted by name and
having linear checks), and even in this case length check does not help,
since again, session ID length is public and so nothing prevents one
from using the correct length from the start. So length check does not
fix the timing attack even in the unlikely case such attack would
actually exist. It's like putting a locked door in an open field, and
claiming it protects from robbers, despite there being nothing to be
robbed and the door can be easily walked around. I really don't see a
point here, especially as a user setting. 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.
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Tue, Feb 18, 2014 at 9:40 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Length check is trivial, then why not check and discard by module
insteadI don't really understand - what is the point of checking length? If you
have session adoption problem, checking length won't help since the
length is public. If you don't have this problem, checking length adds
nothing since the session won't be valid anyway. So why add code that
contributes nothing? Maybe I do not see some contribution, if so, please
explain.
The reason why I would like to add length check is "use_strict_mode"
suffers grater risk of timing attack. I'll use the same scenario in
previous mail.
When "use_strict_mode=on", attacker sends short session ID many times to
see if the char matches part of session ID or not. When underlying data
storage is vulnerable to timing attack, timing is leaked. i.e.
stat(session_data_file_name) could be timing unsafe.
When "use_strict_mode=off", attacker sends session ID character one by one
and all of them are accepted as valid session data files. Since attacker's
invalid short session ID data is created as valid session ID data, attacker
cannot measure timing difference for the targeted session ID (1st one in
the directory entry) Window for attacker is rather small, but attacker may
succeed as long as timing is not safe, though.
There is threat and it could be removed with length check.
Let's have the check and forget about timing attack!
But there's no actual real-world threat - there is an imagined scenario
which requires use of antique filesystems with very specific and
unobvious properties (namely, directories stored sorted by name and
having linear checks), and even in this case length check does not help,
since again, session ID length is public and so nothing prevents one
from using the correct length from the start. So length check does not
fix the timing attack even in the unlikely case such attack would
actually exist. It's like putting a locked door in an open field, and
claiming it protects from robbers, despite there being nothing to be
robbed and the door can be easily walked around. I really don't see a
point here, especially as a user setting. 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.
htree is safe. Weak hash could be issue. Most of us remember hash
collision attack few years ago. (e.g mm save handler uses very simple hash.
It would be difficult to attack still, though.) However, B+tree could be
attackable. B tree is worse. User may use B+tree/B tree based session
storage and it is not obsolete/rare unlike liner directory search.
With minimum length, attacker must guess char combinations of that length
regardless of underlying session data storage.
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.
Minimum session ID length solves all issues/anxieties and we may leave
timing attack behind as long as users set appropriate value to it.
Regards,
P.S. I would like to close doors for attacker as many as possible if
counter measure is feasible. This is my intention.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
When "use_strict_mode=on", attacker sends short session ID many times to
see if the char matches part of session ID or not. When underlying data
storage is vulnerable to timing attack, timing is leaked. i.e.
stat(session_data_file_name) could be timing unsafe.
Why would they send short ID if they could send full-length ID and
easily defeat your checks?
htree is safe. Weak hash could be issue. Most of us remember hash
collision attack few years ago. (e.g mm save handler uses very simple
Hash collistion attacks have nothing to do with this, they rely on the
fact that hash we are using is not perfect and it is easy to generate
colliding strings, thus producing degenerate performance. However, in
this case we're talking about completely different scenario.
With minimum length, attacker must guess char combinations of that
length regardless of underlying session data storage.
Not really. The whole point of timing attack is that the underlying
mechanism must not use all chars of the string for comparison.
Minimum session ID length solves all issues/anxieties and we may leave
timing attack behind as long as users set appropriate value to it.
I still don't see even one issue it solves. Timing attack would still
work the same if underlying mechanism uses comparison where time of
comparing aaaa to bbbb (assuming length is 4) if different from timing
of comparing aaaa to aaab. Different length is not required here, and
since length is public, it won't be the factor in the attack anyway.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Tue, Feb 18, 2014 at 1:11 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:
When "use_strict_mode=on", attacker sends short session ID many times to
see if the char matches part of session ID or not. When underlying data
storage is vulnerable to timing attack, timing is leaked. i.e.
stat(session_data_file_name) could be timing unsafe.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
- User may set hash to SHA-1 or md5 by mistake or intentionally
- 23 chars would be enough for timing attack counter measure
23 chars works with md5 and hash_bits_per_character=6 also.
htree is safe. Weak hash could be issue. Most of us remember hash
collision attack few years ago. (e.g mm save handler uses very simple
Hash collistion attacks have nothing to do with this, they rely on the
fact that hash we are using is not perfect and it is easy to generate
colliding strings, thus producing degenerate performance. However, in
this case we're talking about completely different scenario.With minimum length, attacker must guess char combinations of that
length regardless of underlying session data storage.Not really. The whole point of timing attack is that the underlying
mechanism must not use all chars of the string for comparison.Minimum session ID length solves all issues/anxieties and we may leave
timing attack behind as long as users set appropriate value to it.I still don't see even one issue it solves. Timing attack would still
work the same if underlying mechanism uses comparison where time of
comparing aaaa to bbbb (assuming length is 4) if different from timing
of comparing aaaa to aaab. Different length is not required here, and
since length is public, it won't be the factor in the attack anyway.
When minimum ID length is used, session module discards shorter session ID
immediately. It does not ask save handlers to check ID at all. i.e. No
stat(session_data_file) nor query to database. It creates new ID before it.
Therefore, attacker must guess random string up to minimum ID length. It is
safe with the same reason for timing safe string comparison.
if (strlen(a) != strlen(b)) {
// It leaks length, but this is timing safe, since it's not comparing
string at all.
return FAILURE;
}
if (!timing_safe_compare(a, b, len)) {
return FAILURE;
}
return SUCCESS;
Minimum ID length would do something like
if (min_id_len > strlen(session_id_string)) {
// Timing safe up to min_id_len because it's not comparing string up to
min_id_len.
create_new_id(); // Discard invalid ID and create new ID
}
start_initialize_session(); // Call save handlers
User land solution is possible. To do the same job as session module, user
has to check $_COOKIE, $_GET and $_POST before calling session_start() to
see if session ID is in there according to session related INI. If there
is, check supplied session ID is longer than minimum ID length. If it's
invalid, generate secure session ID and set it via session_id(). Finally,
call session_start().
We don't have API for generating secure session ID currently also. This
would be too much for users to make session timing safe.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi again,
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')
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.
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.
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.
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?
- 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.
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.
IMO, this just needs to be properly described in the RFC.
Btw, the "Backward Incompatible Changes" section currently says
use_SCRIPT_mode instead of 'strict' - needs a fix as well. :)
Cheers,
Andrey.
Hi Andrey,
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.
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
yohgaki@ohgaki.net
Hi Yasuo,
Hi Andrey,
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_WARNINGwhen 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.
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.
Hi Andrey,
On Thu, Feb 20, 2014 at 11:17 PM, Yasuo Ohgak
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.
If underlying storage does liner search, you are right.
I was thinking this for mm handler, but it's useless if there is liner
search :(
Instead of having this, we should advise users to use good hash index.
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_WARNINGwhen a user tries to use an unavailable algorithm.Actually, the last one should be done anyway, IMO.
OK. Third one seems the one to me, too.
- hash_bits_per_character will be removed from the RFC.
- Possible issue with ID length change will be documented.
- "id_length" will be removed. User should use hash index search.
- Raise error when hash function is not available.
Raising error seems not good...
If hash extension is available always, it might be easier for other module,
too.
Do not allow DL/disabling hash extension, perhaps?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
- 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_WARNINGwhen a user tries to use an unavailable algorithm.Actually, the last one should be done anyway, IMO.
OK. Third one seems the one to me, too.
- hash_bits_per_character will be removed from the RFC.
- Possible issue with ID length change will be documented.
- "id_length" will be removed. User should use hash index search.
- Raise error when hash function is not available.
I'd be happy with that, yes. :)
Although for hash_bits_per_character, I only insisted the RFC to
explain that this is a change complementing hash_function.
Raising error seems not good...
If hash extension is available always, it might be easier for other module,
too.
Do not allow DL/disabling hash extension, perhaps?
I see no reason why anybody would want to disable the Hash extension,
so sure - I'd remove that choice.
However, silent fallback for stuff like this is never a good thing and
somebody might i.e. make a typo while configuring, so why not raise
E_WARNING for invalid hash_function regardless?
Cheers,
Andrey.
Hi Andrey,
- hash_bits_per_character will be removed from the RFC.
- Possible issue with ID length change will be documented.
- "id_length" will be removed. User should use hash index search.
- Raise error when hash function is not available.
I'd be happy with that, yes. :)
Although for hash_bits_per_character, I only insisted the RFC to
explain that this is a change complementing hash_function.
OK. I'll document it.
Raising error seems not good...
If hash extension is available always, it might be easier for other
module,
too.
Do not allow DL/disabling hash extension, perhaps?I see no reason why anybody would want to disable the Hash extension,
so sure - I'd remove that choice.
I agree. It will be added to the RFC.
However, silent fallback for stuff like this is never a good thing and
somebody might i.e. make a typo while configuring, so why not raise
E_WARNINGfor invalid hash_function regardless?
Make sense to me, too.
I'll make it raise error for any invalid hashes.
Thank you for your help :)
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Andrey,
- hash_bits_per_character will be removed from the RFC.
- Possible issue with ID length change will be documented.
- "id_length" will be removed. User should use hash index search.
- Raise error when hash function is not available.
I'd be happy with that, yes. :)
Although for hash_bits_per_character, I only insisted the RFC to
explain that this is a change complementing hash_function.OK. I'll document it.
Raising error seems not good...
If hash extension is available always, it might be easier for other
module,
too.
Do not allow DL/disabling hash extension, perhaps?I see no reason why anybody would want to disable the Hash extension,
so sure - I'd remove that choice.I agree. It will be added to the RFC.
However, silent fallback for stuff like this is never a good thing and
somebody might i.e. make a typo while configuring, so why not raise
E_WARNINGfor invalid hash_function regardless?Make sense to me, too.
I'll make it raise error for any invalid hashes.Thank you for your help :)
Well, I'm glad we finally sorted it out.
Although this was all my personal input, somebody else might disagree
on the non-disableable ext/hash. :)
Cheers,
Andrey.
And about to remove these options, should they be keep in the newest
releases?
; http://php.net/session.bug-compat-42
session.bug_compat_42 = Off
; session value into the global space. session.bug_compat_42 must be
enabled before
; http://php.net/session.bug-compat-warn
2014-02-24 7:00 GMT-03:00 Yasuo Ohgaki yohgaki@ohgaki.net:
Hi Andrey,
- hash_bits_per_character will be removed from the RFC.
- Possible issue with ID length change will be documented.
- "id_length" will be removed. User should use hash index search.
- Raise error when hash function is not available.
I'd be happy with that, yes. :)
Although for hash_bits_per_character, I only insisted the RFC to
explain that this is a change complementing hash_function.OK. I'll document it.
Raising error seems not good...
If hash extension is available always, it might be easier for other
module,
too.
Do not allow DL/disabling hash extension, perhaps?I see no reason why anybody would want to disable the Hash extension,
so sure - I'd remove that choice.I agree. It will be added to the RFC.
However, silent fallback for stuff like this is never a good thing and
somebody might i.e. make a typo while configuring, so why not raise
E_WARNINGfor invalid hash_function regardless?Make sense to me, too.
I'll make it raise error for any invalid hashes.Thank you for your help :)
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
--
- Marcel Araujo Analista de SistemasDesenvolvedor
PHP/Zend/JavaScript/jQuery/NodeJSLinux User
#490101http://www.twitter.com/marcelarauj0
http://www.twitter.com/marcelarauj0 http://blog.marcelaraujo.me
http://blog.marcelaraujo.me/http://br.linkedin.com/in/marcelaraujo
http://br.linkedin.com/in/marcelaraujo *
And about to remove these options, should they be keep in the newest
releases?; http://php.net/session.bug-compat-42
session.bug_compat_42 = Off; session value into the global space. session.bug_compat_42 must be
enabled before
; http://php.net/session.bug-compat-warn
These have been removed in PHP 5.4.
Best regards
Rouven
Hi Yasuo,
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_WARNINGwhen 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.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
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.--
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
Hi Ferenc,
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.
I'll change status to discussion.
Thank you.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net