Bug 420902

Summary: Site info falsely claims that connection is secure after certificate exception
Product: [Applications] Falkon Reporter: Florian Bruhin <kde.org>
Component: generalAssignee: David Rosca <nowrep>
Status: CONFIRMED ---    
Severity: normal CC: white.tw.tw
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In:

Description Florian Bruhin 2020-05-02 14:20:36 UTC
When opening https://expired.badssl.com/ and granting a certificate exception, the site info panel (when clicking the site's favicon) claims "Your connection to this site is *secured*" despite that not being the case.

When loading the site again in a new tab (or even just reloading it), QtWebEngine remembers the certificate exemption and doesn't ask again - those two things combined might provide users with a false sense of security that a connection is secure, despite that not being the case.

I noticed this while fixing a similar issue in qutebrowser:
https://github.com/qutebrowser/qutebrowser/issues/5403

While I consider this a security-relevant bug (and will request a CVE for qutebrowser), there's nothing to be exploited by a bad actor, hence I'm opening this publicly.

This is on Archlinux, with Qt 5.14.2 and Falkon 3.1.0.
Comment 1 Florian Bruhin 2020-05-02 16:37:54 UTC
I've looked at how other projects using QtWebEngine handle this case. The only approaches I have found are:

- Open a second connection via QSslSocket to check the certificate: 
  https://github.com/vicr123/theweb/commit/5f6cbc6093a1adb4fdf3db829b182139e065319b

- Save a set of insecure hosts in the certificateError signals, and assume 
  those are always insecure until a restart, from Viper Browser:
  https://github.com/LeFroid/Viper-Browser/blob/master/src/core/network/SecurityManager.cpp

I decided to go for the latter with qutebrowser - I already did set a flag in this situation so the UI was correct for the first load, but not for subsequent loads.

The preliminary qutebrowser fix (currently waiting for CI) is here:

https://github.com/qutebrowser/qutebrowser/commit/c7a0a150b2e991cc1c2fe8b883b074a800c2c40e
Comment 2 David Rosca 2020-05-03 09:35:19 UTC
Well, this is not really a 100% fix, because QtNetwork and QtWebEngine (Chromium) uses different SSL stacks, so you can still show false positives/negatives.

And as there is no way to query this information from QtWebEngine, there is no correct fix.
Comment 3 Florian Bruhin 2020-05-04 09:24:48 UTC
Agreed that there isn't a proper, clean fix for this. Still, remembering insecure hostnames and always treating them as insecure seems like an acceptable workaround: While still showing "insecure" when the problem is fixed server-side is unfortunate, it's still much better than showing "secure" when the connection isn't.

FWIW I added a comment here: https://bugreports.qt.io/browse/QTBUG-80860?focusedCommentId=510104&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-510104
Comment 4 TW3 2020-06-16 23:25:25 UTC
https://doc.qt.io/qt-5/qtwebengine-overview.html#managing-certificates
Appears to be relevant here.

Forgive me if I've missed something but isn't one way to solve this issue, to check the page's error enumeration https://doc.qt.io/qt-5/qwebenginecertificateerror.html#Error-enum and update the padlock icon (in the address bar) along with the site information pop ups to reflect the fact that the connection is insecure?
Comment 5 Florian Bruhin 2020-06-24 14:54:09 UTC
TW3: No, because there's no way to ask "does the current page have a certificate error" - there's only the QWebEnginePage::certificateError signal which doesn't get emitted anymore after an exception was granted.
Comment 6 TW3 2020-06-30 20:18:38 UTC
(In reply to Florian Bruhin from comment #5)
> TW3: No, because there's no way to ask "does the current page...
Exceptions are granted upon the domain not the page.

It's a simple choice between maintaining a temporary or permanent, per-domain list of exceptions once a user chooses to ignore the cert error.
The QWebEnginePage::certificateError signal could be used to double check the error type before the exception is created. It may simply be OK to put an exception list check behind a bool which becomes true the very first time the very first exception is granted or always if there is a permanent list.

Encouraging users to just grant cert exceptions for sites actually defeats the purpose of ssl and should be very heavily discouraged. Thus there should be no issues with incurring a small performance penalty for doing so IMO.

Using a valid cert for https is very important. Various reasons for doing so exist and Google explain it quite well here: https://support.google.com/webmasters/answer/6073543?hl=en

Another way to solve the issue mentioned here is to prevent all exceptions. I'm not advocating to do that but the issue should be dealt with, with absolutely minimal work. Spending time supporting http without encryption is fairy counter intuitive at this point in time.

Thanks.
Comment 7 Florian Bruhin 2020-06-30 20:27:02 UTC
> It's a simple choice between maintaining a temporary or permanent, per-domain list of exceptions once a user chooses to ignore the cert error.

Which is what I already proposed in comment 1 above. The problem with that is that it will still mark the connection as insecure even after the certificate has been fixed. But we're going in circles here, that's all already explained in the comments above.
Comment 8 TW3 2020-07-11 23:51:20 UTC
> But we're going in circles here, that's all already explained in the comments above.

Speak for yourself. I fixed this in my tree a week ago including updating the address bar icons and the site info widget.
And if you still think:

> TW3: No, because there's no way to ask "does the current page have a certificate error"

I urge you to please go and actually look at the source code which is causing this issue. Specifically NetworkManager::certificateError in networkmanager.cpp.

Thanks for your input though. It was interesting.