Open
Bug 1240258
Opened 9 years ago
Updated 2 years ago
Missing or wrong Owner in nsDocShell
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
REOPENED
People
(Reporter: tanvi, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [domsecurity-backlog])
The "owner" passed to nsDocShell::InternalLoad is often missing or wrong.
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9538
The owner should give us an indication of who triggered the load. The owner is used for the requestingPrincipal sent to nsIContentPolicy and the triggeringPrincipal set in loadInfo. It is then used to make security decisions and so we need to get it right.
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9538
We should also rename nsISupports aOwner to nsIPrincipal triggeringPrincipal, but we may do that in a followup bug.
This bug is to:
* identify the places where owner is missing and correct that (ex: by making sure the owner is passed to nsDocShell::AddToSessionHistory() calls - see https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c36 and https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c30)
* identify the places where owner is wrong and correct that (ex: https://www.evernote.com/shard/s200/sh/88f37f17-eccb-4891-be69-26c17472700b/2939af86018e8eba, https://public.etherpad-mozilla.org/p/docShellPrincipals-subframes, https://bugzilla.mozilla.org/show_bug.cgi?id=1181370#c5)
Reporter | ||
Comment 1•9 years ago
|
||
One more thing we need to do in this bug:
* assert that the owner and the referrer are same origin.
The reason we have both an owner and a referrerURI is because the referrerURI provides us with path information. Push state could effect the referrerURI, but doesn't affect the owner. Since push state only works same origin though, the origin should not change.
Updated•9 years ago
|
Whiteboard: [domsecurity-backlog]
Comment 2•8 years ago
|
||
Let's handle all of the required changes regarding the owner within Bug 1181370.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Reporter | ||
Comment 3•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Let's handle all of the required changes regarding the owner within Bug
> 1181370.
>
> *** This bug has been marked as a duplicate of bug 1181370 ***
Hey Chris,
Does Bug 1181370 cover all cases where owner is missing (ex: reload)? Perhaps you could assert(aOwner) in nsDocShell:;InternalLoad() and push to try to see if there are failures. Thanks!
Flags: needinfo?(ckerschb)
Comment 4•8 years ago
|
||
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #3)
> Does Bug 1181370 cover all cases where owner is missing (ex: reload)?
> Perhaps you could assert(aOwner) in nsDocShell:;InternalLoad() and push to
> try to see if there are failures. Thanks!
Yes, Bug 1181370 will hopefully cover all of those cases. Worst case, we have to file another follow up bug :-(
Flags: needinfo?(ckerschb)
Reporter | ||
Comment 5•8 years ago
|
||
Can you check the reload case?
Reporter | ||
Comment 6•8 years ago
|
||
I would rather leave this open until we can confirm that the issue is really fixed.
Comment 7•8 years ago
|
||
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #5)
> Can you check the reload case?
Sure, if you prefer, we can leave this bug open until then - sorry for closing it prematurely.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Updated•8 years ago
|
Reporter | ||
Comment 8•8 years ago
|
||
Chris, can you add a couple assertions?
1) MOZ_ASSERT(owner) here http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1482 and push to try.
2) if(ownerIsExplicit) { MOZ_ASSERT(owner) } and push to try.
Paste the try links and we can see how many failures there are. You mentioned you may have already done this, and hence may already have the try pushes.
Thanks!
Flags: needinfo?(ckerschb)
Comment 9•8 years ago
|
||
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #8)
> Chris, can you add a couple assertions?
Sure, this bug is on my radar anyway. Let me fix up a few other docshell owner (triggeringprincipal) issues first, then I'll get to this one.
Flags: needinfo?(ckerschb)
Reporter | ||
Comment 10•8 years ago
|
||
This bug was spawned form Bug 1105556. That bug is huge and too much to read through. So I've extracted the individual relevant comments.
For more context on the reason this bug was filed see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c18 - part [2]
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c20 - starting with "> A. When we Refresh a page"
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c23
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c24
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c30 (In reply to comment 23 and 24)
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c36
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c38 - number 4)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•