[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87h7xt3a5s.fsf@silverfish.pri>
Date: Thu, 09 Apr 2020 08:06:07 +1000
From: Brian May <brian@...uxpenguins.xyz>
To: oss-security@...ts.openwall.com
Subject: Re: [CVE-2019-16782] Possible Information Leak / Session Hijack Vulnerability in Rack
"Alexander E. Patrakov" <patrakov@...il.com> writes:
> I don't understand why this is reported as something Rack-specific.
For reference, I noticed that Django does by default store sessions in
an index based database - such as mysql or postgresql. So I opened up a
bug report there: https://code.djangoproject.com/ticket/31412
I also believe ruby on rails is not vulnerable, at least if you
applications recommended best best practise.
http://blog.remarkablelabs.com/2012/12/activerecord-sessionstore-gem-extraction-rails-4-countdown-to-2013
> If I read the patch correctly (which is improbable, as I don't know
> Ruby at all), the idea is:
>
> 1. The attacker could send various bogus session ids, starting with
> all possible valid bytes. The database, if it uses a trie (yes,
> strawman example - is it used by any real-world database?) as a data
> structure to speed up looking up sessions, will terminate the
> comparison early on invalid bytes, thus disclosing them.
> 2. Given one valid byte of a session id, the attacker tries to extend
> it using the same procedure.
> 3. At the end, the attacker will get a full session ID.
>
> The patch works by making the thing stored in the database as a key
> not the session ID in the cookie, but a hash of it. Therefore, step 2
> fails, as it is computationally hard to find something with a given
> prefix.
>
> On the other hand, I don't see how a timing attack would be possible
> on the most common data structures (B-Tree and Hash) used for database
> indexes.
I have been looking at this in detail, and getting somewhat confused.
Especially after looking at the solution for this problem. This means I
am not confident in applying the upstream solution to older
distributions. It seems more invasive then required plus it breaks
existing code. Breaking code is never a good option for a security
update for an old distribution. Nor am I confident in creating a patch
of my own.
Applications appear to have a choice of 3 back ends to store session
information:
1. cookie based - stores data in a browser cookie.
2. pool based - stores data in a in memory hash.
3. memcache
As far as I can tell, neither 1 or 2 would be vulnerable. They don't use
a database that uses an index. In fact, I am not even sure 3 falls into
this category.
But the upstream patch only seems to update the pool backend:
https://github.com/rack/rack/commit/7fecaee81f59926b6e1913511c90650e76673b38
OK, the memcache was split up into another Gem, Dalli:
https://github.com/rack/rack/commit/54600771e3c9628c873fb1140b800ebb52f18e70#diff-ec7f0fcff10d701615d85df33fbbd545
But I don't see any security advisory against Dalli - did I miss
something?
https://github.com/petergoldstein/dalli
The patches for the older versions also patch the cookie based backend
too. The patches seem to be far my invasive then they need to be.
The way the patches were done seems to puzzle me. I would have thought
we just simple change to the database operations to use hash(sid) as the
primary key instead of sid. This is an internal value only and does not
need to be made accessible to the application. As such no API change
required.
However, instead we have introduced the terms "public_id" and
"private_id" - which have nothing to do with public key cryptography.
Instead private_id is the hash(public_id). Both need to be kept secret.
Due to this change, we have introduced an API change that makes no sense
to me.
Furthermore there is justification for this design that also doesn't
make a lot of sense, e.g.
https://github.com/rack/rack/issues/1432#issuecomment-571688819
"The private id could be leaked" - huh? If the public id is leaked it is
just as bad. In fact the private id is generated by hashing the public
id. In fact if the private id was leaked - if it weren't for the legacy
lookup, I am not sure there is anything you could do with it. You can't
reverse the hash and regenerate the public id.
"If you're storing the id, which do you want? (Probably not the public
id as you need to look the session up by the private id)" - why would
you want to lookup the session by the private id? Isn't this the job of
the session library?
There is also the aspect that the patch goes to lengths to preserve
existing sessions. But this doesn't make sense with the pool backend,
because sessions will get erased when the application in restarted for
the security update anyway. I am not convinced preserving existing
sessions should be a requirement.
--
Brian May <brian@...uxpenguins.xyz>
https://linuxpenguins.xyz/brian/
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
