Deny remote user login with default credentials #18735

sledgehammer999 · 2023-03-20T15:32:59Z

Apparently there are users exposing the webui client to the internet without changing the default credentials. And apparently there are attackers out there scanning for exposed clients and then logging in with the default credentials and running code (crypto miners).

With this PR users will be able to connect with default credentials only from localhost.
If connecting remotely, they login page will show them the reason the login failed and ask them to login locally to change the credentials (or manually set them in the configuration file).

Closes #13833
Closes #16529
Closes #18731

rmartin16 · 2023-03-20T16:13:30Z

I support this change. However (and this may be unavoidable), this change blocks local connections for me when qBittorrent is running in a Docker container. I can work around by exposing the host network to the Docker container....and that (ofc) comes with its own implications. FYI if nothing else since I exclusively use qBittorrent in Docker for personal and development purposes; I imagine this also may impact existing qBittorrent images and installation guides.

glassez · 2023-03-20T18:14:44Z

or manually set them in the configuration file

It's not a problem to change the username this way, but it's not that easy for a password.
And I think access only from the localhost may be too strict a requirement, because it seems impossible or quite difficult for things like Docker, which is often used to run qbittorrent-nox.

sledgehammer999 · 2023-03-20T22:32:36Z

Can users pass cmd arguments when starting the docker container? (I am a docker noob).
I have introduced 2 cmd parameters that allow the user to change the webui credentials.

src/app/cmdoptions.cpp

src/app/cmdoptions.h

src/app/main.cpp

src/webui/api/authcontroller.cpp

glassez · 2023-03-21T08:50:32Z

src/webui/api/authcontroller.cpp

+        LogMsg(tr("WebAPI login failure. Reason: Remote connection with the default credentials is prohibited.")
+               , Log::WARNING);
+        throw APIError(APIErrorType::AccessDenied
+                       , tr("Remote connection with the default credentials is prohibited. Change the default credentials by connecting from the local network, or by using the %1 and/or %2 command line parameters.",
+                          "Remote connection with the default credentials is prohibited. Change the default credentials by connecting from the local network, or by using the '--change-webui-username' and/or '--change-webui-password' command line parameters.")
+                           .arg(u"'--change-webui-username'"_qs, u"'--change-webui-password'"_qs));


Suggested change

LogMsg(tr("WebAPI login failure. Reason: Remote connection with the default credentials is prohibited.")

, Log::WARNING);

throw APIError(APIErrorType::AccessDenied

, tr("Remote connection with the default credentials is prohibited. Change the default credentials by connecting from the local network, or by using the %1 and/or %2 command line parameters.",

"Remote connection with the default credentials is prohibited. Change the default credentials by connecting from the local network, or by using the '--change-webui-username' and/or '--change-webui-password' command line parameters.")

.arg(u"'--change-webui-username'"_qs, u"'--change-webui-password'"_qs));

LogMsg(tr("WebAPI login failure. Reason: Remote connection with the default credentials is prohibited.")

, Log::WARNING);

throw APIError(APIErrorType::AccessDenied

, tr("Remote connection with the default credentials is prohibited. Change the default credentials by connecting from the local network, or by using the %1 and/or %2 command line parameters."

, "Remote connection with the default credentials is prohibited. Change the default credentials by connecting from the local network, or by using the '--change-webui-username' and/or '--change-webui-password' command line parameters.")

.arg(u"'--webui-username'"_qs, u"'--webui-password'"_qs));

sledgehammer999 · 2023-03-21T16:23:04Z

We don't exit app when Web UI port is changed via command line. Is there any strong reason to behave differently in this case?
If not, then I would also move username/password processing to Application class next to the WebUI port processing.

My rationale for the above and for the naming scheme

It depends on the semantics you want to give. These cmd switches make persistent edits to the config file. They aren't meant to be seen as temporary overrides of the config.
So I want to consider them as another mode of operation. Aka edit config vs normal mode.
I don't feel strongly about that. If you insist I'll make the necessary changes.

glassez · 2023-03-21T17:18:07Z

I want to consider them as another mode of operation. Aka edit config vs normal mode.

This is a good idea in itself, IMO (something similar has occurred to me). But it contradicts the current semantics, so I object to it.
Such different "modes of operation" could be introduced in one of major update.

sledgehammer999 · 2023-03-21T18:47:49Z

ΟΚ. I'll push the requested changes later.

Chocobo1 · 2023-03-21T19:50:46Z

