Closed
Bug 1376971
Opened 6 years ago
Closed 5 years ago
Isolate Page Info media previews to content first party
Categories
(Core :: DOM: Security, defect, P3)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
firefox58 | --- | fixed |
People
(Reporter: jhao, Assigned: allstars.chh)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [tor][tor 22327][OA][userContextId][domsecurity-backlog1])
Attachments
(5 files, 14 obsolete files)
3.55 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
5.44 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
6.68 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
3.50 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
6.93 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
First party isolation is not properly enforced in the Page Info window. See Tor ticket: https://trac.torproject.org/projects/tor/ticket/22327
Updated•6 years ago
|
Whiteboard: [userContextId][tor]
Reporter | ||
Updated•6 years ago
|
Whiteboard: [userContextId][tor] → [tor][OA][userContextId]
Updated•6 years ago
|
Priority: -- → P1
Comment 2•6 years ago
|
||
Ethan, you marked this as a P1. Just making sure it's not slipping the cracks.
Flags: needinfo?(ettseng)
Comment 3•6 years ago
|
||
Maybe this is not urgent. I have to check with the Containers team for the priority.
Comment 4•6 years ago
|
||
This bug is important but not very urgent for First Party Isolation. Set it as P2 for now. If the Containers team has a different opinion, we could change it.
Flags: needinfo?(ettseng)
Priority: P1 → P2
Updated•6 years ago
|
Blocks: ContextualIdentity
Updated•6 years ago
|
Whiteboard: [tor][OA][userContextId] → [tor][tor 22327][OA][userContextId]
Comment 5•6 years ago
|
||
Here is Tor Browser's current patch for this issue: https://torpat.ch/22327 We don't currently have a regression test, however.
Updated•6 years ago
|
Priority: P2 → P3
Whiteboard: [tor][tor 22327][OA][userContextId] → [tor][tor 22327][OA][userContextId][domsecurity-backlog1]
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → allstars.chh
Flags: needinfo?(allstars.chh)
Assignee | ||
Comment 6•6 years ago
|
||
Hi Arthur I have some questions for your patch. 1. You forcibly override the contentType to nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON, whereas their original contentType were nsIContentPolicy::TYPE_INTERNAL_IMAGE, is this intentional? I understand that we only set origin attributes for favicon, http://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/image/imgLoader.cpp#790 But perhaps we should fix in NewImageChannel, instead of changing contentPolicyType there. 2. What are the funuction 'nsContentUtils::ApplyCustomLoadPrincipalToChannel' for? Are the modifications in dom/html/HTMLMediaElement.cpp and dom/media/MediaResource.cpp neccesary? Can you help to explain this? It looks to me these changes are not related to this bug, but perhaps I misunderstood.
Flags: needinfo?(arthuredelstein)
Assignee | ||
Comment 7•5 years ago
|
||
Arthur, in the end I think reusing nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON could be a better idea, I'll reuse it and see how reviewer thinks.
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Yoshi Huang [:allstars.chh] from comment #6) > 2. What are the funuction > 'nsContentUtils::ApplyCustomLoadPrincipalToChannel' for? > Are the modifications in dom/html/HTMLMediaElement.cpp and > dom/media/MediaResource.cpp neccesary? > Okay, I see, it's for <video> and <audio>.
Flags: needinfo?(arthuredelstein)
Assignee | ||
Comment 9•5 years ago
|
||
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
|
||
Attachment #8908565 -
Attachment is obsolete: true
Assignee | ||
Comment 15•5 years ago
|
||
Attachment #8908566 -
Attachment is obsolete: true
Assignee | ||
Comment 16•5 years ago
|
||
Attachment #8908567 -
Attachment is obsolete: true
Assignee | ||
Comment 17•5 years ago
|
||
Attachment #8908569 -
Attachment is obsolete: true
Assignee | ||
Comment 18•5 years ago
|
||
Attachment #8908570 -
Attachment is obsolete: true
Assignee | ||
Comment 19•5 years ago
|
||
Attachment #8909254 -
Attachment is obsolete: true
Comment 20•5 years ago
|
||
Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020
status-firefox57:
--- → fix-optional
Assignee | ||
Comment 21•5 years ago
|
||
Attachment #8909252 -
Attachment is obsolete: true
Assignee | ||
Comment 22•5 years ago
|
||
Attachment #8910190 -
Attachment is obsolete: true
Assignee | ||
Comment 23•5 years ago
|
||
Attachment #8909253 -
Attachment is obsolete: true
Assignee | ||
Comment 24•5 years ago
|
||
Attachment #8909591 -
Attachment is obsolete: true
Assignee | ||
Comment 25•5 years ago
|
||
Comment on attachment 8909250 [details] [diff] [review] Part 1: add loadingprincipal attribute on the node. Review of attachment 8909250 [details] [diff] [review]: ----------------------------------------------------------------- Hi, baku This is similar to previous bug 1277803 and bug 1319908 so I request for your r?. Can you help to review this? Thanks
Attachment #8909250 -
Flags: review?(amarchesini)
Assignee | ||
Comment 26•5 years ago
|
||
Attachment #8909251 -
Attachment is obsolete: true
Attachment #8910200 -
Flags: review?(amarchesini)
Assignee | ||
Updated•5 years ago
|
Attachment #8910194 -
Flags: review?(amarchesini)
Assignee | ||
Updated•5 years ago
|
Attachment #8910195 -
Flags: review?(amarchesini)
Assignee | ||
Updated•5 years ago
|
Attachment #8910197 -
Flags: review?(amarchesini)
Updated•5 years ago
|
Attachment #8909250 -
Flags: review?(amarchesini) → review+
Comment 27•5 years ago
|
||
Comment on attachment 8910200 [details] [diff] [review] Part 2: add GetLoadingPrincipalForXULNode. Review of attachment 8910200 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentUtils.cpp @@ +10405,5 @@ > > +/* static */ bool > +nsContentUtils::GetLoadingPrincipalForXULNode(nsIContent* aLoadingNode, > + nsIPrincipal** aLoadingPrincipal) > +{ MOZ_ASSERT(aLoadingNode); MOZ_ASSERT(aLoadingPrincipal); @@ +10409,5 @@ > +{ > + bool result = false; > + nsCOMPtr<nsIPrincipal> loadingPrincipal = aLoadingNode->NodePrincipal(); > + nsAutoString loadingStr; > + aLoadingNode->GetAttr(kNameSpaceID_None, nsGkAtoms::loadingprincipal, this can fail. ::: dom/base/nsContentUtils.h @@ +3019,5 @@ > nsTArray<nsIContent*>& aKids, > uint32_t aFlags); > > + /** > + * Query loadingPrincipal if it is specified as 'loadingprincipal' on attribute
Attachment #8910200 -
Flags: review?(amarchesini) → review+
Updated•5 years ago
|
Attachment #8910194 -
Flags: review?(amarchesini) → review+
Updated•5 years ago
|
Attachment #8910197 -
Flags: review?(amarchesini) → review+
Comment 28•5 years ago
|
||
> > +/* static */ bool
> > +nsContentUtils::GetLoadingPrincipalForXULNode(nsIContent* aLoadingNode,
> > + nsIPrincipal** aLoadingPrincipal)
> > +{
>
> MOZ_ASSERT(aLoadingNode);
> MOZ_ASSERT(aLoadingPrincipal);
>
Assert that aLoadingNode is a XUL node for real.
Comment 29•5 years ago
|
||
Comment on attachment 8910195 [details] [diff] [review] Part 4: query loadingprincipal in HTMLMediaElement Review of attachment 8910195 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +1198,5 @@ > + // setAttrs is true we will override the origin attributes on the channel > + // later. > + nsCOMPtr<nsIPrincipal> loadingPrincipal; > + bool setAttrs = nsContentUtils::GetLoadingPrincipalForXULNode(aElement, > + getter_AddRefs(loadingPrincipal)); indentation.
Attachment #8910195 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 30•5 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #27) > @@ +10409,5 @@ > > +{ > > + bool result = false; > > + nsCOMPtr<nsIPrincipal> loadingPrincipal = aLoadingNode->NodePrincipal(); > > + nsAutoString loadingStr; > > + aLoadingNode->GetAttr(kNameSpaceID_None, nsGkAtoms::loadingprincipal, > > this can fail. > Hi baku, Thanks for the review. But from nsIContent.h http://searchfox.org/mozilla-central/source/dom/base/nsIContent.h#402 @returns true if the attribute was set (even when set to empty string) false when not set. It doesn't return nsresult, I guess we only care if the value is empty or not, so I'll keep my original code here.
Assignee | ||
Comment 31•5 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #28) > > > +/* static */ bool > > > +nsContentUtils::GetLoadingPrincipalForXULNode(nsIContent* aLoadingNode, > > > + nsIPrincipal** aLoadingPrincipal) > > > +{ > > > > MOZ_ASSERT(aLoadingNode); > > MOZ_ASSERT(aLoadingPrincipal); > > > > Assert that aLoadingNode is a XUL node for real. Hi Baku In my Part 3 patch, this function could be called from nsImageLoadingContent, which means it could be called from content, so in content case aLoadingNode will be a content node. However I've already added an assert that if we successfully get the loadingprincipal value from that node, its nodePrincipal should be System Principal, see my Part 2 patch. + // We only allow specifying loadingprincipal attribute on a node loaded by + // SystemPrincipal. + MOZ_ASSERT(nsContentUtils::IsSystemPrincipal(aLoadingNode->NodePrincipal()), + "aLoadingNode Should be loaded with SystemPrincipal"); + Do you want me to add assertion for XUL node here (right after the assert for aLoadingNode->NodePrincipal() is SystemPrincipal)? Or the original assert is enough? Thanks
Assignee | ||
Comment 32•5 years ago
|
||
rebase
Attachment #8910200 -
Attachment is obsolete: true
Attachment #8911702 -
Flags: review+
Assignee | ||
Comment 33•5 years ago
|
||
Attachment #8910194 -
Attachment is obsolete: true
Attachment #8911703 -
Flags: review+
Assignee | ||
Comment 34•5 years ago
|
||
rebase
Attachment #8910195 -
Attachment is obsolete: true
Attachment #8911704 -
Flags: review+
Assignee | ||
Comment 35•5 years ago
|
||
Baku, can you check my comment 30 and comment 31? Thanks
Flags: needinfo?(amarchesini)
Comment 36•5 years ago
|
||
Yes, just add MOZ_ASSERT(aLoadingNode->IsXULElement());
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 37•5 years ago
|
||
(In reply to Andrea Marchesini [:baku] from comment #36) > Yes, just add MOZ_ASSERT(aLoadingNode->IsXULElement()); I found we shouldn't add this assert. In the test browser/base/content/test/pageinfo/browser_pageinfo_firstPartyIsolation.js (see Part 5), the aLoadingNode is actually an instance of HTMLImageElement, although its OwnerDoc is chrome://browser/content/pageinfo/pageInfo.xul, but it is not a XUL-Element. So I'll remove the assert. Thanks
Comment 38•5 years ago
|
||
Pushed by yhuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/282f44b7a0e1 Part 1: add loadingprincipal attribute on the node. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/89160848b6a0 Part 2: add GetLoadingPrincipalForXULNode. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/23473b758d55 Part 3: Query loadingprincipal attribute in image. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/242437f8be99 Part 4: query loadingprincipal in HTMLMediaElement. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/cb604e7830ec Part 5: test for pageInfo.js. r=baku
Comment 39•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/282f44b7a0e1 https://hg.mozilla.org/mozilla-central/rev/89160848b6a0 https://hg.mozilla.org/mozilla-central/rev/23473b758d55 https://hg.mozilla.org/mozilla-central/rev/242437f8be99 https://hg.mozilla.org/mozilla-central/rev/cb604e7830ec
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•