Don't ignore SSL errors

[sledgehammer999]

Ignoring SSL errors was introduced ~14 years ago with commit 9824d86

I presume that it was a quick'n'dirty way to get SSL going which persisted to this day. It's also possible that back in the day Qt4 (?) didn't support autoloading ca root certificates from the OS's store.

IMO, there isn't a valid reason to unconditionally ignore SSL errors.
Also I am a bit hesitant in providing users an escape hatch for disabling SSL verification.

For my use cases, the log shows only two errors regarding favicon retrieval from a tracker with an expired cert and one with a cert that doesn't match the hostname.

I don't use RSS or search. So I wanna hear from the other testers what they find on their machine running this.

[Chocobo1]

Also I am a bit hesitant in providing users an escape hatch for disabling SSL verification.

For my use cases, the log shows only two errors regarding favicon retrieval from a tracker with an expired cert and one with a cert that doesn't match the hostname.

AFAIK this code path is also used by downloading .torrent files (for example https://archlinux.org/releng/releases/2024.09.01/torrent/). So it affects more than just search or rss.

ps. qbt already exposes a libtorrent setting in Advanced options: https://www.libtorrent.org/reference-Settings.html#validate_https_trackers
I guess user will request qbt provide a counterpart option for this one sooner or later.

[sledgehammer999]

Without thinking I started using the term TLS instead of SSL. I probably should revert to SSL to keep it in line with the error message.
I'll do it after I gather reviews for the rest of the code.

[Chocobo1]

Although not strictly required, it would also be nice to add the option to webui. You'll only need to add a few code to appcontroller.cpp and preferences.html.

[xavier2k6]

I don't use RSS or search.

It would be advisable to test these options, especially on macOS as there have been issues with search-plugins & showing no results e.g. qbittorrent/search-plugins#278

(it may or may not be related!)

For my use cases, the log shows only two errors regarding favicon retrieval from a tracker with an expired cert and one with a cert that doesn't match the hostname.

Experienced same.

[glassez]

What does the log look like in case of SSL error? Does it contain two messages (this and the usual message about failed request)?

[sledgehammer999]

Keep in mind that I merely kept the original formatting and only deleted the "Ignoring" word.
One example from my machine:

27/9/2024 7:18 μμ - SSL error, URL: "https://xxxxxxx.com/favicon.ico", errors: "The host name did not match any of the valid hosts for this certificate"

[glassez]

Keep in mind that I merely kept the original formatting and only deleted the "Ignoring" word. One example from my machine:

27/9/2024 7:18 μμ - SSL error, URL: "https://xxxxxxx.com/favicon.ico", errors: "The host name did not match any of the valid hosts for this certificate"

You probably didn't understand what I was talking about. I'm not talking about formatting this message. I'm asking if there is also a regular error message due to a failed request? (If you don't ignore the SSL error, it leads to a failed request, right?)

[sledgehammer999]

I'm asking if there is also a regular error message due to a failed request? (If you don't ignore the SSL error, it leads to a failed request, right?)

For the favico failure I don't get any additional error logged.
Should I investigate all users of downloadmanager?

[glassez]

For the favico failure I don't get any additional error logged.

OK, I've sorted it out.
Download manager does not produce a log message for each failed request, but leaves it to a specific caller. Therefore, I would not do it for case of SSL error either.
Note that the case when SSL error is ignored is different in that the request is not considered as failed, but the user is still notified of the ignored error.

[glassez]

I'd like to test it myself. How can I get SSL error in a reliable way?

[jsharp6968]

If you want reliable SSL errors you can generate them every time by setting up a proxy on a second machine in your network, routing all traffic from the machine running qBittorrent through that proxy machine before it hits the internet. Then, run certmitm on the proxy machine. This tool replaces the certificates with invalid ones and will show you if the client still accepts it - it's made to test exactly this behaviour.

This is how I originally found this issue and reported via email. You will get a request to download the GeoIP db at startup, and then an update check through fosshub.com after that.

[glassez]

you can generate them every time by setting up a proxy on a second machine in your network, routing all traffic from the machine running qBittorrent through that proxy machine before it hits the internet.

I would prefer some simpler way, for example, to add a tracker or RSS subscription that has a deliberately invalid certificate.

[glassez]

@sledgehammer999
#21364 (comment) is not addressed yet.

[sledgehammer999]

So basically when ignore_ssl_error == false -> the caller and not the download manager should decide if it wants to log a message for the SSL error.
Is that what you propose?

[glassez]

So basically when ignore_ssl_error == false -> the caller and not the download manager should decide if it wants to log a message for the SSL error. Is that what you propose?

Yes. In case of ignore_ssl_error == false, you don't have to do anything at all. SSL error will be handled the same way as any other error, i.e. it will just be promoted to caller as a result of the request.

[sledgehammer999]

If we don't log the SSL error (at download manager level), then there is a chance that the caller will fail silently without logging an error. This may hinder debugging.
Are we ok with that? (honest question)

[glassez]

This may hinder debugging.

