getkirby-v2/toolkit

Session fingerprint has quite bad defaults

bungle opened this issue · 6 comments

bungle commented

I was trying to figure out why I couldn't login to panel, and it seems it is because of this:

return sha1(Visitor::ua() . (ip2long($_SERVER['REMOTE_ADDR']) & ip2long('255.255.0.0')));

https://github.com/getkirby/toolkit/blob/master/lib/s.php#L120

REMOTE_ADDR is not quite what you should rely on (because of NATs and proxies — both reverse and forward proxies, and things like Tor). Also a lot of people use both IPv6 and IPv4 these days (reverse proxies may proxy to IPv4 or IPv6 address etc.). If you really want to check the remote address, then you need to take into consideration headers like:

  • CF-Connecting-IP
  • Fastly-Client-IP
  • Incap-Client-IP
  • ...
  • X-Real-IP
  • X-Forwarded-For

And even if you consider supporting the above, they still share most of the problems described above. There is a new stardard to this as well: https://tools.ietf.org/html/rfc7239 (aka Forwarded HTTP header), but that doesn't help either.

I propose that the default implementation should just be (a bit less secure for sure, but not much, and a lot more correct):

return sha1(Visitor::ua());
bastianallgeier commented

This has actually been a recommendation from OWASP, but we run into issues because of it quite often. @lukasbestle what do you think about this?

lukasbestle commented

The fingerprint is already configurable with a custom function, so I don't think we should compromise security here.

I agree with @bungle that it's currently not optimal, but just removing it all-together isn't a solution either.

bungle commented

Well, it is plain_wrong in this current form. I'm pretty sure you cannot find a solution that leverages on REMOTE_ADDR that will work correctly.

One could argue that the current state of the security in this session library is plausible even with that easily breaking fingerprint. You should definitely look at things like:

  1. Encrypting the session
  2. HMACing the session
  3. Adding support for SameSite flags

Before we can even start to talk about the security.

In my opionion:

  1. You should make a quick fix, and remove that check as a default and add people instructions how to add that IPv6 etc. breaking code back as a custom function.
  2. Fix that session library's security issues
  • 👍1
tmugford commented

I’ve just been bitten by this as a result of traffic being proxied via Cloudflare. Presumably you could try and detect the origin IP by checking for the existence of the various headers in Visitor::ip() and then using that in S::fingerprint(), though that’s probably too brittle a solution—and there’s IPv6 to consider as well of course.

I ended up using the form that @bungle suggested i.e.

s::$fingerprint = function(){
  return sha1(Visitor::ua());
};

…and it seems to be working well so far.

lukasbestle commented

That solution should actually be fine today. I'm pretty sure the OWASP recommendation still comes from a time when TLS wasn't as widely available, so session tokens needed to be protected against MITM attacks.

Because we don't really see an attack vector anymore and the feature was so unreliable, we have decided for Kirby 3 that it's not worth it anymore to have this feature, so our new session implementation in Kirby 3 won't have it. :)

tmugford commented

Makes sense. Keep up the great work guys.