Downgrade page security if an inline has invalid certificate.

If an inline loaded resource has invalid certificate, the overall security of the webpage should be downgraded.

[joleksy, 2014-02-26 12:04:25]

@reviewer: does this patch look OK to you?

[sky, 2014-02-26 14:31:54]

Security stuff always makes me nervous.
+creis

[Charlie Reis, 2014-02-26 18:49:05]

Thanks, sky!

Adam, you might know the right place for this code, though I know it's been a
long time.  I've cc'd palmer@ and rsleevi@ in case they know as well.

https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1810:
displayed_insecure_content_ = true;
I don't know the SSL manager code very well, but it seems like this logic might
belong in SSLManager::DidStartResourceResponse (or something related to it), and
that it should trigger a call to OnDid{Display|Run}InsecureContent.  Just
setting the flag doesn't send out all the intended notifications.

[Ryan Sleevi, 2014-02-26 18:53:14]

On 2014/02/26 18:49:05, Charlie Reis wrote:
> Thanks, sky!
> 
> Adam, you might know the right place for this code, though I know it's been a
> long time.  I've cc'd palmer@ and rsleevi@ in case they know as well.
> 
>
https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_impl.cc:1810:
> displayed_insecure_content_ = true;
> I don't know the SSL manager code very well, but it seems like this logic
might
> belong in SSLManager::DidStartResourceResponse (or something related to it),
and
> that it should trigger a call to OnDid{Display|Run}InsecureContent.  Just
> setting the flag doesn't send out all the intended notifications.

BTW, thanks for taking this on. I was actually planning to do something similar
this week, so I really appreciate it :)

That said, I'm not sure the right place, but will be following this review
carefully.

Also, tests? We have all the testing infrastructure to support this - both using
an out-of-process HTTPS server (meh) or using the URLRequest interception layers
(yay) - so we should definitely have robust test coverage before landing.

[abarth-chromium, 2014-02-26 19:17:06]

https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:1810:
displayed_insecure_content_ = true;
On 2014/02/26 18:49:06, Charlie Reis wrote:
> I don't know the SSL manager code very well, but it seems like this logic
might
> belong in SSLManager::DidStartResourceResponse (or something related to it),
and
> that it should trigger a call to OnDid{Display|Run}InsecureContent.  Just
> setting the flag doesn't send out all the intended notifications.

Yep

[joleksy, 2014-02-27 07:37:10]

On 2014/02/26 19:17:06, abarth wrote:
>
https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
> content/browser/web_contents/web_contents_impl.cc:1810:
> displayed_insecure_content_ = true;
> On 2014/02/26 18:49:06, Charlie Reis wrote:
> > I don't know the SSL manager code very well, but it seems like this logic
> might
> > belong in SSLManager::DidStartResourceResponse (or something related to it),
> and
> > that it should trigger a call to OnDid{Display|Run}InsecureContent.  Just
> > setting the flag doesn't send out all the intended notifications.
> 
> Yep

In both cases notification happens further down the line (in
SSLPolicy::OnRequestStarted, which in turn calls
SSLPolicyBackend::HostRanInsecureContent).
Actually my first solution here was to handle this situation in
SSLPolicyBackend::HostRanInsecureContent(), do you think this should be the way
to go?
Note: this leads to problems with discovering the proper WebContent.

As for the test - what I thought is that it would be really hard to create a
test for this, but if you say we do have everything in place, I will try to
prepare the proper test.

[joleksy, 2014-02-27 15:32:46]

On 2014/02/27 07:37:10, joleksy wrote:
> On 2014/02/26 19:17:06, abarth wrote:
> >
>
https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/181253003/diff/1/content/browser/web_contents...
> > content/browser/web_contents/web_contents_impl.cc:1810:
> > displayed_insecure_content_ = true;
> > On 2014/02/26 18:49:06, Charlie Reis wrote:
> > > I don't know the SSL manager code very well, but it seems like this logic
> > might
> > > belong in SSLManager::DidStartResourceResponse (or something related to
it),
> > and
> > > that it should trigger a call to OnDid{Display|Run}InsecureContent.  Just
> > > setting the flag doesn't send out all the intended notifications.
> > 
> > Yep
> 
> In both cases notification happens further down the line (in
> SSLPolicy::OnRequestStarted, which in turn calls
> SSLPolicyBackend::HostRanInsecureContent).
> Actually my first solution here was to handle this situation in
> SSLPolicyBackend::HostRanInsecureContent(), do you think this should be the
way
> to go?
> Note: this leads to problems with discovering the proper WebContent.
> 
> As for the test - what I thought is that it would be really hard to create a
> test for this, but if you say we do have everything in place, I will try to
> prepare the proper test.

Elaborating a bit on the (lack of) notification issue:

"standard" way of setting displayed_insecure_content_ flag is through
WebContentsImpl::OnDidDisplayInsecureContent(). This is a notification that
comes from WebKit if http inline is loaded in secure context.

What happens in the callback is:
 - the flag is set
 - SSLManager::NotifySSLInternalStateChanged() is called

When the certificate for an inline is invalid, WebKit will not send the
notification, and because of that I need to set the flag manually.
The flag is set in WebContentsImpl::DidGetResourceResponseStart() or
WebContentsImpl::OnDidLoadResourceFromMemoryCache()

What happens afterwards:
SSLManager::DidStartResourceRespons (or SSLManager::DidLoadFromMemoryCache())
SSLPolicy::OnRequestStarted()
SSLPolicyBackend::HostRanInsecureContent()
SSLManager::NotifySSLInternalStateChanged()

meaning that all the intended notifications are sent.

After SSLManager is called from WebContentsImpl, the information about the
specific WebContent object is lost (or at least is harder to retrieve). And we
would probably need a setter for the flag (using the
OnDidDisplayInsecureContent() method would cause
SSLManager::NotifySSLInternalStateChanged to be called twice).

Please correct me if any of the above is wrong.

[Charlie Reis, 2014-02-28 02:07:59]

Sorry, this level of detail is beyond my knowledge and I didn't have time to dig
into it.  Ryan, perhaps you would be able to help?

[palmer, 2014-03-04 00:23:21]

On 2014/02/28 02:07:59, Charlie Reis wrote:

> Sorry, this level of detail is beyond my knowledge and I didn't have time to
dig
> into it.  Ryan, perhaps you would be able to help?

Did you mean rsleevi?

[Charlie Reis, 2014-03-04 00:30:07]

On 2014/03/04 00:23:21, Chromium Palmer wrote:
> On 2014/02/28 02:07:59, Charlie Reis wrote:
> 
> > Sorry, this level of detail is beyond my knowledge and I didn't have time to
> dig
> > into it.  Ryan, perhaps you would be able to help?
> 
> Did you mean rsleevi?

Yes, but I'm happy to get input from anyone who knows more about SSLManager than
me-- suggestions welcome.  :)