We don't exit app when Web UI port is changed via command line. Is there any strong reason to behave differently in this case?
If not, then I would also move username/password processing to Application class next to the WebUI port processing.

My rationale for the above and for the naming scheme

It depends on the semantics you want to give. These cmd switches make persistent edits to the config file. They aren't meant to be seen as temporary overrides of the config. So I want to consider them as another mode of operation. Aka edit config vs normal mode. I don't feel strongly about that. If you insist I'll make the necessary changes.

Other than the 'semantics', passing sensitive information via commandline parameters (especially to a long-time running program) is a very bad idea. For example, in a multi-user machine, someone (one with enough privileges and not necessarily the root) could easily see all your commandline parameters in clear text. And also the sensitive information is in clear text in memory (in argv list) for a long time which is also a bad practice.

IMO using edit config mode changing the user/password make sense and is a lot better than normal mode. I.e. throw away the sensitive data the sooner the better.

sledgehammer999 · 2023-03-21T23:15:36Z

For example, in a multi-user machine, someone (one with enough privileges and not necessarily the root) could easily see all your commandline parameters in clear text. And also the sensitive information is in clear text in memory (in argv list) for a long time which is also a bad practice.

In that threat model, such privileged users already own you because they can easily either edit your config file or monitor your traffic. Am I wrong?

However, your comment reminded me of another possible leak: bash history files. They record each command you give via the terminal. This would require a totally different approach for configuring the password. eg Having qbt to wait for user input in the terminal. Personally I cannot code this in a reasonable timeframe.

sledgehammer999 · 2023-03-21T23:58:15Z

in a multi-user machine, someone (one with enough privileges and not necessarily the root) could easily see all your commandline parameters in clear text.

After doing a bit of research, it seems you don't need any privileges to achieve this. So it is valid threat concern.

glassez · 2023-03-22T05:14:47Z

in a multi-user machine, someone (one with enough privileges and not necessarily the root) could easily see all your commandline parameters in clear text.

After doing a bit of research, it seems you don't need any privileges to achieve this. So it is valid threat concern.

So I think you shouldn't provide it in this PR.

sledgehammer999 · 2023-03-22T07:04:54Z

So I think you shouldn't provide it in this PR.

What do you mean? Should I drop the cmd arguments commit?

glassez · 2023-03-22T09:41:36Z

So I think you shouldn't provide it in this PR.

What do you mean? Should I drop the cmd arguments commit?

Yes. Doesn't it look like you're covering up one vulnerability with another? Of course, CLI is intended not only to be used from a shell like bash, which will save passed parameters in its history file, and an informed user will not use it in this way. But an informed user will also not open access from the outside without changing the default password...

glassez · 2023-03-22T11:19:35Z

Maybe instead add command line parameter to set "trusted host" from which alongside with localhost) a login using default credentials is accepted?

Apparently there are users exposing the webui client to the internet without changing the default credentials. And apparently there are attackers out there scanning for exposed clients and then logging in with the default credentials and running code (crypto miners). Closes qbittorrent#13833 Closes qbittorrent#16529 Closes qbittorrent#18731

Chocobo1 · 2023-10-20T07:07:15Z

I had an alternative idea:
Just generate a 6 (or 10) digit number random password at the very first launch of qbt.
In gui version, webui is by default off and when user turns it on (selected the webui checkbox), we will force the user to enter a new password via some UI widget property. I suspect we don't even need to show the random password.
In nox version. We will print the password to console and let user read it themselves.

glassez · 2023-10-20T07:25:46Z

I had an alternative idea

This is not an alternative idea. It basically coincides with my intention.

In gui version, webui is by default off and when user turns it on (selected the webui checkbox), we will force the user to enter a new password

👍

I suspect we don't even need to show the random password.

👍
Random password is meaningless in GUI mode. Just don't allow to enable WebUI w/o custom password is set.

In nox version. We will print the password to console and let user read it themselves.

👍
Like we do currently for default credentials.

sledgehammer999 · 2023-10-20T07:29:07Z

In nox version. We will print the password to console and let user read it themselves.

Does this have any security implications that we should consider?

glassez · 2023-10-20T08:03:51Z

In nox version. We will print the password to console and let user read it themselves.

Does this have any security implications that we should consider?

We won't be able to do anything if someone else (or malware) has access to the user's computer to read generated credentials. This is beyond our responsibility.

Chocobo1 · 2023-10-20T10:48:27Z

In nox version. We will print the password to console and let user read it themselves.

