Resources restored from CSS cache should be able to trigger InsecureContent messages

Resources restored from CSS cache should be able to trigger InsecureContent messages

After a page reload resources such as an Image or a Font could be loaded
directly from CSS cache and browser may lose information about mixed content
so ViewHostMsg_DidDisplayInsecureContent or ViewHostMsg_DidRunInsecureContent
messages should be sent.

BUG=393572
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183103

[mmal, 2014-08-19 12:38:15]

Hi, guys whom do I need to add to this review?

[philipj_slow, 2014-08-19 13:16:46]

Mike, can you PTAL?

[Mike West, 2014-08-19 13:34:13]

Thanks for taking a look at this! Two questions:

https://codereview.chromium.org/439223005/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/439223005/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:1194: void
ResourceFetcher::maybeNotifyInsecureContent(const Resource* resource) const
1. Why break this one-line call out into a separate function if it's only being
used in one place?

2. If we cache `http://example.com/script.js`, and then load it on
`https://example.com/`, the script should be blocked as active content. This
code would just write an error message to the console, and either add a shield
to the omnibox or break the HTTPS on the page. It wouldn't actually block the
resource load. Perhaps we should do so?

[Mike West, 2014-08-19 13:49:36]

On 2014/08/19 13:34:13, Mike West wrote:
> 2. If we cache `http://example.com/script.js`, and then load it on
> `https://example.com/`, the script should be blocked as active content. This
> code would just write an error message to the console, and either add a shield
> to the omnibox or break the HTTPS on the page. It wouldn't actually block the
> resource load. Perhaps we should do so?

After looking at a bit more of the code, let me rephrase this question in a way
that makes sense:

We block HTTP fonts on HTTPS pages as mixed content, treating them exactly like
scripts. I'd like to get to a point where we can do the same with images (though
that's going to take a long while). Is there a good way to wire up these checks
_before_ we decide to use the resource from the cache?

[João Eiras, 2014-08-19 14:04:39]

On 2014/08/19 13:49:36, Mike West wrote:
> On 2014/08/19 13:34:13, Mike West wrote:
> > 2. If we cache `http://example.com/script.js`, and then load it on
> > `https://example.com/`, the script should be blocked as active content. This
> > code would just write an error message to the console, and either add a
shield
> > to the omnibox or break the HTTPS on the page. It wouldn't actually block
the
> > resource load. Perhaps we should do so?
> 
> After looking at a bit more of the code, let me rephrase this question in a
way
> that makes sense:
> 
> We block HTTP fonts on HTTPS pages as mixed content, treating them exactly
like
> scripts. I'd like to get to a point where we can do the same with images
(though
> that's going to take a long while). Is there a good way to wire up these
checks
> _before_ we decide to use the resource from the cache?

I think CSP checks are done way before the code reaches
notifyLoadedFromMemoryCache(), and doing that change of policy (blocking
insecure scripts) is something much more political than technical, as you very
well know, which would require to go through the whole process of approving a
new feature.

This review is solely in regards to notifying the UI that there *is* mixed
content being rendered, so effectively just a bug fix. Note that those messages
of there being mixed content are already sent when resources are fetched from
the network (according to the commit description).

[Mike West, 2014-08-19 14:14:42]

On 2014/08/19 14:04:39, João Eiras wrote:
> On 2014/08/19 13:49:36, Mike West wrote:
> > On 2014/08/19 13:34:13, Mike West wrote:
> > > 2. If we cache `http://example.com/script.js`, and then load it on
> > > `https://example.com/`, the script should be blocked as active content.
This
> > > code would just write an error message to the console, and either add a
> shield
> > > to the omnibox or break the HTTPS on the page. It wouldn't actually block
> the
> > > resource load. Perhaps we should do so?
> > 
> > After looking at a bit more of the code, let me rephrase this question in a
> way
> > that makes sense:
> > 
> > We block HTTP fonts on HTTPS pages as mixed content, treating them exactly
> like
> > scripts. I'd like to get to a point where we can do the same with images
> (though
> > that's going to take a long while). Is there a good way to wire up these
> checks
> > _before_ we decide to use the resource from the cache?
> 
> I think CSP checks are done way before the code reaches
> notifyLoadedFromMemoryCache(), and doing that change of policy (blocking
> insecure scripts) is something much more political than technical, as you very
> well know, which would require to go through the whole process of approving a
> new feature.

I'm not proposing that we change policy, I'm suggesting that the same rules
should apply to resources read from the cache as to resources loaded over the
network. That is, the fact that I loaded a font on http://page1 shouldn't have
any bearing on whether or not it is blocked when requested from https://page2.

> This review is solely in regards to notifying the UI that there *is* mixed
> content being rendered, so effectively just a bug fix. Note that those
messages
> of there being mixed content are already sent when resources are fetched from
> the network (according to the commit description).

The context matters: if I load a resource on an insecure page, it's cached. If I
then request the same resource on a secure page, we shouldn't bypass the
security checks. Blocking the request is part of bug we should be fixing, not
just ensuring that the UI treatment is correct.

[mmal, 2014-08-19 14:54:06]

1) I enclosed checkInsecureContent in a new function to emphasize that it
doesn't make a real check.

2) I believe restoreCachedResourceIfNeeded functions are invoked only for page
reloads so changing canRequest function should have expected effect on images
and fonts as well.

One drawback I think of is that the fix is affecting other cache requests -
ViewHostMsg_Did*InsecureContent messages will be sent twice.

[mmal, 2014-09-08 13:42:02]

Hello, any progress with reviewing?

[Mike West, 2014-09-08 15:05:04]

Please add a test to verify the functionality. You could check whether the error
message is written to the console after reloading the contents of an iframe, for
instance.

I'm still concerned that this is just masking the problem, as it doesn't provide
any mechanism for blocking use of cached insecure resources in secure contexts.
We block mixed font requests, for instance. I'd like to ensure that
`http://font.com/font.woff` doesn't load on `https://bank.com/` even if it _did_
load on `http://insecure.in/`. I won't block landing this patch on that, since
the warning is certainly better than status quo, but I think there's more to be
done than this patch suggests.

[mmal, 2014-09-08 16:17:45]

On 2014/09/08 15:05:04, Mike West wrote:
> I'm still concerned that this is just masking the problem, as it doesn't
provide
> any mechanism for blocking use of cached insecure resources in secure
contexts.
> We block mixed font requests, for instance. I'd like to ensure that
> `http://font.com/font.woff` doesn't load on `https://bank.com/` even if it
_did_
> load on `http://insecure.in/`. I won't block landing this patch on that, since
> the warning is certainly better than status quo, but I think there's more to
be
> done than this patch suggests.

Now I see that actually there is no need to fix
ViewHostMsg_DidRunInsecureContent because of how ChromeSSLHostStateDelegate
works, once insecure content is ran it is remembered until browser shutdown
(BTW: very small memory leak in ran_insecure_content_hosts_ set, it can only
grow). But we still might want to send ViewHostMsg_DidRunInsecureContent just to
be consistent.

So only real problem is when flow goes through
CSSImageValue::restoreCachedResourceIfNeeded and then directly to
ResourceFetcher::requestLoadStarted which I think can happen only when page is
refreshing and image has been loaded in previous request. So any potential harm
is already done anyway (but I can't see any issue of this kind here) and since
we load image from cache no additional requests are made.

[mmal, 2014-09-12 16:09:22]

Patchset #2 (id:20001) has been deleted

[mmal, 2014-09-12 16:33:22]

Test added, little code changes.

[Mike West, 2014-09-12 18:54:29]

LGTM with one change, as I've been poking at the mixed content object. Sorry for
the overlap.

https://codereview.chromium.org/439223005/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/439223005/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:683:
checkInsecureContent(resource->type(), resource->lastResourceRequest().url(),
resource->options().mixedContentBlockingTreatment);
Sorry! I changed this in a patch that landed just a little while ago. You'll
want to run the static method `MixedContentChecker::shouldBlockFetch(frame(),
resource->lastResourceRequest().requestContext(),
resource>lastResourceRequest().frameType(),
resource->lastResourceRequest().url());` instead of `checkInsecureContent` (it's
been deleted :) ).

[mmal, 2014-09-12 19:31:31]

The CQ bit was checked by mmaliszkiewicz@opera.com

[commit-bot: I haz the power, 2014-09-12 19:32:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/439223005/100001

[commit-bot: I haz the power, 2014-09-12 21:32:49]

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

[commit-bot: I haz the power, 2014-09-12 21:32:50]

Exceeded time limit waiting for builds to trigger.

[mmal, 2014-09-15 10:18:57]

The CQ bit was checked by mmaliszkiewicz@opera.com

[commit-bot: I haz the power, 2014-09-15 10:19:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/439223005/100001

[commit-bot: I haz the power, 2014-09-15 12:20:23]

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

[commit-bot: I haz the power, 2014-09-15 12:20:24]

Exceeded time limit waiting for builds to trigger.

[mmal, 2014-09-16 12:22:36]

The CQ bit was checked by mmaliszkiewicz@opera.com

[commit-bot: I haz the power, 2014-09-16 12:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/439223005/100001

[commit-bot: I haz the power, 2014-09-16 13:20:56]

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

[commit-bot: I haz the power, 2014-09-16 13:20:58]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (None)

[mmal, 2014-09-16 14:38:22]

@Mike I think something is wrong after recent changes in mixed content area.
When you load test:
https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-cs...
in console there is  an error about font but no warning about insecure image.

Also in my calls to shouldBlockFetch I have requestContext set to
RequestContextInternal.

[mmal, 2014-10-01 13:01:00]

The CQ bit was checked by mmaliszkiewicz@opera.com

[commit-bot: I haz the power, 2014-10-01 13:02:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/439223005/100001

[commit-bot: I haz the power, 2014-10-01 13:22:18]

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

[commit-bot: I haz the power, 2014-10-01 13:22:20]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/23530)

[mmal, 2014-10-01 18:07:53]

Patchset #6 (id:120001) has been deleted

[mmal, 2014-10-01 18:35:05]

The CQ bit was checked by mmaliszkiewicz@opera.com

[mmal, 2014-10-01 19:12:10]

CL updated to match current code. It should integrate correctly after recent
mixed content security patch (I hope).

[commit-bot: I haz the power, 2014-10-01 19:38:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/439223005/160001

[commit-bot: I haz the power, 2014-10-01 23:39:36]

Committed patchset #7 (id:160001) as 183103