[Ryan Sleevi, 2014-03-11 01:39:39]

Sorry for the delay. More tests? :)

https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_browser_tests.cc:1114: }
Can you add an additional test to test for 'intranet' names, so that we can make
sure the behaviour is as expected?

On internal host names, we don't force an interstitial, but do degrade the UI.
Ideally, we'd propagate that brokenness (eg: if good.com included a resource
from myinternalhost.invalid)

We can accomplish this with a MockCertVerifier (to set the desired status for
given certificate/hostnames) and a RuleBasedHostResolverProc (IIRC)

[joleksy, 2014-03-14 09:51:09]

On 2014/03/11 01:39:39, Ryan Sleevi wrote:
> Sorry for the delay. More tests? :)
> 
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> File chrome/browser/ssl/ssl_browser_tests.cc (right):
> 
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> chrome/browser/ssl/ssl_browser_tests.cc:1114: }
> Can you add an additional test to test for 'intranet' names, so that we can
make
> sure the behaviour is as expected?
> 
> On internal host names, we don't force an interstitial, but do degrade the UI.
> Ideally, we'd propagate that brokenness (eg: if http://good.com included a
resource
> from myinternalhost.invalid)
> 
> We can accomplish this with a MockCertVerifier (to set the desired status for
> given certificate/hostnames) and a RuleBasedHostResolverProc (IIRC)

I am not sure if I understand this scenario.
You mean: the resource should be opened from internal url (like
myinternalhost.invalid), the host should be resolved to some https server with
broken cert. Is my understanding correct? If yes - why do we need
MockCertVerifier? We already have broken certificate on
https_server_mismatched_.

[Ryan Sleevi, 2014-03-14 21:12:29]

On 2014/03/14 09:51:09, joleksy wrote:
> On 2014/03/11 01:39:39, Ryan Sleevi wrote:
> > Sorry for the delay. More tests? :)
> > 
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> > File chrome/browser/ssl/ssl_browser_tests.cc (right):
> > 
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> > chrome/browser/ssl/ssl_browser_tests.cc:1114: }
> > Can you add an additional test to test for 'intranet' names, so that we can
> make
> > sure the behaviour is as expected?
> > 
> > On internal host names, we don't force an interstitial, but do degrade the
UI.
> > Ideally, we'd propagate that brokenness (eg: if http://good.com included a
> resource
> > from myinternalhost.invalid)
> > 
> > We can accomplish this with a MockCertVerifier (to set the desired status
for
> > given certificate/hostnames) and a RuleBasedHostResolverProc (IIRC)
> 
> I am not sure if I understand this scenario.
> You mean: the resource should be opened from internal url (like
> myinternalhost.invalid), the host should be resolved to some https server with
> broken cert. Is my understanding correct? If yes - why do we need
> MockCertVerifier? We already have broken certificate on
> https_server_mismatched_.

So, the scenario is a this:
https://good.com - has a valid cert
https://good.com includes a resource from https://myinternalhost.internal
https://myinternalhost.internal has a valid cert (in that it's trusted), but
it's seen as 'less than stellar' for policy reasons, so we degrade it

The question is what is the security status for that https://good.com load

This is somewhat similar to
https://good.com - has a valid cert
https://good.com includes a resource from https://bad.com, which has a bad cert

I can't remember if, today, we block the resource load only, or if we block the
resource load AND show mixed-content on https://good.com
Further, I can't remember what we do if the user had previously visited
https://bad.com and bypassed the certificate error, THEN visits https://good.com

The context of why I asked for this test is that we're trying to nail down
exactly what should happen with https://codereview.chromium.org/20628006/ ,
which Sigbjorn originally contributed. Today, the only way to trigger the "bad
but not interstitialed" behaviour is with an internal server name
(myinternalhost.internal - although any name *not* on the Public Suffix List
suffices)

However, with that change from Sibjorn, and with another policy change of
enforcing the BRs, we're going to show it for overly-valid certificates and for
certificates that don't meet the cryptographic requirements of the BRs. I want
to make sure we've got tests that cover this propogation (which doesn't happen
today), so the only way to do that today is for the .internal host

[joleksy, 2014-03-17 13:51:40]

On 2014/03/14 21:12:29, Ryan Sleevi wrote:
> On 2014/03/14 09:51:09, joleksy wrote:
> > On 2014/03/11 01:39:39, Ryan Sleevi wrote:
> > > Sorry for the delay. More tests? :)
> > > 
> > >
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> > > File chrome/browser/ssl/ssl_browser_tests.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> > > chrome/browser/ssl/ssl_browser_tests.cc:1114: }
> > > Can you add an additional test to test for 'intranet' names, so that we
can
> > make
> > > sure the behaviour is as expected?
> > > 
> > > On internal host names, we don't force an interstitial, but do degrade the
> UI.
> > > Ideally, we'd propagate that brokenness (eg: if http://good.com included a
> > resource
> > > from myinternalhost.invalid)
> > > 
> > > We can accomplish this with a MockCertVerifier (to set the desired status
> for
> > > given certificate/hostnames) and a RuleBasedHostResolverProc (IIRC)
> > 
> > I am not sure if I understand this scenario.
> > You mean: the resource should be opened from internal url (like
> > myinternalhost.invalid), the host should be resolved to some https server
with
> > broken cert. Is my understanding correct? If yes - why do we need
> > MockCertVerifier? We already have broken certificate on
> > https_server_mismatched_.
> 
> So, the scenario is a this:
> https://good.com - has a valid cert
> https://good.com includes a resource from https://myinternalhost.internal
> https://myinternalhost.internal has a valid cert (in that it's trusted), but
> it's seen as 'less than stellar' for policy reasons, so we degrade it
> 
> The question is what is the security status for that https://good.com load
> 
> This is somewhat similar to
> https://good.com - has a valid cert
> https://good.com includes a resource from https://bad.com, which has a bad
cert
> 
> I can't remember if, today, we block the resource load only, or if we block
the
> resource load AND show mixed-content on https://good.com
> Further, I can't remember what we do if the user had previously visited
> https://bad.com and bypassed the certificate error, THEN visits
https://good.com
> 
> The context of why I asked for this test is that we're trying to nail down
> exactly what should happen with https://codereview.chromium.org/20628006/ ,
> which Sigbjorn originally contributed. Today, the only way to trigger the "bad
> but not interstitialed" behaviour is with an internal server name
> (myinternalhost.internal - although any name *not* on the Public Suffix List
> suffices)
> 
> However, with that change from Sibjorn, and with another policy change of
> enforcing the BRs, we're going to show it for overly-valid certificates and
for
> certificates that don't meet the cryptographic requirements of the BRs. I want
> to make sure we've got tests that cover this propogation (which doesn't happen
> today), so the only way to do that today is for the .internal host