Does this have any security implications that we should consider?

The password is limited to the specific user and is local to the machine so I think it is pretty safe.

sledgehammer999 · 2023-10-20T11:29:33Z

We won't be able to do anything if someone else (or malware) has access to the user's computer to read generated credentials. This is beyond our responsibility.

Generally I agree but see below.

The password is limited to the specific user and is local to the machine so I think it is pretty safe.

This will downgrade the current security offered to the user. Now we don't store the password itself but a salted hash of it.
Printing to console requires having access to the plaintext password.

glassez · 2023-10-20T11:43:13Z

Now we don't store the password itself but a salted hash of it.
Printing to console requires having access to the plaintext password.

It is supposed to be printed once generated, i.e. once qBittorrent is started w/o stored password. Then it is stored as usual and never be accessed as plain text. If the user did not remember it at the first launch, then he will have to reset it in the usual way, as it was previously available, i.e. clearing the saved password hash.

Chocobo1 · 2023-10-20T11:52:26Z

It is supposed to be printed once generated, i.e. once qBittorrent is started w/o stored password. Then it is stored as usual and never be accessed as plain text.

Perhaps the password can be generated every time if the user haven't updated it.

sledgehammer999 · 2023-10-20T12:37:27Z

It is supposed to be printed once generated, i.e. once qBittorrent is started w/o stored password. Then it is stored as usual and never be accessed as plain text. If the user did not remember it at the first launch, then he will have to reset it in the usual way, as it was previously available, i.e. clearing the saved password hash.

It is probably easy to miss it in the first launch. And hard to debug afterwards (lots of "hey what's the default password" questions).

Perhaps the password can be generated every time if the user haven't updated it.

Sounds good. However, qBt will need to track this.

I am not rejecting the above but where do we stand on the following? Provide a cmd arg for the nox version for configurin credentials (eg --configure-web-credentials). It will enter interactive mode asking the user to input username and password. Then write them to settings and exit.

glassez · 2023-10-20T12:48:03Z

Perhaps the password can be generated every time if the user haven't updated it.

Sounds good. However, qBt will need to track this.

It's not a problem, IMO. Just don't store generated password so it is always empty at start unless user set custom one and it is stored.

Chocobo1 · 2023-10-20T13:09:21Z

Sounds good. However, qBt will need to track this.

There should be a field (maybe WebUIPasswordUpdated) that records whether user has updated the password.
If the field is non-existent/empty/false (or the webui hash is still the default adminadmin) then a random password is generated at launch and updates the webui password/hash and prints clear text password to screen. Otherwise do nothing and start qbt normally.
Once the user updates the password, this field is marked true (or something else, I haven't consider it thoroughly).

It's not a problem, IMO. Just don't store generated password so it is always empty at start unless user set custom one and it is stored.

But won't you need more code to handle it at webui authentication? I suppose update the webui hash would be easier.

Provide a cmd arg for the nox version for configurin credentials (eg --configure-web-credentials). It will enter interactive mode asking the user to input username and password. Then write them to settings and exit.

I don't consider it necessary but I don't object it either. IMO it can be done later (not a high priority) if anyone is interested.

glassez · 2023-10-20T15:55:54Z

There should be a field (maybe WebUIPasswordUpdated) that records whether user has updated the password.

I believe it is better to avoid adding such an additional/temporary entry to the config file, if we can do with a variable.

glassez · 2023-10-20T16:38:49Z

The main thing now is to decide about the basic concept, and the implementation details are just details.

sledgehammer999 · 2023-10-20T19:11:31Z

Let me summarize. I think there is consensus for the following:

Fresh install equals empty credentials. Old default credentials are treated as empty too.
GUI: On empty webui credentials the GUI will enforce the configuration of credentials before accepting the configuration setting "Enable WebUI"
NOX: On empty webui credentials, it will create ephemeral(per session) credentials that will be output on the console/log.

That being said, I think @glassez's proposition of tracking the "empty" state via a variable is doable.

xavier2k6 · 2023-10-20T20:53:11Z

@sledgehammer999 please take #18735 (comment) in to account too.

Currently, the default password of adminadmin = 10 characters, yet we allow to lower that default by only requiring 6

glassez · 2023-10-21T04:13:50Z

Old default credentials are treated as empty too.

I suppose there may be problems with this in some cases.
There is no problem if WebUI is disabled.
We need to think about how to behave if WebUI is enabled, but uses legacy default credentials.

Also we need to reject "adminadmin" from being set as password.

Chocobo1 · 2023-10-21T05:11:42Z

Old default credentials are treated as empty too.
We need to think about how to behave if WebUI is enabled, but uses legacy default credentials.

Why not detect if the password hash is still equal to adminadmin (the previous default hash value) and require a new password if it is.

Also we need to reject "adminadmin" from being set as password.

I don't think we need to always reject it since the new default password is randomly generated. We only need to reject it when the user hasn't change it when upgrading from an older qbt version.

glassez · 2023-10-21T05:54:42Z

Why not detect if the password hash is still equal to adminadmin (the previous default hash value) and require a new password if it is.

I'm not worried about how to detect this (it's not a problem), but about how to "require a new password" in a non-confusing way. In GUI, we could just show a dialog stating that the existing credentials are no longer valid. But what about no-GUI case? To a greater extent, I worry about systems running in non-interactive mode.

