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)

Product:
Core
Component:
DOM: Security
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
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)

Part 1: add loadingprincipal attribute on the node.
3.55 KB, patch
baku
: review+
Details | Diff | Splinter Review
Part 5: test for pageInfo.js
5.44 KB, patch
baku
: review+
Details | Diff | Splinter Review
Part 2: add GetLoadingPrincipalForXULNode. v2
6.68 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Part 3: Query loadingprincipal attribute in image. v2
3.50 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Part 4: query loadingprincipal in HTMLMediaElement. v2
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
Whiteboard: [userContextId][tor]
Whiteboard: [userContextId][tor] → [tor][OA][userContextId]
Priority: -- → P1
Yoshi, do you think you can take this bug?
Flags: needinfo?(allstars.chh)
Ethan, you marked this as a P1. Just making sure it's not slipping the cracks.
Flags: needinfo?(ettseng)
Maybe this is not urgent.  I have to check with the Containers team for the priority.
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
Whiteboard: [tor][OA][userContextId] → [tor][tor 22327][OA][userContextId]
Here is Tor Browser's current patch for this issue: https://torpat.ch/22327
We don't currently have a regression test, however.
Priority: P2 → P3
Whiteboard: [tor][tor 22327][OA][userContextId] → [tor][tor 22327][OA][userContextId][domsecurity-backlog1]
Assignee: nobody → allstars.chh
Flags: needinfo?(allstars.chh)
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)
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.
(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)
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)

        Attachment #8909251 -
        Attachment is obsolete: true

        Attachment #8910200 -
        Flags: review?(amarchesini)

        Attachment #8909250 -
        Flags: review?(amarchesini) → review+

    
    

    
  
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+

        Attachment #8910194 -
        Flags: review?(amarchesini) → review+

        Attachment #8910197 -
        Flags: review?(amarchesini) → review+

    
    

    
  
> > +/* static */ bool
> > +nsContentUtils::GetLoadingPrincipalForXULNode(nsIContent* aLoadingNode,
> > +                                              nsIPrincipal** aLoadingPrincipal)
> > +{
> 
> MOZ_ASSERT(aLoadingNode);
> MOZ_ASSERT(aLoadingPrincipal);
> 

Assert that aLoadingNode is a XUL node for real.

    
    

    
  
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+

    
    

    
  
(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.

    
    

    
  
(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

    
    

    
  
Baku, can you check my comment 30 and comment 31?
Thanks
Flags: needinfo?(amarchesini)

    
    

    
  
Yes, just add MOZ_ASSERT(aLoadingNode->IsXULElement());
Flags: needinfo?(amarchesini)

    
    

    
  
(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

    
    

    
  
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
Depends on: 1405195
Depends on: 1403365

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