Session and ENV-variables behaving massively insecure #989

Open
Brennwert opened this Issue Jan 14, 2014 · 14 comments

Comments

Projects
None yet
5 participants

My Dancer-Implementation shows a misbehavior in session handling and the content of request->user_agent.

I recognized being already logged in even at devices connecting to the service for the first time. Changing session engines did not solve this behavior. Sooner or later I experienced wrong sessions with YAML, memcached and cookies. To work around the problem I enabled saving the user-agent after session-creation and comparing it to the current one in before-hook for each request, to destroy the session in this case:

-- Save after login:
session userAgent => request->user_agent;

[...]

-- Check for mismatch:
if (session('user') && (request->user_agent ne session('userAgent'))) {
  error 'CURRENT AGENT: '.request->user_agent;
  error 'SESSION-AGENT: '.session('userAgent');
  session->destroy;
}

Now see what I just catched:

[19066] error @0.003552> [hit #18] CURRENT AGENT: Mozilla/5.0 (Linux; U; Android 4.3; en-de; HTC_One Build/JSS15J) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30, Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36 in /blip/lib/blop.pm l. 160

[19066] error @0.003772> [hit #18]SESSION-AGENT: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36 in /blip/lib/blop.pm l. 161

The content of request->user_agent has become a mix of some previous content of HTTP_USER_AGENT (earlier request in some other session on mobile device) followed by a comma, a space and the real value of HTTP_USER_AGENT. Seems like an array-output. Once it happened, this behavior persists for every request until the app gets killed and restarted.
I'm pretty sure that something very similar mixes up my sessions, too. ;)

This seems to only happen after some hours of runtime (at low to nearly no traffic). I could never reproduce it immediatelly after start. The instance resides behind an nginx, if this may matter somehow. But restarting nginx does not solve the problem, so I guess it's Dancer having accidently stored something in one or more arrays.

Owner

bigpresh commented Jan 14, 2014

That's... bizarre, and worrying.

It might be good to log the session ID and not destroy the session so you could see what the session data contained (e.g. for the YAML engine, just looking at the appropriate session file) to look for any clues.

I'm wondering if it's possible that some (dodgy) client sent a request with two User-Agent headers, and request->user_agent returned a list of both - perhaps we should test that possibility.

Owner

bigpresh commented Jan 14, 2014

Hmm - tested, if I send two User-Agent headers, of 'Foo' and 'Bar', I get back a string of Foo, Bar from request->user_agent.

More alarming is what you mentioned at the beginning of your message:

I recognized being already logged in even at devices connecting to the service for the first time.

Do you mean accessing from a device which you know does not have a session cookie, and yet being logged in? This should of course be impossible; I can't see how this could happen, unless you were somehow seeing session ID collisions - but the session IDs are generated from, OTTOMH, the time, the process ID, and a large random number - so you're more likely to win the lottery while being hit by lightning twice.

Thanks man. Could you try another request following afterwards with only one header 'Baz'? Is 'Foo' or 'Bar' magically added then? ;)

Looking into session-data may be a good idea, I just enabled ID-logging and YAML-engine. Let's see.

And I'm afraid that yes, I'm totally sure. Fresh device that never knew the address before got an existing session immediatelly - also session('user') was printed out on the interface, containing valid content.
I guess in some combination session-values get stuck just like the request-value, lying in a variable-stack and never being popped afterwards.

Owner

bigpresh commented Jan 15, 2014

Brennwert notifications@github.com wrote:

Thanks man. Could you try another request following afterwards with
only one header 'Baz'? Is 'Foo' or 'Bar' magically added then? ;)

Will try that in the morning (and also dump all of request->env etc to look for oddness there).

And I'm afraid that yes, I'm totally sure. Fresh device that never knew
the address before got an existing session immediatelly - also
session('user') was printed out on the interface, containing valid
content.

Couldn't be nginx caching responses and just serving up a previous response could it?

It's jammed again right now, so I just connected directly to Dancer without using nginx.
The client (Lynx) gets the stuck agent's session, without having visited before:

CURRENT AGENT: Mozilla/5.0 (Linux; U; Android 4.3; en-de; HTC_One Build/JSS15J) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30, Lynx/2.8.8dev.5 libwww-FM/2.14 SSL-MM/1.4.1 GNUTLS/2.8.6

