polonel / trudesk

:coffee: :seedling: Trudesk is an open-source help desk/ticketing solution.
http://trudesk.io
Other
1.31k stars 442 forks source link

Rate limiter temporary bans IP disregarding which username attempted to log in #547

Closed mrt-prodz closed 2 years ago

mrt-prodz commented 2 years ago

Is this a BUG REPORT or FEATURE REQUEST?:

What happened: If someone on the same network fails to log in multiple times the IP is banned temporary resulting in everybody unable to log in for an hour. A small company with a user who forgot their credentials could block the access of Trudesk to everybody.

What did you expect to happen: The IP combined with the username failing to log after multiple times should be banned, not only their IP. This would keep access to Trudesk for other people.

How to reproduce it (as minimally and precisely as possible): On a multi-devices network, make one device fail to log in multiple times until the rate limit kicks in. Try to log from another device with another username, Trudesk will deny its access.

Anything else we need to know?: A small fix would be to patch : https://github.com/polonel/trudesk/blob/5bbc42395de7d2589daa97d6837500826448ac11/src/controllers/main.js#L108-L109 And append the body username to the IP. This will result in the rate limit taking into account both IP and username blocking only the concerned username in case of bruteforcing from a single IP.

I will send a PR since I can't remember exactly what I did at work to patch this, it was something like :

let ipAddress = req.ip + req.body['username']

Do you see any issue with a patch like this ? It fixed the rate limit issue on our end where an employee would forget their credentials.

Environment:

polonel commented 2 years ago

Are you running Trudesk behind a reverse proxy?

See issue #536 which is not merged onto master yet.

mrt-prodz commented 2 years ago

Exactly, I have a docker with Caddy acting as a reverse proxy.

mrt-prodz commented 2 years ago

After looking at the patch I don't think it would solve the issue for us since we are all originating from the same IP.

If I understand the PR correctly it's forwarding the originating IP and letting express trust the IP of the proxy ?

polonel commented 2 years ago

It should forward the client's IP, I have not been able to test the patch externally yet, It's on my list before I merge with master.

mrt-prodz commented 2 years ago

Yes in that case it won't work with a company using Trudesk internally since all users originate from the same IP.

If a single user in said company try too many times and fails to login they will block the whole company from accessing Trudesk.

clackner-gpa commented 2 years ago

@mrt-prodz depending on what sits between the outside and inside of your system. If you have a proxy the proxy would need to be set up to forward the external IP address in the header for #536 to resolve this issue. I do think your approach would resolve this. Although I would appreciate the ability to turn that off since you are risking DDOS attacks and effectively disable the rate limits if someone changes the username on every attempt.

Edit just realized that if you are sitting behind a NAT you probably have no way of forwarding the IP in the header so you probably can't resolve this via IP.

mrt-prodz commented 2 years ago

@clackner-gpa Yes our configuration is as such :

[ multiple devices on the same public IP ] ---> [ remote server with reverse proxy -> trudesk docker on the same server ]

The reverse proxy is on the same server as our docker services / applications, we just use it to forward the domain name to the proper docker with exposed ports locally. That way we only have 80 / 443 ports open on that server from the outside.

536 should definitely fix the risk of Trudesk banning a legitimate proxy (if IP forwarding is configured on the proxy) or if we ran everything on our internal network.

But I don't think this patch by itself will solve the issue of multiple devices coming from the same public IP.

If we have 2 clients from the same public IP, and one of them is spamming the log in page, the IP will be banned either way disregarding which user attempted to log in. It happened here while testing Trudesk with a co-worker and he managed to block us both after 4 failed attempts.

Although I would appreciate the ability to turn that off since you are risking DDOS attacks and effectively disable the rate limits if someone changes the username on every attempt.

To be honest, I don't think any solutions talked so far will mitigate any real DDOS attacks either way, we recently encountered an attack where thousands of IP would cycle and try to login with over half a million users from some apparent database dumps compiled together. As soon as we would block whole IP ranges (we were desperate..), another thousand would spawn and continue the attack immediately.

What we have so far only protects from some basic single IP bruteforce attack on the login page.

clackner-gpa commented 2 years ago

I tried to include this in #536 via #551 I have not done extensive testing on it tho.

mrt-prodz commented 2 years ago

I personally like the fact that you can use environment variables to set (or not) USE_XFORWARDIP and USE_USERRATELIMIT.

I tested #551 and just changed :

ipAddress = ipAddress + req.body['username']

to

ipAddress = ipAddress + req.body['login-username']

I tested the following :

I couldn't test USE_XFORWARDIP behind a proxy to check if the forwarded IP was banned instead of the IP of the proxy.

It seems to work properly on my end.

mrt-prodz commented 2 years ago

I suppose I can close the issue since @clackner-gpa PR was merged. Thank you.