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

Fix #1963, Fix #1964 - URL bar alignment is off and secure page icon not showing #1971

Merged
merged 10 commits into from
Apr 15, 2020

Conversation

Brandon-T
Copy link
Collaborator

@Brandon-T Brandon-T commented Nov 15, 2019

Security Review:

Summary of Changes

  • Fixes URL bar alignment due to security bug Handling truncated domains #552
  • Fixes URL bar secure webpage icon not showing 100% of the time
  • Like Android, added green and red colours to the icon.
  • Fixes 4 security flaws in the validation of the SecTrust for WKWebView (calling SecTrustEvaluate was not enough.. I have added a list of policies to validate.. can be tested with https://badssl.com)
    • Now validates expired certificates
    • Now validates wrong host
    • Now validates CRL (Certificate Revocation)
    • Now validates x509 (SHA1, Subject, KeyLength, etc)..
      • In iOS 13, any certificate with SHA1 or weak algorithms will fail.
      • Any non TLS1.2 cert will fail.
      • Any certificate with invalid SubjectInfo or any invalid fields will fail.

Prior to these "small" changes, all tests against badssl.com would fail and show the page was secure when it wasn't.

WIP:

  • Still waiting on the real icons for iOS to be uploaded to the ticket.
  • Unit tests for security.

This pull request fixes issue #1964 & #1963

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).

@Brandon-T Brandon-T changed the title Feature/security fixes Fix #1963, Fix #1964 - URL bar alignment is off and secure page icon not showing Nov 15, 2019
@iccub
Copy link
Contributor

iccub commented Nov 19, 2019

What is verdict on this? I mean how this PR works with etld+1 stuff?

@Brandon-T
Copy link
Collaborator Author

What is verdict on this? I mean how this PR works with etld+1 stuff?

It works great. Basically we want to keep that it shows the ETLD+1 in the middle of the bar, but I reduced the amount of padding on the sides so it doesn't look like it's severely cut off..

However, @jhreis has asked someone if we're okay with how many icons are in the URL bar causing it to look really cluttered on iPhone 5S. Not sure the verdict on that part.

@Brandon-T Brandon-T requested a review from jhreis November 19, 2019 16:13
@iccub
Copy link
Contributor

iccub commented Nov 19, 2019

thanks, 5S runs on iOS 12 and is a really small fraction of devices, this is definitely out of scope for this PR

Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lock icon is red for new tab page, reader mode and error pages is showing, and it's red color.
We should hide it

@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from d6a583a to f2ab308 Compare November 19, 2019 16:37
@Brandon-T Brandon-T requested a review from iccub November 19, 2019 17:13
@Brandon-T
Copy link
Collaborator Author

Brandon-T commented Nov 19, 2019

The lock icon is red for new tab page, reader mode and error pages is showing, and it's red color.
We should hide it

I fixed it for about page and reader mode by changing its colour back to the default or secure. Are you sure we should hide it? If we want to hide it, I can definitely do that but what about reader mode? Our GCD webserver has no cert or trust to evaluate so it will technically be insecure or undetermined. So when we switch back and forth from reader to non-reader mode, it will shift the URL every time if I hide it.

@jumde
Copy link
Contributor

jumde commented Nov 19, 2019

thanks, 5S runs on iOS 12 and is a really small fraction of devices, this is definitely out of scope for this PR

@iccub the experience is same on iPhone SE.

@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch 2 times, most recently from 42d9669 to da60cbe Compare November 26, 2019 13:41
@Brandon-T Brandon-T requested a review from iccub November 26, 2019 13:43
@Brandon-T Brandon-T added this to the 1.15 milestone Nov 26, 2019
@kylehickinson kylehickinson removed this from the 1.15 milestone Nov 29, 2019
@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from da60cbe to 15f2859 Compare December 9, 2019 16:15
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait with merging until security review is finished

@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from 15f2859 to 5a78ee0 Compare December 16, 2019 15:55
@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from 5a78ee0 to acd81b7 Compare January 8, 2020 13:42
@Brandon-T Brandon-T requested review from jhreis and fmarier January 10, 2020 19:53
@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from 1d086f4 to 27bb6a8 Compare January 16, 2020 17:34
@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from 27bb6a8 to af79dad Compare January 20, 2020 19:12
@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from af79dad to bd5d487 Compare January 28, 2020 16:38
@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from 7659157 to 3164491 Compare February 14, 2020 17:26
…olean. This allows us to determine what the state of the security truly is and allows us to display the correct lock icon for reader mode and about page.
…e to the interstitial page and the GCD-WebServer, it redirects causes the icon to flash and keep the colour of the GCD-Server which is trusted and marked secure. This was a bug and is now fixed.
@Brandon-T Brandon-T force-pushed the feature/SecurityFixes branch from 3164491 to fd7cf6d Compare April 15, 2020 17:38
@jumde
Copy link
Contributor

jumde commented Apr 15, 2020

security review approved 👍

@jhreis jhreis merged commit 7b79971 into development Apr 15, 2020
@jhreis jhreis deleted the feature/SecurityFixes branch April 15, 2020 19:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants