New issue
Advanced search Search tips

Issue 739480 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug
Team-Security-UX

Blocking:
issue 721898


Show other hotlists

Hotlists containing this issue:
Security-UX-Consistency


Sign in to add a comment

mark-non-secure-as Dangerous has wrong behavior on iOS

Project Member Reported by elawrence@chromium.org, Jul 5 2017

Issue description

Chrome Version: 61.0.3142.0
OS: iOS 10.3.2

What steps will reproduce the problem?
(1) Open chrome://flags on iOS and set mark-non-secure-as to "Always mark HTTP as actively dangerous"
(2) Restart Chrome (manually)
(3) Visit chrome://version

What is the expected result? Security chip and protocol renders normally.

What happens instead? Security chip turns to /!\ and the "chrome" scheme shows in red with a strikethrough.

 
Labels: Hotlist-GoodFirstBug
Status: Available (was: Untriaged)
I *think* this shouldn't be too bad: https://cs.chromium.org/chromium/src/ios/chrome/browser/ui/omnibox/omnibox_view_ios.mm?dr=C&q=%22OmniboxViewIOS::UpdateSchemeStyle%22&l=635

Optimistically also labeling with Hotlist-GoodFirstBug.
Blocking: 721898
Labels: -Pri-3 Hotlist-HttpBad Pri-2
The security_state::GetSecurityInfo function that sets security_state::DANGEROUS based on the mark-non-secure-as setting consults the supplied IsOriginSecureCallback callback to allow the host to decide whether a scheme is secure or not.

On Desktop, that's content::IsOriginSecure [1], which consults GetSecureSchemes() which is fed by RegisterContentSchemes() [2] with a list of secure protocols, including kChromeUIScheme. Interestingly, several secure schemes, including kChromeNativeScheme (chrome-native:// used on Android) are omitted from this list, leading to other corner-case issues e.g.  Issue 734581 ).

On iOS, the IsOriginSecureCallback implementation is the much simpler (and source of the bug) web::IsOriginSecure [3]; it doesn't consult an extensible list of schemes, and as a consequence it leads to this bug.

The "simple" fix here would be to update web::IsOriginSecureCallback.
The possibly more robust fix would be to change security_state::GetSecurityInfo from

  if (!is_cryptographic_with_certificate) {
    if (!is_origin_secure_callback.Run(url) &&
        (url.IsStandard() || url.SchemeIs(url::kBlobScheme))) {
      return GetSecurityLevelForNonSecureFieldTrial(...)
    }

to 

  if (!is_cryptographic_with_certificate && url.IsStandard() &&
     (url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kFtpScheme) || url.SchemeIs(url::kBlobScheme)) {
      return GetSecurityLevelForNonSecureFieldTrial(...)
    }

... explicitly listing out the non-secure schemes for which we want this feature to apply.

[1] content::IsOriginSecure() https://cs.chromium.org/chromium/src/content/common/origin_util.cc?l=30&rcl=f6016b060cb2133958b6903a527f637adddf31b4
[2] RegisterContentSchemes() https://cs.chromium.org/chromium/src/content/common/url_schemes.cc?l=40&rcl=43346fbbcf8132e6eba96218d97f0b8b56d03eda
[3] web::IsOriginSecure()  https://cs.chromium.org/chromium/src/ios/web/public/origin_util.mm?l=21&rcl=b2464355416823fb43fc861f51a3ea0135c32932
Hmm. Changing security_state::GetSecurityInfo as suggested in #2 would mean that the existing GetSecureOrigins() call made on desktop is skipped, which would mean that command line overrides propagated by GetSecureOriginWhitelist() would be ignored. So we can't really do that. 

We could do something like:

  if (!is_cryptographic_with_certificate && url.IsStandard() && !is_origin_secure_callback.Run(url) &&
     (url.SchemeIs(url::kHttpScheme) || url.SchemeIs(url::kFtpScheme) || url.SchemeIs(url::kBlobScheme)) {
      return GetSecurityLevelForNonSecureFieldTrial(...)
  }

...
Owner: elawrence@chromium.org
Status: Assigned (was: Available)
Presently, almost all traffic on iOS is from one of these protocols: [http,https,file,data,about,chrome]

We're deprecating data: navigations at the top level and are fine with about: showing as non-secure, so for iOS the fix is trivial.
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0

commit e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0
Author: Eric Lawrence <elawrence@chromium.org>
Date: Thu Jul 13 18:57:47 2017

iOS: Update IsOriginSecure to consult WebClient-set Secure Schemes

iOS previously did not consult the embedder for its list of secure
schemes, meaning that chrome:// urls were not treated as secure. This
change enables the Web Client to declare a list of application-specific
secure schemes, and adds the chrome:// scheme to that list.

Bug:  739480 
Change-Id: Ia794aef6d9b34229755f9c7aad0cbc93158a659e
Reviewed-on: https://chromium-review.googlesource.com/568624
Commit-Queue: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486450}
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/chrome/browser/web/chrome_web_client.h
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/chrome/browser/web/chrome_web_client.mm
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/web/public/origin_util.mm
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/web/public/test/fakes/test_web_client.h
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/web/public/test/fakes/test_web_client.mm
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/web/public/url_schemes.h
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/web/public/url_schemes.mm
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/web/public/web_client.h
[modify] https://crrev.com/e74ed3b6e61d7e43b5ebbdba55a2cc64607d98c0/ios/web/public/web_client.mm

commit e74ed3b6e61d7e43b5eb was:
  initially in 61.0.3157.0
Status: Fixed (was: Started)

Sign in to add a comment