1264231 - fix loadInfo-loadContext mismatch

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Dragana Damjanovic [:dragana]]

Bug 1125916 has a work around for a loadInfo-loadContext mismatch in channels created by imgLoader.
This bug should find a real fix.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: inherit OA from docshell.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: ignore moz-icon when checking loadInfo.

This is like Bug 1266022, I assume moz-icon is only used by XUL(?), so ignore the check for loadinfo.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: remove loadInfo work-around

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=744198ca9e30

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8775507 [details] [diff] [review]
Part 1: inherit OA from docshell.

Hi Jonas
This should be the mistake I made in Bug 1263496 (Part 3).
I found in those tests under dom/browser-element, the origin attributes of those data:text/html tests are inherited from the owner, (for example, owner will be http://mochitest:8888/.../dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html)

but they should be inherited from the docshell of current iframe(mozbrowser frame)

So I've fixed to inherit from docshell,

Could you review this for me ?

Thanks

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8775509 [details] [diff] [review]
Part 2: ignore moz-icon when checking loadInfo.

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

Should we also check for blob URIs? blob:null-uuid

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Jonas
There is one remaing problem I am still not sure,
what's the right way to fix loading image that uses a SystemPrincipal?

For example, the test mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1125916#c89
test_focus_blur_manage_events.html (currently it's disabled now), the loadingPrincipal is SystemPrincipal,
and same case for the about:newtab to load a thumbnail image 

Can you provide some suggestions?

Thanks

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: override loadInfo's origin attributes in imgLoader

I tried to override the origin attributes of the loadInfo when doing image load.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8776529 [details] [diff] [review]
Part 4: override loadInfo's origin attributes in imgLoader

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

Why is this needed? Isn't the purpose of this bug exactly to remove this?

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8776529 [details] [diff] [review]
Part 4: override loadInfo's origin attributes in imgLoader

yeah, probably a bad idea to move it back.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Jonas
I still have questions regarding to loading image.

Take the test ./dom/inputmethod/mochitest/test_focus_blur_manage_events.html for instance as you mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1125916#c89

In this test it will create a mozbrowser frame to load another html
http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/dom/inputmethod/mochitest/test_focus_blur_manage_events.html#170

At this time, nsDocShell::DoURILoad will try to load the test file, also set the origin attributes on the loadInfo.

http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/docshell/base/nsDocShell.cpp#10841

since the docshell now is for mozbrowser, so it will have the isIsolatedMozBrowser flag is true.

And since the path of the test file is chrome://mochitests/content/chrome/dom/inputmethod/mochitest/file_test_focus_blur_manage_events.html, so the owner of the channel will be the System Principal.
http://searchfox.org/mozilla-central/rev/3df383b8552c1f8059f5c21258388ddb5a2f33d0/chrome/nsChromeProtocolHandler.cpp#194

And this principal(or owner) will in turn become the NodePrincipal of this document, which in turn will become the loadingPrincipal of the following sub-resources requests.

So the problem I saw is, for the channel to load the test file(file_test_focus_blur_manage_events.html), its loadInfo has isIsolatedMozBrowser flag set, however its owner is System Principal, which doesn't have the mozbrowser flag, and we use that SystemPrincipal to load sub-resources, it looks that we didn't propagate the origin attributes to the sub-resource loading (for example, for the top-level document load, we have a loadInfo with correct origin attributes, but not for those loadInfos of sub-resource loading), 

Should the loadInfos of sub-resources 'inherit' the origin attributes?

Or should I ignore the check of calling NS_CompareLoadInfoAndLoadContext when the loadingPrincipal is SystemPrincipal?

could you kindly help to share some comments on this? :D

Thanks

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't understand comment 12.

First off, the searchfox link appears to be wrong since it's pointing at error handling code and not something that's related to OA or loadInfo.

Second, I'm not sure what "owner" you are referring to. Are you talking about the "owner" in docshell? Docshell generally loads html/xul pages, but the assertion we are hitting is when imglib is loading an image. Are you saying that the testcase is loading an image into a docshell, rather than loading a HTML/XUL page?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think the test_focus_blur_manage_events.html test doesn't actually want isolation. The best way to fix the assertion there is likely to fix bug 1254823.

The other situation is about:newtab. about:newtab runs with system privileges, so the document there has a system principal, but runs in a docshell which might have a userContextId set.

For reasons that I think are off-topic to this bug, about:newtab is not possible to fix without making bigger changes.

So what I think we should do in this bug is to avoid the assertion when the loadingDocument is a about:newtab page.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: skip checking for about:newtab

Hi Jonas,
This patch prevent checking loadInfo for about:newtab,
I still keep the moz-icon, for it's needed by about:sync-tabs, and it's not related to mozbrowser so I think it doesn't related to bug 1254823.

If you still have time, could you review for me?

Thanks

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8777284 [details] [diff] [review]
Part 4: skip checking for about:newtab

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

Hi Tanvi
It looks Jonas can't review this. :(
Could you review this?

So this part fixed Jonas' comment 14, avoid the asserion when the loadingDocument is about:newtab.
However I found in nsBaseChannel.cpp there's no documentURI, so I use scheme.EqualsLiteral("blob") && isSystemPrinicipal to check this.
(the blob url is used to load some icons in about:newtab)

Thanks

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8777284 [details] [diff] [review]
Part 4: skip checking for about:newtab

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

Jonas said he could finish this. :D

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8777284 [details] [diff] [review]
Part 4: skip checking for about:newtab

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

::: netwerk/base/nsBaseChannel.cpp
@@ +655,5 @@
>    mURI->GetScheme(scheme);
> +  bool isSystem = nsContentUtils::IsSystemPrincipal(mLoadInfo->LoadingPrincipal());
> +  if (!(scheme.EqualsLiteral("file") ||
> +        scheme.EqualsLiteral("moz-icon") ||
> +        (scheme.EqualsLiteral("blob") && isSystem))) {

I don't think we should skip blob. Why is that needed?


Rather than excluding moz-icon and blob, I think we should do the same as in httpchannel and look at the document uri, which you can get from mLoadInfo->LoadingNode()->OwnerDoc()->GetDocumentURI() (or some such, with nullchecks of course).

Then exclude abotu:newtab and about:sync-tabs

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8777284 [details] [diff] [review]
Part 4: skip checking for about:newtab

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

Another thing that would be a good idea is to *always* assert that for http requests, that mPrivateBrowsingId in the two OAs match. That way we know that we're not affecting private browsing. We should assert that no matter if the request is coming from about:newtab or about:sync-tabs or if it's a system principal which does the loading.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: skip checking for about:newtab and about:sync-tabs

merged part 2 and part 4 together

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: compare privateBrowsingId for HTTP channel

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8777675 [details] [diff] [review]
Part 2: skip checking for about:newtab and about:sync-tabs

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

It would probably be cleaner to move this into NS_CompareLoadInfoAndLoadContext, so it doesn't have to be duplicated in two places. But given that these assertions are likely going away soon, I think this is ok.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8777676 [details] [diff] [review]
Part 4: compare privateBrowsingId for HTTP channel

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

Since there's just one caller of this function. I'd say just make the function static and local to nsHttpChannel.cpp. No need to treat this as a function that is part of the necko API.

Or even inline at the callsite. Up to you.

r=me with that fixed.

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: skip checking for about:newtab and about:sync-tabs v3

moved those checks inside NS_CompareLoadInfoAndLoadContext.

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: compare privateBrowsingId for HTTP channel v2

check in the call site

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: add a TODO in test.

Hi Kanru
See the commit message for the detail.

Thanks

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

The part 4 patch (compare mPrivateBrowsingId between LoadInfo and LoadContext) will trigger asserion on try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7480f005031c&selectedJob=25092619
I am checking this.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 6: sync privateBrowsing in LoadInfo.

Hi jdm
This should be some leftovers in Bug 1269231,
could you review this for me?

Thanks

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8778064 [details] [diff] [review]
Part 6: sync privateBrowsing in LoadInfo.

wait, I still found some test failed.

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I think PrivateBrowsing has a bigger problem, so I'll file another bug to fix PB.

The problem I saw that is the origin attributes of the loadInfo and LoadContext are already wrong, they don't have the mPrivateBrowsingId set(mPrivateBrowsingId is 0), however the loadInfo->GetUsePrivateBrowsing() and loadContext->GetUserPrivateBrowsing() are both true.

And somehow after IPC, I guess somewhere calls attrs.SyncAttributesWithPrivateBrowsing to update the loadContext's origin attributes, so we'll have an assertion on the parent side.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2dbbb2abaef
Part 1: inherit OA from docshell. r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a4bb4e923c
Part 2: skip checking for about:newtab and about:sync-tabs r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6b262acd1b3
Part 3: remove loadInfo work-around. r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d487ebf9af
Part 4: add a TODO in test. r=kanru

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c2dbbb2abaef
https://hg.mozilla.org/mozilla-central/rev/b1a4bb4e923c
https://hg.mozilla.org/mozilla-central/rev/a6b262acd1b3
https://hg.mozilla.org/mozilla-central/rev/46d487ebf9af