Last Comment Bug 837959 - Cached media content not calling Content Policies
: Cached media content not calling Content Policies
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: reuseme2600
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
https://www.mozilla.org/en-US/firefox...
Depends on:
Blocks: MixedContentBlocker
  Show dependency treegraph
 
Reported: 2013-02-04 16:51 PST by Tanvi Vyas [:tanvi]
Modified: 2013-02-09 07:51 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move content policy check earlier v1 (3.13 KB, patch)
2013-02-06 18:34 PST, Tanvi Vyas [:tanvi]
roc: review+
Details | Diff | Splinter Review

Description Tanvi Vyas [:tanvi] 2013-02-04 16:51:15 PST
https://www.mozilla.org/en-US/firefox/fx/ has mixed content:

[10:28:31.358] GET http://videos.mozilla.org/serv/marketing/Firefox/Firefox%20Tabs_1.webm [HTTP/1.1 206 Partial Content 1970ms]

With security.mixed_content.block_display set to true, we sometimes still see the globe icon instead of the lock.  I think this is related to a caching issue, where the cached video request does not go through the content policy shouldLoad() but some how nsSecureBrowserUIImpl still catches it.
Comment 1 Tanvi Vyas [:tanvi] 2013-02-06 17:04:50 PST
The psm code ends up here and sets the 

nsSecureBrowserUIImpl::OnStateChange, we call 
nsCOMPtr<nsISupports> securityInfo(ExtractSecurityInfo(aRequest));

to get the securityInfo (https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#777).  When we have an http connection, there is no security info when we get down to line 1273 and call

UpdateSubrequestMembers(securityInfo);

UpdateSubrequestMembers calls GetSecurityStateFromSecurityInfo(securityInfo)
https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#573

Which tries to get the psmInfo from the securityInfo.  Since there is no securityInfo, there is no psmInfo and we return nsIWebProgressListener::STATE_IS_INSECURE;

When we do this, we fall into the else case here https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#589 and increment mSubRequestsNoSecurity.  Which later ends up setting lis_mixed_securty, and hence STATE_IS_BROKEN.
https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#1339

But still don't know why nsMixedContentBlocker.cpp doesn't catch this.  It's caught cached content before.
Comment 2 Tanvi Vyas [:tanvi] 2013-02-06 17:29:19 PST
Backtrace.  This may be an issue in the code for media (nsHTMLMediaElement, for example).  Maybe it's missing a call to shouldLoad in the mDownloadSuspendedByCache case.

http://www.pastebin.mozilla.org/2120245
Comment 3 Tanvi Vyas [:tanvi] 2013-02-06 17:54:55 PST
In nsHTMLMediaElement.cpp, the content policies are checked in LoadResource (NS_CheckContentLoadPolicy()).  LoadResource is called from the following functions:

SelectResource()
LoadFromSourceChildren()
ResumeLoad()

Putting a break point at the beginning of LoadResource() and visiting https://www.mozilla.org/en-US/firefox/fx/ (after you have visited it recently so it is cached) shows that we return at line 1085, before the content policies are called.

Breakpoint 12, nsHTMLMediaElement::LoadResource (this=0x1218b6c00) at /Users/tvyas/dev/mozilla-central/content/html/content/src/nsHTMLMediaElement.cpp:1064
1064	  NS_ASSERTION(mDelayingLoadEvent,
(gdb) n
1067	  if (mChannel) {
(gdb) 
1073	  mCORSMode = AttrValueToCORSMode(GetParsedAttr(nsGkAtoms::crossorigin));
(gdb) 
1075	  nsHTMLMediaElement* other = LookupMediaElementURITable(mLoadingSrc);
(gdb) 
1076	  if (other && other->mDecoder) {
(gdb) 
1078	    nsresult rv = InitializeDecoderAsClone(other->mDecoder);
(gdb) 
1083	    mMimeType = other->mMimeType;
(gdb) 
1084	    if (NS_SUCCEEDED(rv))
(gdb) 
1085	      return rv;
(gdb) 
0x000000010204ef24	1194	}

This occurs when I open a new tab or reload or revisit the page on the same tab it was opened previously.

When I go to about:config and disable mixed display content, the media is still loaded and I get the globe instead of the green lock.
Comment 4 Tanvi Vyas [:tanvi] 2013-02-06 18:07:57 PST
It's not exactly cached media, but its media stored in memory via a custom cache mechanism.

https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#1723

This should be solved by moving the content policy calls from line 1088-1101 to before line 1075.
Comment 5 Tanvi Vyas [:tanvi] 2013-02-06 18:11:11 PST
Are there any other instances of a custom cache we should double check.  Adding Joe in case images also have some custom memory cache that doesn't go through the content policies.  What about fonts?
Comment 6 Tanvi Vyas [:tanvi] 2013-02-06 18:34:32 PST
Created attachment 711127 [details] [diff] [review]
Move content policy check earlier v1

Here is a proposed patch that fixes the problem, but I'm not familiar enough with the media code to know if this could potentially break something.
Comment 7 Jonathan Kew (:jfkthame) 2013-02-07 01:42:58 PST
(In reply to Tanvi Vyas [:tanvi] from comment #5)
> Are there any other instances of a custom cache we should double check. 
> Adding Joe in case images also have some custom memory cache that doesn't go
> through the content policies.  What about fonts?

There's a custom cache for @font-face downloaded fonts (see gfxUserFontSet::UserFontCache), so that multiple pages using the same font resource can share the activated font, and even the shaped glyph data. Here's where we look for things in that cache:

  http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.cpp#513

The intention was that this cache is checked only -after- we know we're allowed to load the resource in question, by calling nsUserFontSet::CheckFontLoad:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.cpp#805

which in turn calls nsFontFaceLoader::CheckLoadAllowed:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.cpp#252

This includes a content-policy check. It looks like this stuff comes from bug 441473. I don't really know that aspect of the code, but AFAICS the addition of the cache shouldn't have had any effect on the behavior.
Comment 8 Honza Bambas (:mayhemer) 2013-02-07 08:25:23 PST
Is the loadgroup aware of all of these loads?

It seems to me like CSP APIs are either broken (it's sad we have to call this important API "manually" when there is some kind of a custom caching, I'd expect there would be a single point) or mixed content detection is simply based on wrong technology (what I was a long time ago afraid of...)
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-07 15:28:39 PST
(In reply to Honza Bambas (:mayhemer) from comment #8)
> Is the loadgroup aware of all of these loads?

No.

Images, fonts, and media are all (independently) doing their own URL matching and sharing resources whenever URLs match (ok, there are some other checks, such as when CORS is used we also ensure the principal of the loader matches). The HTML5 spec says we can do this.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-07 15:29:48 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> such as when CORS is used we also ensure the principal of the loader
> matches). The HTML5 spec says we can do this.

Er, I lied, we don't do that, but maybe we should.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-07 15:30:10 PST
Comment on attachment 711127 [details] [diff] [review]
Move content policy check earlier v1

Review of attachment 711127 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, anyway.
Comment 12 Tanvi Vyas [:tanvi] 2013-02-07 16:05:24 PST
Pushed to try -
https://tbpl.mozilla.org/?tree=Try&rev=62a5ff1c0cef

If it looks good, I will land it.
Comment 13 Tanvi Vyas [:tanvi] 2013-02-08 11:56:07 PST
Try looks good.  Pushed to inbound - http://hg.mozilla.org/integration/mozilla-inbound/rev/476d3de99c9c
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-02-09 07:51:55 PST
https://hg.mozilla.org/mozilla-central/rev/476d3de99c9c

Note You need to log in before you can comment on or make changes to this bug.