> > On internal host names, we don't force an interstitial, but do degrade the
UI.
I am using intranet hostnames (i.e. with name not on Public Suffix List) in my
daily work, the security is not downgraded (i.e. the UI is shown as fully
secure).
Also I performed a quick test and included an intranet resource on a secure
page, with similar result (i.e. fully secure webpage).
After I tried to perform the test with intranet address being resolved as ssh
test server all I got was a warning about mismatched server name (which is
expected), but I cannot see any difference in handling internal and public
hostnames.

What am I missing? How can I force the UI to be degraded?

[Ryan Sleevi, 2014-03-17 15:01:24]

On Mar 17, 2014 6:51 AM, <joleksy@opera.com> wrote:
>
> On 2014/03/14 21:12:29, Ryan Sleevi wrote:
>>
>> On 2014/03/14 09:51:09, joleksy wrote:
>> > On 2014/03/11 01:39:39, Ryan Sleevi wrote:
>> > > Sorry for the delay. More tests? :)
>> > >
>> > >
>> >
>
>
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
>>
>> > > File chrome/browser/ssl/ssl_browser_tests.cc (right):
>> > >
>> > >
>> >
>
>
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
>>
>> > > chrome/browser/ssl/ssl_browser_tests.cc:1114: }
>> > > Can you add an additional test to test for 'intranet' names, so that
we
>
> can
>>
>> > make
>> > > sure the behaviour is as expected?
>> > >
>> > > On internal host names, we don't force an interstitial, but do
degrade the
>> UI.
>> > > Ideally, we'd propagate that brokenness (eg: if http://good.comincluded
a
>> > resource
>> > > from myinternalhost.invalid)
>> > >
>> > > We can accomplish this with a MockCertVerifier (to set the desired
status
>> for
>> > > given certificate/hostnames) and a RuleBasedHostResolverProc (IIRC)
>> >
>> > I am not sure if I understand this scenario.
>> > You mean: the resource should be opened from internal url (like
>> > myinternalhost.invalid), the host should be resolved to some https
server
>
> with
>>
>> > broken cert. Is my understanding correct? If yes - why do we need
>> > MockCertVerifier? We already have broken certificate on
>> > https_server_mismatched_.
>
>
>> So, the scenario is a this:
>> https://good.com - has a valid cert
>> https://good.com includes a resource from https://myinternalhost.internal
>> https://myinternalhost.internal has a valid cert (in that it's trusted),
but
>> it's seen as 'less than stellar' for policy reasons, so we degrade it
>
>
>> The question is what is the security status for that https://good.comload
>
>
>> This is somewhat similar to
>> https://good.com - has a valid cert
>> https://good.com includes a resource from https://bad.com, which has a
bad
>
> cert
>
>> I can't remember if, today, we block the resource load only, or if we
block
>
> the
>>
>> resource load AND show mixed-content on https://good.com
>> Further, I can't remember what we do if the user had previously visited
>> https://bad.com and bypassed the certificate error, THEN visits
>
> https://good.com
>
>> The context of why I asked for this test is that we're trying to nail
down
>> exactly what should happen with https://codereview.chromium.org/20628006/,
>> which Sigbjorn originally contributed. Today, the only way to trigger
the "bad
>> but not interstitialed" behaviour is with an internal server name
>> (myinternalhost.internal - although any name *not* on the Public Suffix
List
>> suffices)
>
>
>> However, with that change from Sibjorn, and with another policy change of
>> enforcing the BRs, we're going to show it for overly-valid certificates
and
>
> for
>>
>> certificates that don't meet the cryptographic requirements of the BRs.
I want
>> to make sure we've got tests that cover this propogation (which doesn't
happen
>> today), so the only way to do that today is for the .internal host
>
>
>> > On internal host names, we don't force an interstitial, but do degrade
the
>
> UI.
> I am using intranet hostnames (i.e. with name not on Public Suffix List)
in my
> daily work, the security is not downgraded (i.e. the UI is shown as fully
> secure).
> Also I performed a quick test and included an intranet resource on a
secure
> page, with similar result (i.e. fully secure webpage).
> After I tried to perform the test with intranet address being resolved as
ssh
> test server all I got was a warning about mismatched server name (which is
> expected), but I cannot see any difference in handling internal and public
> hostnames.
>
> What am I missing? How can I force the UI to be degraded?

Has to be a public CA cert (eg: not an internal cert)

net::TestRootCerts allows you to simulate this.

>
> https://codereview.chromium.org/181253003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[joleksy, 2014-03-18 11:57:56]

On 2014/03/17 15:01:24, Ryan Sleevi wrote:
> On Mar 17, 2014 6:51 AM, <mailto:joleksy@opera.com> wrote:
> >
> > On 2014/03/14 21:12:29, Ryan Sleevi wrote:
> >>
> >> On 2014/03/14 09:51:09, joleksy wrote:
> >> > On 2014/03/11 01:39:39, Ryan Sleevi wrote:
> >> > > Sorry for the delay. More tests? :)
> >> > >
> >> > >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> >>
> >> > > File chrome/browser/ssl/ssl_browser_tests.cc (right):
> >> > >
> >> > >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> >>
> >> > > chrome/browser/ssl/ssl_browser_tests.cc:1114: }
> >> > > Can you add an additional test to test for 'intranet' names, so that
> we
> >
> > can
> >>
> >> > make
> >> > > sure the behaviour is as expected?
> >> > >
> >> > > On internal host names, we don't force an interstitial, but do
> degrade the
> >> UI.
> >> > > Ideally, we'd propagate that brokenness (eg: if http://good.comincluded
> a
> >> > resource
> >> > > from myinternalhost.invalid)
> >> > >
> >> > > We can accomplish this with a MockCertVerifier (to set the desired
> status
> >> for
> >> > > given certificate/hostnames) and a RuleBasedHostResolverProc (IIRC)
> >> >
> >> > I am not sure if I understand this scenario.
> >> > You mean: the resource should be opened from internal url (like
> >> > myinternalhost.invalid), the host should be resolved to some https
> server
> >
> > with
> >>
> >> > broken cert. Is my understanding correct? If yes - why do we need
> >> > MockCertVerifier? We already have broken certificate on
> >> > https_server_mismatched_.
> >
> >
> >> So, the scenario is a this:
> >> https://good.com - has a valid cert
> >> https://good.com includes a resource from https://myinternalhost.internal
> >> https://myinternalhost.internal has a valid cert (in that it's trusted),
> but
> >> it's seen as 'less than stellar' for policy reasons, so we degrade it
> >
> >
> >> The question is what is the security status for that https://good.comload
> >
> >
> >> This is somewhat similar to
> >> https://good.com - has a valid cert
> >> https://good.com includes a resource from https://bad.com, which has a
> bad
> >
> > cert
> >
> >> I can't remember if, today, we block the resource load only, or if we
> block
> >
> > the
> >>
> >> resource load AND show mixed-content on https://good.com
> >> Further, I can't remember what we do if the user had previously visited
> >> https://bad.com and bypassed the certificate error, THEN visits
> >
> > https://good.com
> >
> >> The context of why I asked for this test is that we're trying to nail
> down
> >> exactly what should happen with https://codereview.chromium.org/20628006/,
> >> which Sigbjorn originally contributed. Today, the only way to trigger
> the "bad
> >> but not interstitialed" behaviour is with an internal server name
> >> (myinternalhost.internal - although any name *not* on the Public Suffix
> List
> >> suffices)
> >
> >
> >> However, with that change from Sibjorn, and with another policy change of
> >> enforcing the BRs, we're going to show it for overly-valid certificates
> and
> >
> > for
> >>
> >> certificates that don't meet the cryptographic requirements of the BRs.
> I want
> >> to make sure we've got tests that cover this propogation (which doesn't
> happen
> >> today), so the only way to do that today is for the .internal host
> >
> >
> >> > On internal host names, we don't force an interstitial, but do degrade
> the
> >
> > UI.
> > I am using intranet hostnames (i.e. with name not on Public Suffix List)
> in my
> > daily work, the security is not downgraded (i.e. the UI is shown as fully
> > secure).
> > Also I performed a quick test and included an intranet resource on a
> secure
> > page, with similar result (i.e. fully secure webpage).
> > After I tried to perform the test with intranet address being resolved as
> ssh
> > test server all I got was a warning about mismatched server name (which is
> > expected), but I cannot see any difference in handling internal and public
> > hostnames.
> >
> > What am I missing? How can I force the UI to be degraded?
> 
> Has to be a public CA cert (eg: not an internal cert)
> 
> net::TestRootCerts allows you to simulate this.
> 
> >
> > https://codereview.chromium.org/181253003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