What kind of debugging do you have in mind?

If we don't log the SSL error (at download manager level), then there is a chance that the caller will fail silently without logging an error

Yes. This is exactly how it is supposed to be. If some caller is not interested in reporting errors, then it does not do it, and it does not matter what kind of error this is.

[sledgehammer999]

What kind of debugging do you have in mind?

"Hey, my RSS feed suddenly stopped updating with the new version and I don't know why! Reverting to the previous version it works!"

Arguably it is heavy handed to handle this at the downloadmanager level. The SSL errors are a special category of failure and I don't know if we should leave it to the callers to log them.
I am thinking out loud. I don't have strong feelings towards it. I can implement your proposal tomorrow probably.

[glassez]

What kind of debugging do you have in mind?

"Hey, my RSS feed suddenly stopped updating with the new version and I don't know why! Reverting to the previous version it works!"

I believe downloading failures are logged in every important cases. If some are not, then most likely they should be logged regardless of the type of error.

The SSL errors are a special category of failure and I don't know if we should leave it to the callers to log them.

In the cases where we deliberately ignore downloading errors (for example, when downloading favicons), we should hardly force log SSL errors, IMO.

I am thinking out loud. I don't have strong feelings towards it. I can implement your proposal tomorrow probably.

Let's get @Chocobo1 opinion.

[glassez]

ps. qbt already exposes a libtorrent setting in Advanced options: https://www.libtorrent.org/reference-Settings.html#validate_https_trackers
I guess user will request qbt provide a counterpart option for this one sooner or later.

Shouldn't this be a single (global) option?

[Chocobo1]

I am thinking out loud. I don't have strong feelings towards it. I can implement your proposal tomorrow probably.

Let's get @Chocobo1 opinion.

I think the suggestion in #21364 (comment) is reasonable.

ps. qbt already exposes a libtorrent setting in Advanced options: https://www.libtorrent.org/reference-Settings.html#validate_https_trackers
I guess user will request qbt provide a counterpart option for this one sooner or later.

Shouldn't this be a single (global) option?

I don't have strong opinion but having two independent options seems better for the user. They can turn off the one that affects them and keep the other on.

[jsharp6968]

SSL verification errors are the only kind of network error I can think of which sometimes mean a user is being attacked. Most times it's just a cert that expired and wasn't renewed on time. On occasion the wrong cert will be put in the wrong place.

But, sometimes, users will be under Man In The Middle attack and they should know about that. I think SSL errors should always be logged for this reason.

If logging is not done in DownloadManager, then it should definitely be done in ProgramUpdater, because currently no logging is done there and that is the most dangerous part of the application in light of this issue. SSL errors currently mean a MITM attacker can give users an arbitrary URL in the RSS data to download the update package, which users will then visit if they accept the update prompt. If I were a government trying to send a malicious binary to a target, that is what I would use.

Giving only one option to ignore all or no SSL errors is dangerous because the moment a user's favourite RSS feed throws an error, they will remove protection from the update mechanism.

Favicons seem like a less critical attack surface, until you consider how many critical RCE exploits have come from image parsing libraries in recent years. XML parsers have also had their fair share of vulnerabilities. SSL exists as much for security as it does for privacy, and it is qualitatively more important than a network resource being unavailable for whatever reason. If you have a reason to use SSL, you have a reason to care if verification fails. Just my $0.02 as a security professional.

[sledgehammer999]

The SSL errors are a special category of failure and I don't know if we should leave it to the callers to log them.

I think @jsharp6968 expressed better my thoughts.
For what it's worth even now, on every SSL failure there is a log message (of "ignoring SSL errror: blah blah") regardless if it was "innocuous" (Favicon) or serious (update url). Users haven't complained about that (presumably the errors were very limited).
So it makes more sense to continue logging centrally (from downloadmanager) every non-ignored ssl error too.

[glassez]

So it makes more sense to continue logging centrally (from downloadmanager) every non-ignored ssl error too.

Okay, so be it.

[sledgehammer999]

Still someone has to give an approval to the PR...

[sledgehammer999]

I'll open backport PRs (or add to existing ones) in the next ~12 hours.

[proton-ab]

Can we get this backported to v4.6.x branch given its security nature? Due to v5.0 requirements for Qt, new versions are not available in Ubuntu 24.04 (#21608 (comment)). I believe because of this, it warrants having security release for older branch. Leaving 24.04 out of support feels kinda cheap consider this is current LTS and released just few months ago.

[alienbob]

I re-based the patch for 5.0.1 into a patch for 4.6.7 and was able to build a working package for Slackware which I am going to upload soon.
Find the patch attached, I hope I missed nothing.
qbittorrent-4.6.7_ssl.PATCH

[sledgehammer999]

This is backported now.

[Neustradamus]

It is possible to create a backport fix for qBittorrent which work on previous Windows system -> 4.4.x branch?
Several people always use it.

[stalkerok]

And for 4.5.x, and 4.3.x, etc.
@sledgehammer999 you should have explained to people that this is not a serious security issue...