Ola Lundqvist <ola@inguza.com> writes:
> I have looked into this some but I have not been able to determine how long
> the session ids were before the fix. Do anyone have that information?
> Based on that we can rather easily determine how long time a timing attack
> would take. My guess is a really long time.
My understanding is increasing.
The problem is when we lookup the session details from the database:
@pool[sid]
As sid is untrusted data, it means that can attacker can carefully
control the value and find out information on existing sessions based on
timing the query.
This patch would change that database lookup to:
@pool[sid.private_id]
Where sid.private_id = "#{ID_VERSION}::#{hash_sid(public_id)}"
This means the timing attack is harder (impossible?), because an
attacker cannot create good test data for the database lookup.
The problem with this approach however is we now have broken all
existing sessions. Because we have already stored the public id, not the
private id in the database. So the upstream solution is to use:
@pool[sid.private_id] || @pool[sid.public_id]
If the private_id lookup fails, the public_id lookup will be tried.
In https://github.com/rack/rack/issues/1431 the issue is raised that the
code still does the public_id lookup, which is what was responsible for
the bug in the first place. However the counter claim is that we have
already done the private_id lookup, and that is going to make timing
attacks hard.
If I understand this correctly, the goal will be to remove the public_id
lookup eventually.
Also, as upstream changed sid from a string to a more complicated
object, this breaks the current API. There are other possible solutions,
but as mentioned in
https://github.com/rack/rack/issues/1432#issuecomment-571688819 these
could lead to silent breakage, which is probably not a better outcome.
--
Brian May <bam@debian.org>