So basically you want to have a test to check whether or not the error code
CERT_STATUS_NON_UNIQUE_NAME is propagated properly.
I do not think this error status is any different from the others and it will be
detected in net::IsCertStatusError(), then displayed_insecure_content_ will be
set. Does it deserve a separate treatment?

More interesting question can be asked: is this status set properly when we are
loading webpage from intranet hostname with certificate issued by known root
(i.e. is_issued_by_known_root is set). Do we have a test for this?

Please correct me if I am wrong but I did not find any place to hook custom
certificate verification from browsertest.
What I think would be best is to have some place that would allow us to modify
SSLInfo (and/or CertVerifyResult) after certificate is verified. This would make
it easy to e.g. add the flag CERT_STATUS_NON_UNIQUE_NAME or set
is_issued_by_known_root.

Note: I just did a quick test and indeed, the cert status is set properly and
insecure content is propagated. But in order to test it I needed to modify
cert_verify_proc_mac.cc (i.e. change IsIssuedByKnownRoot())

[Ryan Sleevi, 2014-03-18 21:04:23]

On 2014/03/18 11:57:56, joleksy wrote:
> On 2014/03/17 15:01:24, Ryan Sleevi wrote:
> > On Mar 17, 2014 6:51 AM, <mailto:joleksy@opera.com> wrote:
> > >
> > > On 2014/03/14 21:12:29, Ryan Sleevi wrote:
> > >>
> > >> On 2014/03/14 09:51:09, joleksy wrote:
> > >> > On 2014/03/11 01:39:39, Ryan Sleevi wrote:
> > >> > > Sorry for the delay. More tests? :)
> > >> > >
> > >> > >
> > >> >
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> > >>
> > >> > > File chrome/browser/ssl/ssl_browser_tests.cc (right):
> > >> > >
> > >> > >
> > >> >
> > >
> > >
> > >
> >
>
https://codereview.chromium.org/181253003/diff/20001/chrome/browser/ssl/ssl_b...
> > >>
> > >> > > chrome/browser/ssl/ssl_browser_tests.cc:1114: }
> > >> > > Can you add an additional test to test for 'intranet' names, so that
> > we
> > >
> > > can
> > >>
> > >> > make
> > >> > > sure the behaviour is as expected?
> > >> > >
> > >> > > On internal host names, we don't force an interstitial, but do
> > degrade the
> > >> UI.
> > >> > > Ideally, we'd propagate that brokenness (eg: if
http://good.comincluded
> > a
> > >> > resource
> > >> > > from myinternalhost.invalid)
> > >> > >
> > >> > > We can accomplish this with a MockCertVerifier (to set the desired
> > status
> > >> for
> > >> > > given certificate/hostnames) and a RuleBasedHostResolverProc (IIRC)
> > >> >
> > >> > I am not sure if I understand this scenario.
> > >> > You mean: the resource should be opened from internal url (like
> > >> > myinternalhost.invalid), the host should be resolved to some https
> > server
> > >
> > > with
> > >>
> > >> > broken cert. Is my understanding correct? If yes - why do we need
> > >> > MockCertVerifier? We already have broken certificate on
> > >> > https_server_mismatched_.
> > >
> > >
> > >> So, the scenario is a this:
> > >> https://good.com - has a valid cert
> > >> https://good.com includes a resource from https://myinternalhost.internal
> > >> https://myinternalhost.internal has a valid cert (in that it's trusted),
> > but
> > >> it's seen as 'less than stellar' for policy reasons, so we degrade it
> > >
> > >
> > >> The question is what is the security status for that https://good.comload
> > >
> > >
> > >> This is somewhat similar to
> > >> https://good.com - has a valid cert
> > >> https://good.com includes a resource from https://bad.com, which has a
> > bad
> > >
> > > cert
> > >
> > >> I can't remember if, today, we block the resource load only, or if we
> > block
> > >
> > > the
> > >>
> > >> resource load AND show mixed-content on https://good.com
> > >> Further, I can't remember what we do if the user had previously visited
> > >> https://bad.com and bypassed the certificate error, THEN visits
> > >
> > > https://good.com
> > >
> > >> The context of why I asked for this test is that we're trying to nail
> > down
> > >> exactly what should happen with
https://codereview.chromium.org/20628006/,
> > >> which Sigbjorn originally contributed. Today, the only way to trigger
> > the "bad
> > >> but not interstitialed" behaviour is with an internal server name
> > >> (myinternalhost.internal - although any name *not* on the Public Suffix
> > List
> > >> suffices)
> > >
> > >
> > >> However, with that change from Sibjorn, and with another policy change of
> > >> enforcing the BRs, we're going to show it for overly-valid certificates
> > and
> > >
> > > for
> > >>
> > >> certificates that don't meet the cryptographic requirements of the BRs.
> > I want
> > >> to make sure we've got tests that cover this propogation (which doesn't
> > happen
> > >> today), so the only way to do that today is for the .internal host
> > >
> > >
> > >> > On internal host names, we don't force an interstitial, but do degrade
> > the
> > >
> > > UI.
> > > I am using intranet hostnames (i.e. with name not on Public Suffix List)
> > in my
> > > daily work, the security is not downgraded (i.e. the UI is shown as fully
> > > secure).
> > > Also I performed a quick test and included an intranet resource on a
> > secure
> > > page, with similar result (i.e. fully secure webpage).
> > > After I tried to perform the test with intranet address being resolved as
> > ssh
> > > test server all I got was a warning about mismatched server name (which is
> > > expected), but I cannot see any difference in handling internal and public
> > > hostnames.
> > >
> > > What am I missing? How can I force the UI to be degraded?
> > 
> > Has to be a public CA cert (eg: not an internal cert)
> > 
> > net::TestRootCerts allows you to simulate this.
> > 
> > >
> > > https://codereview.chromium.org/181253003/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> So basically you want to have a test to check whether or not the error code
> CERT_STATUS_NON_UNIQUE_NAME is propagated properly.
> I do not think this error status is any different from the others and it will
be
> detected in net::IsCertStatusError(), then displayed_insecure_content_ will be
> set. Does it deserve a separate treatment?

