PerlDancer / Dancer

The easiest way to write web applications with Perl (Perl web micro-framework)
http://perldancer.org/
739 stars 210 forks source link

Session and ENV-variables behaving massively insecure #989

Open Brennwert opened 10 years ago

Brennwert commented 10 years ago

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.

bigpresh commented 10 years ago

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.

bigpresh commented 10 years ago

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.

Brennwert commented 10 years ago

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.

bigpresh commented 10 years ago

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?

Brennwert commented 10 years ago

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?

Brennwert commented 10 years ago

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.

Brennwert commented 10 years ago

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:

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

Brennwert commented 10 years ago

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.

bigpresh commented 10 years ago

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.

jacqueslareau commented 10 years ago

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.

yanick commented 10 years ago

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...

jacqueslareau commented 9 years ago

Happened again. Also: http://www.perlmonks.org/?node_id=1088799

Brennwert commented 9 years ago

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?

caribnow commented 9 years ago

Hey team, is there any news on this issue?