RocketChat / Rocket.Chat

The communications platform that puts data protection first.
https://rocket.chat/
Other
40.22k stars 10.42k forks source link

Cookie is not secure #10402

Open rasos opened 6 years ago

rasos commented 6 years ago

We've been running a simple penetration test with https://pentest-tools.com/ against a RC 0.62 with NGINX reverse proxy. It reveals a security risk, as the connect.sid cookie does not have a secure flag set.

This could be probably modified by the proxy, but I think it should be better done in the backend.

Since the Secure flag is not set on the cookie, the browser will send it over an unencrypted channel (plain HTTP) if such a request is made. Thus, the risk exists that an attacker will intercept the clear-text communication between the browser and the server and he will steal the cookie of the user. If this is a session cookie, the attacker could gain unauthorized access to the victim's web session.

geekgonecrazy commented 6 years ago

this one is kind of tough. We can't just start flagging secure... Then everyone that isn't using https will suddenly not have cookies sent.

This one would need a bit more logic worked out before we could implement

TwizzyDizzy commented 6 years ago

I agree with @geekgonecrazy there. In order for Rocket.Chat to know what is spoken to the client's side by the reverse proxy (usually nginx / apache) the proxy would have to declare this and set a header accordingly. Rocket.Chat would then need to honour that header and set the secure flag.

The de-facto standard to indicate to backends that there is HTTPS in the front, there is the X-Forwarded-Proto header. Also this is relevant: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded

Cheers Thomas

rasos commented 6 years ago

How about an option for administrators:

TwizzyDizzy commented 6 years ago

The problem there is, is that hidden behind this little checkbox there is so much potential complexity (and possibilities of your setup) that it's not worth anything ... yes, you would reach your goal with that but to 100 other users this would be confusing and potentially kill their setup.

I would vote for supporting the Forwarded and X-Forwarded-* headers and setting the cookie flags on that basis automatically. This enables advanced users to control Rocket.Chat behaviour from the proxy side...

Only my thoughts, though... would welcome any further input.

Cheers Thomas

otakuu commented 6 years ago

Why not Secure-Flag as an option?

geekgonecrazy commented 5 years ago

Going to go ahead and close this one. If someone could add compelling case maybe it would make sense why we would need it

TwizzyDizzy commented 5 years ago

Hey Aaron,

good to see you on a closing-frenzy, yet for this one I'll vote against closing it.

Setting the secure-flag on cookies in HTTPS connections is a non-controversial best-practice and the handling I described in my last post should be implemented.

You could still fall back to the old behaviour in case of an absence of these headers.

Cheers Thomas

geekgonecrazy commented 5 years ago

I’m not sure how feasible this would be. Since a deeper meteor thing I think. But will re-Open

localguru commented 5 years ago

@TwizzyDizzy How would the nginx config would look like?

I tested

proxy_set_header X-Forward-Proto https;

in my nginx config, but testssl.sh criticized "Cookie(s) 1 issued: NOT secure, 1/1 HttpOnly"

localguru commented 5 years ago

What about

proxy_cookie_path / "/; secure; HttpOnly";

for nginx?

TwizzyDizzy commented 5 years ago

but testssl.sh criticized "Cookie(s) 1 issued: NOT secure, 1/1 HttpOnly"

Yes. I don't know if that's a Meteor issue or Rocket.Chat issue, but the behaviour I described above ("controlling RC behaviour from proxy side by headers") is not implemented yet, hence the cookie is still not secure.

What about ... for nginx

Indeed, one could modify the cookie headers manually but ... I mean... come on!

Cheers Thomas

Karreg commented 3 years ago

Any news about this issue? I'm pretty surprised security is not hardened in 2021. http should not be blocking security increase?