It gets separate treatment by chrome/content (namely, it doesn't get an
interstitial). Mainly I just wanted to make sure we test for this, but I agree,
it can be a bit tricky (because we're testing the end result here

> 
> More interesting question can be asked: is this status set properly when we
are
> loading webpage from intranet hostname with certificate issued by known root
> (i.e. is_issued_by_known_root is set). Do we have a test for this?

We *only* set this for cases when is_issued_by_known_root is set.

Test is
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/cert_veri...

> 
> Please correct me if I am wrong but I did not find any place to hook custom
> certificate verification from browsertest.
> What I think would be best is to have some place that would allow us to modify
> SSLInfo (and/or CertVerifyResult) after certificate is verified. This would
make
> it easy to e.g. add the flag CERT_STATUS_NON_UNIQUE_NAME or set
> is_issued_by_known_root.

Yeah, "easy" isn't exactly the way to describe it, but it is (supposed to) be
possible.

In essence, you create a new Profile/Profile subclass, override the
Profile/ProfileIOData's CertVerifier, then you can do whatever.

The Sync tests (
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
) are an example of this. Look for calls to
profile_manager->RegisterTestingProfile() for code that sets up a dummy profile.


> 
> Note: I just did a quick test and indeed, the cert status is set properly and
> insecure content is propagated. But in order to test it I needed to modify
> cert_verify_proc_mac.cc (i.e. change IsIssuedByKnownRoot())

Fair enough. I think we can probably defer this test and I'll take it on myself
as we propagate the error through.

[joleksy, 2014-03-21 09:31:43]

On 2014/03/18 21:04:23, Ryan Sleevi wrote:
> 
> Fair enough. I think we can probably defer this test and I'll take it on
myself
> as we propagate the error through.

Does it mean: accepted?

[joleksy, 2014-03-21 09:32:48]

On 2014/03/21 09:31:43, joleksy wrote:
> On 2014/03/18 21:04:23, Ryan Sleevi wrote:
> > 
> > Fair enough. I think we can probably defer this test and I'll take it on
> myself
> > as we propagate the error through.
> 
> Does it mean: accepted?

And BTW - we would probably want to wait until
https://codereview.chromium.org/184483002/ gets (hopefully) integrated, and then
rebase this one.

[Ryan Sleevi, 2014-03-21 20:22:54]

LGTM, mod style nit.

https://codereview.chromium.org/181253003/diff/20001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/181253003/diff/20001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1811: }
style nit: Throughout this file, it seems like it uses brace-less ifs for
one-liners

Per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
, let's "be consistent" and drop the {}

[joleksy, 2014-04-09 13:09:12]

On 2014/03/21 20:22:54, Ryan Sleevi wrote:
> LGTM, mod style nit.
> 
>
https://codereview.chromium.org/181253003/diff/20001/content/browser/web_cont...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/181253003/diff/20001/content/browser/web_cont...
> content/browser/web_contents/web_contents_impl.cc:1811: }
> style nit: Throughout this file, it seems like it uses brace-less ifs for
> one-liners
> 
> Per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
> , let's "be consistent" and drop the {}

Braces removed, browser test rebased.
Is there anything else I should do in order to get this integrated?

[Ryan Sleevi, 2014-04-09 16:51:08]

On 2014/04/09 13:09:12, joleksy wrote:
> On 2014/03/21 20:22:54, Ryan Sleevi wrote:
> > LGTM, mod style nit.
> > 
> >
>
https://codereview.chromium.org/181253003/diff/20001/content/browser/web_cont...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/181253003/diff/20001/content/browser/web_cont...
> > content/browser/web_contents/web_contents_impl.cc:1811: }
> > style nit: Throughout this file, it seems like it uses brace-less ifs for
> > one-liners
> > 
> > Per
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
> > , let's "be consistent" and drop the {}
> 
> Braces removed, browser test rebased.
> Is there anything else I should do in order to get this integrated?

You just need an LGTM from someone for content/browser/web_contents

[Charlie Reis, 2014-04-09 17:20:17]

Thanks for working through it.  LGTM once you add the comment mentioned below.

https://codereview.chromium.org/181253003/diff/40001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/181253003/diff/40001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1886:
displayed_insecure_content_ = true;
I think the main thing that confused me was that it's not clear the appropriate
notifications will be sent in DidStartResourceResponse (and same in the case
below), and thus we don't need to call OnDidDisplayInsecureContent.

Can you add a comment to that effect both here and in
OnDidLoadResourceFromMemoryCache?

[joleksy, 2014-04-10 13:48:39]

On 2014/04/09 17:20:17, Charlie Reis wrote:
> Can you add a comment to that effect both here and in
> OnDidLoadResourceFromMemoryCache?

Comments added.
If you have any comments about the comments, do not hesitate to comment on it
;-)

[Charlie Reis, 2014-04-10 16:57:19]

Great!  I think you can even trim it down a bit.  :)  LGTM with the nits below.

https://codereview.chromium.org/181253003/diff/60001/content/browser/web_cont...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/181253003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1886: // See
https://codereview.chromium.org/181253003/
No need for this line-- the rest of the summary is sufficient.  (We can always
track down the CL that introduced the comment in revision history.)

https://codereview.chromium.org/181253003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1895: // from
SSLManager::DidStartResourceResponse().
This comment is great.

https://codereview.chromium.org/181253003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:1896: // See
SSLPolicyBackend::HostRanInsecureContent()
No need for this line, since it's easy for it to get stale if that
implementation changes.  We just care that DidStartResourceResponse will do it,
as you mention above.

https://codereview.chromium.org/181253003/diff/60001/content/browser/web_cont...
content/browser/web_contents/web_contents_impl.cc:2293: // Note: the call to
SSLManager::NotifySSLInternalStateChanged() will be done
No need for these last 3 lines, since they're covered in the comment you
reference.