SESSION-AGENT: Mozilla/5.0 (Linux; U; Android 4.3; en-de; HTC_One Build/JSS15J) AppleWebKit/534.30 (KHTML, like Gecko) Version/4.0 Mobile Safari/534.30

SESSION-ID: 929805679490924489226805302673219568

There's nothing wrong with the session's YAML-file, created yesterday, having a stored login - looking totally normal. After deleting it, session-handling seems to be OK. Also Lynx is asking to accept a new cookie and shows login-page then. BUT user-agent-string is still mixed up.

So it seems that when Dancer is in this jammed state and the client provides no session-ID, it automatically gets the stuck user agent's session. I can't imagine that Dancer authenticates anything by user agent at any point, so I guess that it's just not the only thing that gets accidently "cached".

All occasions I observed until now where involving an Android phone's session (HTC One and Sensation), I'll try to see if it also happens without them.
Do you have another hint for me on how and what to print out for debugging?

Okay, indeed that seems to happen only with a mobile device involved (at least Android Browser >= 4.0.3). All ENV-parameters seem to get stuck then and are part of every following request, including $env->{COOKIE} and/or $env->{HTTP_COOKIE}. This is where it gets really really bad, guys.

I added the following to see what it will contain exactly:

use Dancer::SharedData;
my $request = Dancer::SharedData->request;
my $env = (defined $request) ? $request->env : {};
my $env_str = $env->{COOKIE} || $env->{HTTP_COOKIE};
debug "ENV_STR: $env_str";

Still dunno how to force the wrong behavior, so it will take some time.

Does anyone know how to reliably flush the whole ENV after every request?
Dancer::SharedData->reset_all() does not seem to do that, but I still don't really understand the code there.

As suggested, it contains the stuck cookie and the correct one following. The second value is the one being preferred, so users get their session when they provide a cookie.

ENV_STR: blip.session=32764166330640460646172206896321349, blip.session=825751984813316473289243524302533521

After deleting my browser cookie and coming around without providing one, I get the mobile phone's session AND I get the cookie saved to my browser for further exploitation. ;)

ENV_STR: blip.session=32764166330640460646172206896321349, blip.session=32764166330640460646172206896321349

So here's what I can conclude:

  • Take a Dancer-instance that's running for a few hours (I don't get why this has a matter, but it has)
  • Take an Android device, use the native browser to be sure
  • Click around in your app a bit and have your device's %ENV suddenly burned into Dancer
    (Some requests come double because of the known ghost-click-phenomenon. I assume this is fucking things up at some point.)
  • Enjoy your cookie being shared with the poor people not having one ;)

I'll try to fix it, but never looked at Dancer's code before yesterday. So any help would be appreciated. :)

@Brennwert Brennwert added a commit to Brennwert/Dancer that referenced this issue Jan 21, 2014

@Brennwert Brennwert Reset %ENV in purging accessor e15b1c8

The above change solved the problem for me. I did not recognize any other side-effects until now, running nicely for 2 days. Resetting %ENV in after-hook works, too.
Resetting $env does not btw, it really is %ENV not being purged correctly.

Owner

bigpresh commented Jan 21, 2014

Thanks! Will have to check out that PR locally and run the tests, as it seems from Jenkins that it caused quite a few failures.

It this fixed in the current version?
Something like this happened to me while using Cookie sessions. Tried switching to DB sessions and everything worked fine....until now.

A user got the session of another one. That's all I can say because it's very hard to go back and look debug this stuff. Session data is ok. I just don't know how to reproduce this behavior.

Could it be higher than Dancer, like Starman sharing a worker? My setup Apache -> Starman -> Dancer.

This is really alarming.

Contributor

yanick commented Aug 23, 2014

It looks like something is saving its state in %ENV, which causes it to stay across requests. Unfortunately, we can't just nuke %ENV totally, as it's also used (this time legitimately) for app-wide configurations.

I've looked at the core Dancer code, and (as far as I could see) we just read from %ENV, and never push anything to it during requests. So I suspect the session module or a plugin is to blame here. I'll try to have a look at D::Session::Cookie...

I experienced this behavior with various session-engines (YAML, memcached and cookies). Please don't bother digging in Session::Cookie. ;)
And as mentioned somewhere above it can't be higher, I could isolate the problem to pure Dancer without any middleware.

Still no probs in my environment with nuking %ENV for every request btw. :) What app-wide configs are stored there?

Hey team, is there any news on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment