Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Icons to highlight cert errors in the address bar is missing #1963

Closed
jumde opened this issue Nov 14, 2019 · 12 comments
Closed

Icons to highlight cert errors in the address bar is missing #1963

jumde opened this issue Nov 14, 2019 · 12 comments

Comments

@jumde
Copy link
Contributor

jumde commented Nov 14, 2019

Description:

Address bar icons to highlight cert errors are missing in the address bar.

image

Steps to Reproduce

  1. Navigate to expired.badssl.com

Actual result:
No icon for expired certs

Expected result:
Icon should be displayed

Reproduces how often: [Easily reproduced, Intermittent Issue]
Easily

Brave Version:
v1.13 (19.11.06.18)

Device details:
iPhone SE

@jumde jumde added the security label Nov 14, 2019
@jumde
Copy link
Contributor Author

jumde commented Nov 14, 2019

@anthonypkeane
Copy link

@Brandon-T
Copy link
Collaborator

Brandon-T commented Nov 14, 2019

Need to also fix the evaluation of the "trust" to handle invalid hash of SHA1, Invalid Host, and CRL (Certificate Revocation).. otherwise it fails a bunch of tests on: https://badssl.com/

Currently, the code seems to ignore revocation and bad hosts..

Something like:

let x509 = SecPolicyCreateBasicX509()
let sslPolicy = SecPolicyCreateSSL(true, (webView.url?.host ?? "") as CFString)
SecTrustSetPolicies(trust, [x509, sslPolicy] as! CFTypeRef)
// SecTrustSetNetworkFetchAllowed(trust, true) //not sure if needed

then call evaluate.

@diracdeltas
Copy link
Member

Need to also fix the evaluation of the "trust" to handle invalid hash of SHA1, Invalid Host, and CRL (Certificate Revocation).. otherwise it fails a bunch of tests on: https://badssl.com/

@Brandon-T does this mean the cert error page doesn't show up at all in those cases? if so this is a much more serious issue than the icon not showing up; i would say it's probably a P1 or even a P0 if it means that generally we are not showing cert errors if the hostname on the cert doesn't match the domain hostname. Could you prioritize this as such and open a separate issue if needed?

@Brandon-T
Copy link
Collaborator

Brandon-T commented Nov 19, 2019

Need to also fix the evaluation of the "trust" to handle invalid hash of SHA1, Invalid Host, and CRL (Certificate Revocation).. otherwise it fails a bunch of tests on: https://badssl.com/

@Brandon-T does this mean the cert error page doesn't show up at all in those cases? if so this is a much more serious issue than the icon not showing up; i would say it's probably a P1 or even a P0 if it means that generally we are not showing cert errors if the hostname on the cert doesn't match the domain hostname. Could you prioritize this as such and open a separate issue if needed?

The icon won't show correctly.

Example: Visit website with wrong host, or SHA1 or revoked certificate, and it will show the website as secure even though it isn't (the page will be flagged but icon will be completely wrong and show as secure). Because the original code never evaluated the trust with specified policies :(

To fix that, we have to add the code I wrote above which creates an SSL policy to validate host and other cert fields and also create an x509 policy to validate the algorithms used. Now when we visit those same sites, it will show up as red.

I can upload a video showing the difference if we want. I also created a PR that addresses all of the above issues already.

Should I open a security review?

There's a few that the icon catches, but not the error page.

@jumde
Copy link
Contributor Author

jumde commented Nov 19, 2019

@Brandon-T - If I navigate to:

  1. https://wrong.host.badssl.com - I see cert error warning ( not the icon )
  2. https://sha1-intermediate.badssl.com - ^

Am I missing something?

revoked.badssl and pinning.badssl need more work - integrating CRLs

@diracdeltas
Copy link
Member

diracdeltas commented Nov 19, 2019

i'm seeing the same as @jumde - those sites with bad ssl states do show the interstitial page (which is the more important part), but not any kind of icon.

@diracdeltas
Copy link
Member

Should I open a security review?

that would be good for tracking, thanks. will assign @jumde to it

@Brandon-T
Copy link
Collaborator

Brandon-T commented Nov 20, 2019

The icon showing was part of my fix. The colours of the icon is part of it as well. But what I meant was that for some pages, the icon shows red but the error page doesn't show.
Note: Safari also does not show an error or warning for the same pages as Brave and Brave's icon.

ezgif-2-ca4376dde2f3

@anthonypkeane
Copy link

@jumde can this be incorporated into the Origin-only URL bar spec?

@jumde
Copy link
Contributor Author

jumde commented May 6, 2020

Test Plan

  • Navigate to badssl.com
  • Verify that green/red/grey icon is highlighted for appropriated domains.

@srirambv
Copy link
Contributor

srirambv commented May 6, 2020

Verification passed on iPhone XR with iOS 13.5 running 1.16(20.05.05.20)

  • Verified red icon used for expired cert
  • Verified grey icon used for valid cert

Verification passed on iPhone 7+ with iOS 13.3.1 running 1.16(20.05.05.20)

  • Verified red icon used for expired cert
  • Verified grey icon used for valid cert

Verification passed on iPhone 6 with iOS 12.4.5 running 1.16(20.05.05.20)

  • Verified red icon used for expired cert
  • Verified grey icon used for valid cert

Verification passed on iPad Pro with iOS 13.3.1 running 1.16(20.05.05.20)

  • Verified red icon used for expired cert
  • Verified grey icon used for valid cert

Verification passed on iPad Pro with iOS 12.4.5 running 1.16(20.05.05.20)

  • Verified red icon used for expired cert
  • Verified grey icon used for valid cert

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.