[joleksy, 2014-04-11 06:17:54]

On 2014/04/10 16:57:19, Charlie Reis wrote:
> Great!  I think you can even trim it down a bit.  :)  LGTM with the nits
below.
> 

Changes to comments applied.

[Charlie Reis, 2014-04-11 16:01:47]

Great, thanks.  Still LGTM.

[joleksy, 2014-04-14 13:27:28]

On 2014/04/11 16:01:47, Charlie Reis wrote:
> Great, thanks.  Still LGTM.

There is one more issue with this one. If there is a problem with inline
certificate that should be considered minor (like
CERT_STATUS_UNABLE_TO_CHECK_REVOCATION), the security status should not be
downgraded.

Please wait with integrating this one. Fix is coming.

[joleksy, 2014-04-15 07:35:27]

On 2014/04/14 13:27:28, joleksy wrote:
> On 2014/04/11 16:01:47, Charlie Reis wrote:
> > Great, thanks.  Still LGTM.
> 
> There is one more issue with this one. If there is a problem with inline
> certificate that should be considered minor (like
> CERT_STATUS_UNABLE_TO_CHECK_REVOCATION), the security status should not be
> downgraded.
> 
> Please wait with integrating this one. Fix is coming.

Added fix for minor cert error.

[Ryan Sleevi, 2014-04-16 19:45:05]

lgtm

[joleksy, 2014-09-04 07:31:37]

On 2014/04/16 19:45:05, Ryan Sleevi wrote:
> lgtm

Hey,
is there any chance of having this integrated?:-)

[Ryan Sleevi, 2014-09-04 19:48:47]

On 2014/09/04 07:31:37, joleksy wrote:
> On 2014/04/16 19:45:05, Ryan Sleevi wrote:
> > lgtm
> 
> Hey,
> is there any chance of having this integrated?:-)

You had the LGTMs, just needed to check the commit box.

Would you mind rebasing to ToT? I'll do a final stab and then we can just CQ it
in. Not sure what else is needed? :)

[joleksy, 2014-09-05 08:56:13]

On 2014/09/04 19:48:47, Ryan Sleevi wrote:
> On 2014/09/04 07:31:37, joleksy wrote:
> > On 2014/04/16 19:45:05, Ryan Sleevi wrote:
> > > lgtm
> > 
> > Hey,
> > is there any chance of having this integrated?:-)
> 
> You had the LGTMs, just needed to check the commit box.
> 
> Would you mind rebasing to ToT? I'll do a final stab and then we can just CQ
it
> in. Not sure what else is needed? :)

Sorry, I was completely unaware of the checkbox and thought that it just happens
automatically after getting LGTM.

[joleksy, 2014-09-08 07:54:57]

The CQ bit was checked by joleksy@opera.com

[commit-bot: I haz the power, 2014-09-08 07:55:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joleksy@opera.com/181253003/120001

[commit-bot: I haz the power, 2014-09-08 09:02:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-08 09:02:25]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joleksy, 2014-09-10 12:20:00]

On 2014/09/08 09:02:25, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_swarming on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

I added the flag to
SSLUITest.TestUnsafeContentsInWorker
SSLUITest.TestBadFrameNavigation

but it seems that SSLUITest.TestRefNavigation is somewhat random.
Note: there is  bug http://crbug.com/84419 for this test.

It seems that the flag is set when favicon request comes. Not sure how we should
handle this, any ideas?

[Ryan Sleevi, 2014-09-10 21:40:16]

On 2014/09/10 12:20:00, joleksy wrote:
> On 2014/09/08 09:02:25, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_rel_swarming on tryserver.chromium.mac
> >
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> I added the flag to
> SSLUITest.TestUnsafeContentsInWorker
> SSLUITest.TestBadFrameNavigation
> 
> but it seems that SSLUITest.TestRefNavigation is somewhat random.
> Note: there is  bug http://crbug.com/84419 for this test.
> 
> It seems that the flag is set when favicon request comes. Not sure how we
should
> handle this, any ideas?

I'm not sure. Are you able to reproduce locally? It sounds like the tests may
have inherent flakiness that this is exacerbating. The bad news is "last to
touch, first to own", which is you may need to dig in to the flakiness. I'm not
too familiar with the code, but if you're able to reproduce and report your
findings, I'll let you know if anything stands out as odd/unexpected.

[joleksy, 2014-09-11 06:43:10]

On 2014/09/10 21:40:16, Ryan Sleevi wrote:
> On 2014/09/10 12:20:00, joleksy wrote:
> > On 2014/09/08 09:02:25, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   mac_chromium_rel_swarming on tryserver.chromium.mac
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> > 
> > I added the flag to
> > SSLUITest.TestUnsafeContentsInWorker
> > SSLUITest.TestBadFrameNavigation
> > 
> > but it seems that SSLUITest.TestRefNavigation is somewhat random.
> > Note: there is  bug http://crbug.com/84419 for this test.
> > 
> > It seems that the flag is set when favicon request comes. Not sure how we
> should
> > handle this, any ideas?
> 
> I'm not sure. Are you able to reproduce locally? It sounds like the tests may
> have inherent flakiness that this is exacerbating. The bad news is "last to
> touch, first to own", which is you may need to dig in to the flakiness. I'm
not
> too familiar with the code, but if you're able to reproduce and report your
> findings, I'll let you know if anything stands out as odd/unexpected.

Yep, I can reproduce the randomness locally. I will try to find out what exactly
is missing here.

[joleksy, 2014-09-11 08:40:16]

On 2014/09/11 06:43:10, joleksy wrote:
> On 2014/09/10 21:40:16, Ryan Sleevi wrote:
> > On 2014/09/10 12:20:00, joleksy wrote:
> > > On 2014/09/08 09:02:25, I haz the power (commit-bot) wrote:
> > > > Try jobs failed on following builders:
> > > >   mac_chromium_rel_swarming on tryserver.chromium.mac
> > > >
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> > > 
> > > I added the flag to
> > > SSLUITest.TestUnsafeContentsInWorker
> > > SSLUITest.TestBadFrameNavigation
> > > 
> > > but it seems that SSLUITest.TestRefNavigation is somewhat random.
> > > Note: there is  bug http://crbug.com/84419 for this test.
> > > 
> > > It seems that the flag is set when favicon request comes. Not sure how we
> > should
> > > handle this, any ideas?
> > 
> > I'm not sure. Are you able to reproduce locally? It sounds like the tests
may
> > have inherent flakiness that this is exacerbating. The bad news is "last to
> > touch, first to own", which is you may need to dig in to the flakiness. I'm
> not
> > too familiar with the code, but if you're able to reproduce and report your
> > findings, I'll let you know if anything stands out as odd/unexpected.
> 
> Yep, I can reproduce the randomness locally. I will try to find out what
exactly
> is missing here.