glassez · 2023-10-21T06:26:35Z

I don't think we need to always reject it since the new default password is randomly generated.

What does a randomly generated password have to do with it?
I mean that "adminadmin" is well-known default password that is most likely to be affected by attakers. So I suggest to forbid it at all (i.e. disallow user to set such a password like any other invalid one).

glassez · 2023-10-21T06:35:03Z

But what about no-GUI case? To a greater extent, I worry about systems running in non-interactive mode.

Of course we will have to post a warning on the site that THE DEFAULT CREDENTIALS ARE NO LONGER VALID. Given the severity of the solved problem, the possible transitional difficulties of users can be considered as negligible.

Chocobo1 · 2023-10-21T06:58:52Z

I worry about systems running in non-interactive mode.

I presume users would have to check the logs. In fact they do it often when qbt service is not up. AFAIK some service manager will redirect stderr/stdout to their own log.

I mean that "adminadmin" is well-known default password that is most likely to be affected by attakers. So I suggest to forbid it at all (i.e. disallow user to set such a password like any other invalid one).

I don't insist but I don't favor disallowing "adminadmin" as a password if some dumb user really wants it to be (or they could have their reasons). I don't like being a nanny for users.
Also note that another reason I dislike complicating setting/getting passwords is because potential timing attacks that might creep in...

glassez · 2023-10-21T07:20:58Z

I don't favor disallowing "adminadmin" as a password if some dumb user really wants it to be (or they could have their reasons). I don't like being a nanny for users.
Also note that another reason I dislike complicating setting/getting passwords is because potential timing attacks that might creep in...

Reasonable enough, IMO.

sledgehammer999 · 2023-10-21T10:13:41Z

I am in favor of disabling "adminadmin" for existing users. This is a security fix and a breakage of sorts is warranted. Also it is most likely a mistake for the user to have webui enabled and default credentials.
Otherwise we will complicate things on our side unnecessarily.

On the WebUI front we can leverage the response to /api/v2/auth/login. Now it responds with 200 and a string reading Fails when credentials are wrong. We could return a detailed string about the ephemeral credentials and make the WebUI display that.

sledgehammer999 · 2023-10-21T10:15:31Z

Of course we will have to post a warning on the site that THE DEFAULT CREDENTIALS ARE NO LONGER VALID. Given the severity of the solved problem, the possible transitional difficulties of users can be considered as negligible.

In fact, I'll put a similar warning to the news page now that I'll make the 4.6.0 release. "DONT USE DEFAULT CREDENTIALS FOR THE WEBUI. THEY WILL BE DISABLED IN A FOLLOWING RELEASE".

ownrepo1s

Seems appropriate

staze · 2023-10-24T16:13:44Z

Upon login the server will check if default credentials are used.

If yes, it will allow API access only to the "change credentials" API(plus logout/login). Every other API request will be denied.

Our WebUI will recognize this and redirect to a page for changing the credentials.

Step 2 will happen regardless of the network from which the connection comes from.

(optional) Maybe consider full access on default credentials if it comes from localhost.

I think this idea has been altered, but wanted to say, I think the issue with this is users may not know what enabling web gui does, so they may enable and never go change it. allowing change credentials via the API, or web interface, when the default password is still set would let an attacker change the password using the default. =/

glassez · 2023-10-26T05:27:48Z

Superseded by #19777.

