Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qbittorrent-nox + Nginx (config from wiki) + 5 failed logins = ban of 127.0.0.1 #11908

Closed
Kahana82 opened this issue Jan 22, 2020 · 26 comments
Closed
Labels
Confirmed bug An issue confirmed by project team to be considered as a bug Core WebAPI WebAPI-related issues/changes

Comments

@Kahana82
Copy link

Kahana82 commented Jan 22, 2020

Please provide the following information

qBittorrent version and Operating System

qBittorrent v4.1.7 Web UI (qbittorrent-nox/now 4.1.7-1 arm64)
OMV5 (openmediavault/usul,usul,now 5.2.3-2)
Debian Buster (ARM64)

If on linux, libtorrent-rasterbar and Qt version

libtorrent-rasterbar9/now 1.1.13-1+b1 arm64 [installed,local]
Qt N/A

What is the problem

I'm using the Nginx /qbt/ location configuration from the official wiki page inserted in OpenmediaVault's generated server config for nginx. (I'm aware this is probably not the best practice)

It seems that qbittorrent only sees 127.0.0.1 as ip address connecting to it when using /qbt/
When connecting using https://server:port then qbittorrent sees the correct ip address.
This came to my attention when making 5 wrong login attempts through the WebUI (was planning on setting a jail up for Fail2Ban) and got banned on localhost (127.0.0.1)

What is the expected behavior

qbittorrent should log/see the correct ip address (not localhost) when using the /qbt/ nginx location, so that a ban of localhost would not occur.

Steps to reproduce

Make 5 wrong login attempts through the WebUI to get banned.
Check qbittorrent.log (/home/qbittorrent/.local/share/data/qBittorrent/logs/qbittorrent.log) to verify which ip was banned.

Extra info(if any)

Log output:
--- with nginx /qbt/

(W) 2020-01-22T15:59:39 - WebAPI login failure. Reason: invalid credentials, attempt count: 5, IP: ::ffff:127.0.0.1, username: F2B
(W) 2020-01-22T15:59:43 - WebAPI login failure. Reason: IP has been banned, IP: ::ffff:127.0.0.1, username: F2B

--- with server:port

(W) 2020-01-22T16:30:58 - WebAPI login failure. Reason: invalid credentials, attempt count: 5, IP: ::ffff:xxx.xxx.xxx.xxx, username: F2B
(W) 2020-01-22T16:31:00 - WebAPI login failure. Reason: IP has been banned, IP: ::ffff:xxx.xxx.xxx.xxx, username: F2B
@FranciscoPombal
Copy link
Member

Hmm, seems like qbittorrent could look at the X-Forwarded-For header and ban that instead (and the nginx config would need to be modified to send that header of course).

@glassez @Piccirello thoughts? Is there any security consideration I am missing?

@Kahana82
Copy link
Author

Good suggestion, I tried adding the X-Forwarded-For header (again I'm by no means an Nginx expert) :

proxy_set_header        X-Forwarded-For $proxy_add_x_forwarded_for;

Not working :
(N) 2020-01-23T10:09:48 - WebAPI login success. IP: ::ffff:127.0.0.1
Obviously the issue doesn't only occur for bans, but also for normal logins (facepalm)

Then tried this:

proxy_set_header        X-Real-IP $remote_addr;
proxy_set_header        X-Forwarded-For $proxy_add_x_forwarded_for;

Same result.

@FranciscoPombal
Copy link
Member

@Kahana82

Your nginx config is correct, the problem is that qbittorrent itself doesn't know about X-Forwarded-For (yet). That feature must be added, and the ban logic has to be changed/expanded to account for it.

@Kahana82
Copy link
Author

@FranciscoPombal

Thanks for checking ! The fact that nobody is getting those headers on the qbittorrent side explains my headache :)
I was kind of coming to the same conclusion after messing some more with it this afternoon :
The Nginx access logs are indeed correctly registering the remote ip address and would be passed on to qbittorrent through the headers.

Is that feature already on the dev's list of things to do, or should a suggestion/ticket be made somewhere to get it into their pipeline ?

@FranciscoPombal
Copy link
Member

Is that feature already on the dev's list of things to do, or should a suggestion/ticket be made somewhere to get it into their pipeline ?

Nope, this is it. I already tagged the main WebAPI contributors so we just have to wait. I have other things on my plate right now so unfortunately I cannot get this started now. If you want and have the ability to do so, you can get the PR started yourself.

@glassez
Copy link
Member

glassez commented Jan 24, 2020

Is #9176 related?

@Kahana82
Copy link
Author

Is #9176 related?

Yes, I would say the problem has the same roots.

As a test (from a remote IP) I enabled the "Bypass authentication for clients on localhost" option in the settings (WebUI/Web User Interface (Remote control)/Authentication).
This resulted in the WebUI showing up without having to login as the IP qbittorrent gets is that of the reverse proxy running on localhost...which is quite problematic also.

@Piccirello
Copy link
Member

I think we've rejected this idea in the past, under the belief that we shouldn't build reverse proxy-specific features. I no longer agree with this reasoning. We should support the header.

@Chocobo1
Copy link
Member

Chocobo1 commented Feb 12, 2020

I think the real problem is the unwanted banning of the reverse proxy IP and thus we should expose the ban counter and make it configurable from 1 to infinity instead of mingling with proxy-specific headers. If users want to ban clients connecting to their proxy, they should hook the ban functionality on the proxy (such as fail2ban reading the proxy log) and not on qbt.

@Piccirello
Copy link
Member

I'd agree with you if banning ips was a feature request. But IP banning is a long-standing feature, so supporting this additional use case seems reasonable. And it seems that enough users are impacted by this.

@Chocobo1
Copy link
Member

Chocobo1 commented Feb 12, 2020

I'd agree with you if banning ips was a feature request.

It is, looking from the first post, or you mean something else?

But IP banning is a long-standing feature, so supporting this additional use case seems reasonable.

I assume you already included proxy's client IPs in that statement. Well I suppose qbt needn't (and shouldn't) differentiated its clients whether it is a WebUI user, WebAPI client or a reverse proxy, unless it is something very necessary.

And it seems that enough users are impacted by this.

As said before, having a configurable ban counter in qbt would solve half of the problem.
AFAIK the reverse proxy can detect when qbt reply a authentication failure message and then it should be possible to setup a fail2ban script that detects it and then ban clients of the proxy.
Users are setting up nontrivial software (reverse proxy) and I would expect them to make good use of it (hook it up with fail2ban) instead of pushing more unnecessary complex features (reverse proxy specifics) to qbt.

@Chocobo1
Copy link
Member

Chocobo1 commented Feb 12, 2020

Anyway I will try to add that tuning knob (configurable ban counter) when I have some free time. At least it will solve half of OP problem.

@Piccirello
Copy link
Member

I'd agree with you if banning ips was a feature request.

It is, looking from the first post, or you mean something else?

Banning the client's IP is an existing feature. X-Forwarded-For is a defacto web standard for passing a client ip, according to MDN. Therefore our client ip calculation logic is faulty when used with a reverse proxy. It seems reverse proxies are a popular enough use case to warrant support for one additional header.

(There's a properly standardized Forwarded header as of 2014, but it doesn't seem to be widely adopted)

Anyway I will try to add that tuning knob (configurable ban counter) when I have some free time.

The tuning knob is a good feature. It only solves this PR's problem by accident though. I'm happy to implement the solution that I proposed.

Is there any security consideration I am missing?

@FranciscoPombal there is one: we should only trust the header when the user indicates they're using a reverse proxy. Otherwise the X-Forwarded-For Header can be set by the client.

@Chocobo1
Copy link
Member

Chocobo1 commented Feb 12, 2020

Therefore our client ip calculation logic is faulty when used with a reverse proxy.

Faulty or not, really depends on whether we differentiate the clients and currently qbt does not, which means it isn't deemed to be a problem.

Anyway I will try to add that tuning knob (configurable ban counter) when I have some free time.

The tuning knob is a good feature. It only solves this PR's problem by accident though.

I would say the hardcoded ban count is actually the limitation of OP is facing directly so adding the tuning knob would be the proper solution.
Adding support for X-Forwarded-For header is another feature and is questionable whether it is a necessity.

Is there any security consideration I am missing?

@FranciscoPombal there is one: we should only trust the header when the user indicates they're using a reverse proxy. Otherwise the X-Forwarded-For Header can be set by the client.

There is more than that, even if the user explicitly indicates they're using a reverse proxy, it is real easy for an attacker to trigger ban for any IP he wants by filling out any value to the X-Forwarded-For header and thus becoming an easy way for DDOS the WebUI. On the other hand, hooking fail2ban with proxy would identify/ban the correct offending IP and it won't suffer from this issue.

@Piccirello
Copy link
Member

There is more than that, even if the user explicitly indicates they're using a reverse proxy, it is real easy for an attacker to trigger ban for any IP he wants by filling out any value to the X-Forwarded-For header and thus becoming an easy way for DDOS the WebUI.

If the user is using a reverse proxy, it'll set the X-Forwarded-For header. This overwrites any client-provided value, allowing the header value to be trusted.

@Chocobo1
Copy link
Member

Chocobo1 commented Feb 12, 2020

If the user is using a reverse proxy, it'll set the X-Forwarded-For header. This overwrites any client-provided value, allowing the header value to be trusted.

What if the attacker can connect to WebUI directly? You can't assume users who setup reverse proxy to properly limit connectivity to WebUI, also users might want both reverse proxy and usual client to be able to connect to WebUI.

@iganeshk
Copy link

If the user is using a reverse proxy, it'll set the X-Forwarded-For header. This overwrites any client-provided value, allowing the header value to be trusted.

What if the attacker can connect to WebUI directly? You can't assume users who setup reverse proxy to properly limit connectivity to WebUI, also users might want both reverse proxy and usual client to be able to connect to WebUI.

If you also assume the fact that the mostly widely adopted way to install and run qbittorrent is via docker containers, I don't think attacker would be able to access the WebUI directly because Traefik/Nginx are primarily setup as reverse proxy.

@FranciscoPombal
Copy link
Member

@Chocobo1

What if the attacker can connect to WebUI directly? You can't assume users who setup reverse proxy to properly limit connectivity to WebUI,

Honestly, that is out-of-scope. The program should provide sane options, sane defaults for those options, and a sane mode of operation. We could even put a warning beside the option. The rest is the user's responsibility. It's also not qBittorrent's fault if the user misconfigures their wifi connection and then complains about downloads not working...

also users might want both reverse proxy and usual client to be able to connect to WebUI.

Seems unlikely. Once you have a reverse proxy correctly set up (which I assume most of the times is done for easier HTTPS setup), what is the purpose/advantage of connecting any other way?

@Piccirello

If the user is using a reverse proxy, it'll set the X-Forwarded-For header. This overwrites any client-provided value, allowing the header value to be trusted.

Is this done by default, at least in nginx for example? If not, we must warn the user in both the options and the wiki to set this header in their config.


Slightly off-topic:

For my use case, and a lot of other users, qBittorrent's native HTTPS functionality is a bit lacking, thus the need for an SSL-terminated reverse proxy config.
The reverse proxy setup is the only sane way of setting up qbittorrent over HTTPS with other homeserver stuff along side it.

The problem is that most users nowadays are using Let's Encrypt certs, and qBittorrent does not have the capability of automatically reloading the certs every x hours/days. This is relevant because LE certs expire in 90 days.

With an nginx reverse proxy config, there is no need to worry, since certbot and nginx auto-reload the certs on their own and there is no need to configure anything on qBittorrent's side.

@Piccirello
Copy link
Member

Is this done by default, at least in nginx for example? If not, we must warn the user in both the options and the wiki to set this header in their config.

Unfortunately it's not; we'll need to warn users about this. Maybe we even make reverse proxy support an Advanced option?

Wiki documentation w/ options for Apache and nginx won't be difficult- @Kahana82's already included a great nginx example. I'm happy to write the doc for it too.

For my use case, and a lot of other users, qBittorrent's native HTTPS functionality is a bit lacking, thus the need for an SSL-terminated reverse proxy config.

It's worth noting that I have this exact same use case. In my time using qBittorrent I've almost exclusively used qbittorrent-nox, and I've never used it without a reverse proxy. If I were completely unable to use a reverse proxy with qBittorrent, I'd use a different client.

@Piccirello
Copy link
Member

Apache 2.4+

  1. Enable the remoteip module
$ sudo a2enmod remoteip headers
$ sudo service apache2 restart
  1. Uncomment this line (or add it) in your apache2.conf
LoadModule remoteip_module modules/mod_remoteip.so
  1. Enable the header in your <VirtualHost>
RemoteIPHeader X-Forwarded-For

@FranciscoPombal FranciscoPombal added Confirmed bug An issue confirmed by project team to be considered as a bug WebAPI WebAPI-related issues/changes Core Help wanted labels Feb 18, 2020
@Rouzax
Copy link

Rouzax commented Apr 29, 2020

Curious, is this pull bb80b37 a solution for this?

@HiFiPhile
Copy link
Contributor

I think it's time to revisit #9176, as most of the features have been implemented in that patch.

@qix67
Copy link

qix67 commented Oct 23, 2020

I think it's time to revisit #9176, as most of the features have been implemented in that patch.

To whom it may concern, I just have updated master branch of the fork I used for #9176 . It is based on the patch I currently use on qBittorrent 4.2.5 and for months, it works perfectly behind nginx

@Walter-o
Copy link

Hey!

I am also doing a reverse proxy from qb.mysite.com pointing to the webui.

This luckily didn't open up the possibility of logging in as everyone since logins are cookie based instead of IP based.

But this does make it possible to perform a DOS to the qbittorrent web-ui when reverse proxies are used, even transparent ones that provide X-Forwarded-For etc headers.

qix67's patch looks good, i hope you guys can implement this change as it makes things a lot safer.

My reasons for wanting the change:

  • Being able to run the web-ui on a subdomain (eg: qb.mysite.com) to avoid port scanners and it looks cooler
  • Extra safety from apache/nginx by being able to reverse-proxy requests
  • Being able to place all SSL/HTTPS logic in the reverse proxy config so it's all in 1 place

@HiFiPhile
Copy link
Contributor

Hi

This feature is working on #15047 .

@ghost
Copy link

ghost commented May 5, 2022

Fixed by #15047

@ghost ghost closed this as completed May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Confirmed bug An issue confirmed by project team to be considered as a bug Core WebAPI WebAPI-related issues/changes
Projects
None yet
Development

No branches or pull requests

10 participants