Now I set the flag only when what we load is not favicon. This fixes the
randomness issue and seems like a logical thing to do.

[Ryan Sleevi, 2014-09-12 00:32:48]

On 2014/09/11 08:40:16, joleksy wrote:
> Now I set the flag only when what we load is not favicon. This fixes the
> randomness issue and seems like a logical thing to do.

Why is it a logical thing to do? I'm actually really apprehensive of the change.

Does this mean we're requesting HTTP favicons for HTTPS sites?

[Ryan Sleevi, 2014-09-12 00:48:21]

On 2014/09/12 00:32:48, Ryan Sleevi wrote:
> On 2014/09/11 08:40:16, joleksy wrote:
> > Now I set the flag only when what we load is not favicon. This fixes the
> > randomness issue and seems like a logical thing to do.
> 
> Why is it a logical thing to do? I'm actually really apprehensive of the
change.
> 
> Does this mean we're requesting HTTP favicons for HTTPS sites?

After reading through the scads of Blink code to figure out how the Favicon URL
is computed, I'm convinced that overriding for the favicon is not LGTM.

The relevant code is
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

which goes through each <link rel="icon"> element for consideration

Namely, http://www.whatwg.org/specs/web-apps/current-work/#rel-icon

Since the href may include a mixed content URL, or may include a mixed script, I
think the UI status should apply. Perhaps you could explain why not?

[joleksy, 2014-09-12 07:15:01]

On 2014/09/12 00:48:21, Ryan Sleevi wrote:
> On 2014/09/12 00:32:48, Ryan Sleevi wrote:
> > On 2014/09/11 08:40:16, joleksy wrote:
> > > Now I set the flag only when what we load is not favicon. This fixes the
> > > randomness issue and seems like a logical thing to do.
> > 
> > Why is it a logical thing to do? I'm actually really apprehensive of the
> change.
> > 
> > Does this mean we're requesting HTTP favicons for HTTPS sites?
> 
> After reading through the scads of Blink code to figure out how the Favicon
URL
> is computed, I'm convinced that overriding for the favicon is not LGTM.
> 
> The relevant code is
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> which goes through each <link rel="icon"> element for consideration
> 
> Namely, http://www.whatwg.org/specs/web-apps/current-work/#rel-icon
> 
> Since the href may include a mixed content URL, or may include a mixed script,
I
> think the UI status should apply. Perhaps you could explain why not?

The insecure content flag is set here for the main resource first
(files/ssl/page_with_refs.html in the failing test), later it is being reset in
DidNavigateMainFramePostCommit(), the comment says: "Once the main frame is
navigated, we're no longer considered to have displayed insecure content.".
My understanding may be completely incorrect here but what I thought was that
favicon should not be considered an external resource but more a part of the
main frame, and that is why the insecure content flag should not be set (as it
is not going to be reset in DidNavigateMainFramePostCommit()).

[joleksy, 2014-10-07 11:52:50]

On 2014/09/12 07:15:01, joleksy wrote:
> On 2014/09/12 00:48:21, Ryan Sleevi wrote:
> > On 2014/09/12 00:32:48, Ryan Sleevi wrote:
> > > On 2014/09/11 08:40:16, joleksy wrote:
> > > > Now I set the flag only when what we load is not favicon. This fixes the
> > > > randomness issue and seems like a logical thing to do.
> > > 
> > > Why is it a logical thing to do? I'm actually really apprehensive of the
> > change.
> > > 
> > > Does this mean we're requesting HTTP favicons for HTTPS sites?
> > 
> > After reading through the scads of Blink code to figure out how the Favicon
> URL
> > is computed, I'm convinced that overriding for the favicon is not LGTM.
> > 
> > The relevant code is
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > which goes through each <link rel="icon"> element for consideration
> > 
> > Namely, http://www.whatwg.org/specs/web-apps/current-work/#rel-icon
> > 
> > Since the href may include a mixed content URL, or may include a mixed
script,
> I
> > think the UI status should apply. Perhaps you could explain why not?
> 
> The insecure content flag is set here for the main resource first
> (files/ssl/page_with_refs.html in the failing test), later it is being reset
in
> DidNavigateMainFramePostCommit(), the comment says: "Once the main frame is
> navigated, we're no longer considered to have displayed insecure content.".
> My understanding may be completely incorrect here but what I thought was that
> favicon should not be considered an external resource but more a part of the
> main frame, and that is why the insecure content flag should not be set (as it
> is not going to be reset in DidNavigateMainFramePostCommit()).

Any progress here?
Can we or can we not consider the favicon as part of the main frame (even if it
is loaded later apparently...)?

[Ryan Sleevi, 2014-10-17 20:35:23]

On 2014/10/07 11:52:50, joleksy wrote:
> On 2014/09/12 07:15:01, joleksy wrote:
> > On 2014/09/12 00:48:21, Ryan Sleevi wrote:
> > > On 2014/09/12 00:32:48, Ryan Sleevi wrote:
> > > > On 2014/09/11 08:40:16, joleksy wrote:
> > > > > Now I set the flag only when what we load is not favicon. This fixes
the
> > > > > randomness issue and seems like a logical thing to do.
> > > > 
> > > > Why is it a logical thing to do? I'm actually really apprehensive of the
> > > change.
> > > > 
> > > > Does this mean we're requesting HTTP favicons for HTTPS sites?
> > > 
> > > After reading through the scads of Blink code to figure out how the
Favicon
> > URL
> > > is computed, I'm convinced that overriding for the favicon is not LGTM.
> > > 
> > > The relevant code is
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > which goes through each <link rel="icon"> element for consideration
> > > 
> > > Namely, http://www.whatwg.org/specs/web-apps/current-work/#rel-icon
> > > 
> > > Since the href may include a mixed content URL, or may include a mixed
> script,
> > I
> > > think the UI status should apply. Perhaps you could explain why not?
> > 
> > The insecure content flag is set here for the main resource first
> > (files/ssl/page_with_refs.html in the failing test), later it is being reset
> in
> > DidNavigateMainFramePostCommit(), the comment says: "Once the main frame is
> > navigated, we're no longer considered to have displayed insecure content.".
> > My understanding may be completely incorrect here but what I thought was
that
> > favicon should not be considered an external resource but more a part of the
> > main frame, and that is why the insecure content flag should not be set (as
it
> > is not going to be reset in DidNavigateMainFramePostCommit()).
> 
> Any progress here?
> Can we or can we not consider the favicon as part of the main frame (even if
it
> is loaded later apparently...)?

Sorry for the delay, I've been on the road and then sick. I think this is a
question for abarth, and you should potentially ping him directly, as this is an
old CL that he may not be watching. Mike West may also have ideas.

