fix loadInfo-loadContext mismatch

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: dragana, Unassigned)

Tracking

(Depends on: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [domsecurity-active][OA])

Attachments

(4 attachments, 7 obsolete attachments)

1.15 KB, patch
sicking
: review+
Details | Diff | Splinter Review
2.78 KB, patch
sicking
: review+
Details | Diff | Splinter Review
6.64 KB, patch
allstars
: review+
Details | Diff | Splinter Review
1.46 KB, patch
kanru
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Bug 1125916 has a work around for a loadInfo-loadContext mismatch in channels created by imgLoader.
This bug should find a real fix.
Whiteboard: [domsecurity-backlog]
(Assignee)

Updated

2 years ago
See Also: → bug 1266022
Priority: -- → P1

Updated

2 years ago
Whiteboard: [domsecurity-backlog] → [domsecurity-backlog][OA]
(Assignee)

Updated

2 years ago
Assignee: nobody → allstars.chh
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1264230
(Assignee)

Updated

2 years ago
Summary: imgLoader loadInfo-loadContext mismatch → fix loadInfo-loadContext mismatch
Created attachment 8775507 [details] [diff] [review]
Part 1: inherit OA from docshell.
Created attachment 8775509 [details] [diff] [review]
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.
Created attachment 8775511 [details] [diff] [review]
Part 3: remove loadInfo work-around
(Assignee)

Updated

2 years ago
Attachment #8775511 - Attachment description: Bug 1264231 - Part 3: remove loadInfo work-around → Part 3: remove loadInfo work-around
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
Attachment #8775507 - Flags: review?(jonas)
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
Attachment #8775509 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Attachment #8775511 - Flags: review?(jonas)
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
Whiteboard: [domsecurity-backlog][OA] → [domsecurity-active][OA]
Created attachment 8776529 [details] [diff] [review]
Part 4: override loadInfo's origin attributes in imgLoader

I tried to override the origin attributes of the loadInfo when doing image load.
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 on attachment 8776529 [details] [diff] [review]
Part 4: override loadInfo's origin attributes in imgLoader

yeah, probably a bad idea to move it back.
Attachment #8776529 - Attachment is obsolete: true
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
(Assignee)

Updated

2 years ago
Flags: needinfo?(jonas)
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?
Flags: needinfo?(jonas)
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.

Updated

2 years ago
Blocks: 1260931
Created attachment 8777284 [details] [diff] [review]
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
Attachment #8777284 - Flags: review?(jonas)
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
Attachment #8777284 - Flags: review?(tanvi)
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
Attachment #8777284 - Flags: review?(tanvi)
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
Attachment #8777284 - Flags: review?(jonas) → review-
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.
Created attachment 8777675 [details] [diff] [review]
Part 2: skip checking for about:newtab and about:sync-tabs

merged part 2 and part 4 together
Attachment #8775509 - Attachment is obsolete: true
Attachment #8777284 - Attachment is obsolete: true
Created attachment 8777676 [details] [diff] [review]
Part 4: compare privateBrowsingId for HTTP channel
(Assignee)

Updated

2 years ago
Attachment #8777675 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Attachment #8777676 - Flags: review?(jonas)
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.
Attachment #8777675 - Flags: review?(jonas) → review+
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.
Attachment #8777676 - Flags: review?(jonas) → review+
Created attachment 8777718 [details] [diff] [review]
Part 2: skip checking for about:newtab and about:sync-tabs v3

moved those checks inside NS_CompareLoadInfoAndLoadContext.
Attachment #8777675 - Attachment is obsolete: true
Attachment #8777718 - Flags: review+
Created attachment 8777719 [details] [diff] [review]
Part 4: compare privateBrowsingId for HTTP channel v2

check in the call site
Attachment #8777676 - Attachment is obsolete: true
Attachment #8777719 - Flags: review+
Created attachment 8777720 [details] [diff] [review]
Part 4: add a TODO in test.

Hi Kanru
See the commit message for the detail.

Thanks
Attachment #8777720 - Flags: review?(kchen)
Attachment #8777720 - Flags: review?(kchen) → review+
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.
Created attachment 8778064 [details] [diff] [review]
Part 6: sync privateBrowsing in LoadInfo.

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

Thanks
Attachment #8778064 - Flags: review?(josh)
Comment on attachment 8778064 [details] [diff] [review]
Part 6: sync privateBrowsing in LoadInfo.

wait, I still found some test failed.
Attachment #8778064 - Flags: review?(josh)
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.
(Assignee)

Updated

2 years ago
Attachment #8777719 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8778064 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8777720 - Attachment description: Part 5: add a TODO in test. → Part 4: add a TODO in test.

Comment 31

2 years ago
Pushed by yhuang@mozilla.com:
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

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1308938
You need to log in before you can comment on or make changes to this bug.