sledgehammer999 added Security Related to software vulnerability in qbt (don't overuse this) WebUI WebUI-related issues/changes WebAPI WebAPI-related issues/changes labels Mar 20, 2023

sledgehammer999 added this to the 4.5.3 milestone Mar 20, 2023

sledgehammer999 requested review from glassez and Chocobo1 March 20, 2023 15:33

sledgehammer999 force-pushed the deny_remote_default branch from 089c2a6 to dbd9eb2 Compare March 20, 2023 15:39

sledgehammer999 force-pushed the deny_remote_default branch from 3cb12c9 to 710f8f1 Compare March 20, 2023 22:33

sledgehammer999 removed the WebAPI WebAPI-related issues/changes label Mar 20, 2023

sledgehammer999 force-pushed the deny_remote_default branch 3 times, most recently from fe19e85 to 9dd5456 Compare March 20, 2023 23:06

glassez reviewed Mar 21, 2023

View reviewed changes

src/app/cmdoptions.cpp Outdated Show resolved Hide resolved

glassez reviewed Mar 21, 2023

View reviewed changes

xtpkt previously approved these changes Mar 21, 2023

View reviewed changes

sledgehammer999 added 2 commits March 22, 2023 19:44

WebUI: Minor optimizations to the login page

55122f0

ownrepo1s approved these changes Oct 23, 2023

View reviewed changes

glassez closed this Oct 26, 2023

glassez mentioned this pull request Oct 26, 2023

Drop WebUI default credentials #19777

Merged

sledgehammer999 deleted the deny_remote_default branch November 14, 2023 01:09

Deny remote user login with default credentials #18735

Deny remote user login with default credentials #18735

Conversation

sledgehammer999 commented Mar 20, 2023 • edited Loading

rmartin16 commented Mar 20, 2023 • edited Loading

glassez commented Mar 20, 2023

sledgehammer999 commented Mar 20, 2023 • edited Loading

glassez Mar 21, 2023

Choose a reason for hiding this comment

sledgehammer999 commented Mar 21, 2023

My rationale for the above and for the naming scheme

glassez commented Mar 21, 2023

sledgehammer999 commented Mar 21, 2023

Chocobo1 commented Mar 21, 2023 • edited Loading

My rationale for the above and for the naming scheme

sledgehammer999 commented Mar 21, 2023

sledgehammer999 commented Mar 21, 2023 • edited Loading

glassez commented Mar 22, 2023

sledgehammer999 commented Mar 22, 2023 • edited Loading

glassez commented Mar 22, 2023

glassez commented Mar 22, 2023

Chocobo1 commented Oct 20, 2023 • edited Loading

glassez commented Oct 20, 2023 • edited Loading

sledgehammer999 commented Oct 20, 2023 • edited Loading

glassez commented Oct 20, 2023

Chocobo1 commented Oct 20, 2023

sledgehammer999 commented Oct 20, 2023

glassez commented Oct 20, 2023

Chocobo1 commented Oct 20, 2023

sledgehammer999 commented Oct 20, 2023

glassez commented Oct 20, 2023

Chocobo1 commented Oct 20, 2023 • edited Loading

glassez commented Oct 20, 2023

glassez commented Oct 20, 2023

sledgehammer999 commented Oct 20, 2023

xavier2k6 commented Oct 20, 2023

glassez commented Oct 21, 2023

Chocobo1 commented Oct 21, 2023 • edited Loading

glassez commented Oct 21, 2023

glassez commented Oct 21, 2023

glassez commented Oct 21, 2023

Chocobo1 commented Oct 21, 2023 • edited Loading

glassez commented Oct 21, 2023

sledgehammer999 commented Oct 21, 2023

sledgehammer999 commented Oct 21, 2023

ownrepo1s left a comment

Choose a reason for hiding this comment

staze commented Oct 24, 2023

glassez commented Oct 26, 2023

sledgehammer999 commented Mar 20, 2023 •

edited

Loading

rmartin16 commented Mar 20, 2023 •

edited

Loading

sledgehammer999 commented Mar 20, 2023 •

edited

Loading

Chocobo1 commented Mar 21, 2023 •

edited

Loading

sledgehammer999 commented Mar 21, 2023 •

edited

Loading

sledgehammer999 commented Mar 22, 2023 •

edited

Loading

Chocobo1 commented Oct 20, 2023 •

edited

Loading

glassez commented Oct 20, 2023 •

edited

Loading

sledgehammer999 commented Oct 20, 2023 •

edited

Loading

Chocobo1 commented Oct 20, 2023 •

edited

Loading

Chocobo1 commented Oct 21, 2023 •

edited

Loading

Chocobo1 commented Oct 21, 2023 •

edited

Loading