I do feel that the favicon - or more aptly/especially, resources with link-rel,
should be constrained by the mixed content checks, and I don't think ignoring
the favicon is acceptable. Adam or Mike may have further guidance on the next
steps on how to harmonize the states, or if I'm completely wrong.

[joleksy, 2014-11-12 08:30:40]

On 2014/10/17 20:35:23, Ryan Sleevi (OOO until 11-18) wrote:
> On 2014/10/07 11:52:50, joleksy wrote:
> > On 2014/09/12 07:15:01, joleksy wrote:
> > > On 2014/09/12 00:48:21, Ryan Sleevi wrote:
> > > > On 2014/09/12 00:32:48, Ryan Sleevi wrote:
> > > > > On 2014/09/11 08:40:16, joleksy wrote:
> > > > > > Now I set the flag only when what we load is not favicon. This fixes
> the
> > > > > > randomness issue and seems like a logical thing to do.
> > > > > 
> > > > > Why is it a logical thing to do? I'm actually really apprehensive of
the
> > > > change.
> > > > > 
> > > > > Does this mean we're requesting HTTP favicons for HTTPS sites?
> > > > 
> > > > After reading through the scads of Blink code to figure out how the
> Favicon
> > > URL
> > > > is computed, I'm convinced that overriding for the favicon is not LGTM.
> > > > 
> > > > The relevant code is
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > > 
> > > > which goes through each <link rel="icon"> element for consideration
> > > > 
> > > > Namely, http://www.whatwg.org/specs/web-apps/current-work/#rel-icon
> > > > 
> > > > Since the href may include a mixed content URL, or may include a mixed
> > script,
> > > I
> > > > think the UI status should apply. Perhaps you could explain why not?
> > > 
> > > The insecure content flag is set here for the main resource first
> > > (files/ssl/page_with_refs.html in the failing test), later it is being
reset
> > in
> > > DidNavigateMainFramePostCommit(), the comment says: "Once the main frame
is
> > > navigated, we're no longer considered to have displayed insecure
content.".
> > > My understanding may be completely incorrect here but what I thought was
> that
> > > favicon should not be considered an external resource but more a part of
the
> > > main frame, and that is why the insecure content flag should not be set
(as
> it
> > > is not going to be reset in DidNavigateMainFramePostCommit()).
> > 
> > Any progress here?
> > Can we or can we not consider the favicon as part of the main frame (even if
> it
> > is loaded later apparently...)?
> 
> Sorry for the delay, I've been on the road and then sick. I think this is a
> question for abarth, and you should potentially ping him directly, as this is
an
> old CL that he may not be watching. Mike West may also have ideas.
> 
> I do feel that the favicon - or more aptly/especially, resources with
link-rel,
> should be constrained by the mixed content checks, and I don't think ignoring
> the favicon is acceptable. Adam or Mike may have further guidance on the next
> steps on how to harmonize the states, or if I'm completely wrong.

Sorry, I have been away for some time.
What seems to be happening here is that favicon is loaded after
DidNavigateMainFramePostCommit resets insecure content flag.
If we are to consider favicon in the mixed content check then probably some more
sophisticated solution would be needed.

[Mike West, 2014-11-12 12:22:24]

On 2014/11/12 08:30:40, joleksy wrote:
> Sorry, I have been away for some time.
> What seems to be happening here is that favicon is loaded after
> DidNavigateMainFramePostCommit resets insecure content flag.
> If we are to consider favicon in the mixed content check then probably some
more
> sophisticated solution would be needed.

Given that the favicon is associated with the page, it's not clear to me why
it's a good idea to avoid the mixed content (or CSP) checks. Note also that the
spec includes the `favicon` context as "blockable content":
https://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable. We
should really be blocking the load entirely (which would have the nice
side-effect of restoring the page's secure UI, as the content would be blocked
rather than displayed).

[joleksy, 2014-12-11 08:27:59]

On 2014/11/12 12:22:24, Mike West (sick) wrote:
> On 2014/11/12 08:30:40, joleksy wrote:
> > Sorry, I have been away for some time.
> > What seems to be happening here is that favicon is loaded after
> > DidNavigateMainFramePostCommit resets insecure content flag.
> > If we are to consider favicon in the mixed content check then probably some
> more
> > sophisticated solution would be needed.
> 
> Given that the favicon is associated with the page, it's not clear to me why
> it's a good idea to avoid the mixed content (or CSP) checks. Note also that
the
> spec includes the `favicon` context as "blockable content":
> https://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable. We
> should really be blocking the load entirely (which would have the nice
> side-effect of restoring the page's secure UI, as the content would be blocked
> rather than displayed).

Right. (Again, sorry for delay).

So, the problem as I see it comes from the fact that the security status is
being kept in a global flag, the flag is being reset after the whole page loads
(in DidNavigateMainFramePostCommit).
The time when favicon is loaded is not totally deterministic (correct me if I am
wrong), it can occur before or after DidNavigateMainFramePostCommit. If it
occurs after DidNavigateMainFramePostCommit (and after resetting the flag), it
sets the flag, but DidNavigateMainFramePostCommit never comes afterwards and the
flag is not reset, making the favicon an unsafe resource.

I am not sure how we should approach this, any tips?

Maybe a separate flag for each host would help?

[joleksy, 2015-02-27 14:24:14]

On 2014/12/11 08:27:59, joleksy wrote:
> On 2014/11/12 12:22:24, Mike West (sick) wrote:
> > On 2014/11/12 08:30:40, joleksy wrote:
> > > Sorry, I have been away for some time.
> > > What seems to be happening here is that favicon is loaded after
> > > DidNavigateMainFramePostCommit resets insecure content flag.
> > > If we are to consider favicon in the mixed content check then probably
some
> > more
> > > sophisticated solution would be needed.
> > 
> > Given that the favicon is associated with the page, it's not clear to me why
> > it's a good idea to avoid the mixed content (or CSP) checks. Note also that
> the
> > spec includes the `favicon` context as "blockable content":
> > https://w3c.github.io/webappsec/specs/mixedcontent/#category-blockable. We
> > should really be blocking the load entirely (which would have the nice
> > side-effect of restoring the page's secure UI, as the content would be
blocked
> > rather than displayed).
> 
> Right. (Again, sorry for delay).
> 
> So, the problem as I see it comes from the fact that the security status is
> being kept in a global flag, the flag is being reset after the whole page
loads
> (in DidNavigateMainFramePostCommit).
> The time when favicon is loaded is not totally deterministic (correct me if I
am
> wrong), it can occur before or after DidNavigateMainFramePostCommit. If it
> occurs after DidNavigateMainFramePostCommit (and after resetting the flag), it
> sets the flag, but DidNavigateMainFramePostCommit never comes afterwards and
the
> flag is not reset, making the favicon an unsafe resource.
> 
> I am not sure how we should approach this, any tips?
> 
> Maybe a separate flag for each host would help?

PING?