1376971 - Isolate Page Info media previews to content first party

Status: RESOLVED FIXED

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

Description

[Jonathan Hao (inactive) [:jhao]]

First party isolation is not properly enforced in the Page Info window.
See Tor ticket: https://trac.torproject.org/projects/tor/ticket/22327

Comment 1

[Ethan Tseng [:ethan]]

Yoshi, do you think you can take this bug?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Ethan, you marked this as a P1. Just making sure it's not slipping the cracks.

Comment 3

[Ethan Tseng [:ethan]]

Maybe this is not urgent.  I have to check with the Containers team for the priority.

Comment 4

[Ethan Tseng [:ethan]]

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.

Comment 5

[Arthur Edelstein [:arthur]]

Here is Tor Browser's current patch for this issue: https://torpat.ch/22327
We don't currently have a regression test, however.

Comment 6

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

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.

Comment 7

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

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.

Comment 8

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

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

Comment 9

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

Attachment: Part 1: add loadingprincipal attribute on the node.

Comment 10

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

Attachment: Part 2: Query loadingprincipal attribute in image

Comment 11

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

Attachment: Part 3: add GetLoadingPrincipalForXULNode.

Comment 12

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

Attachment: Part 4: query loadingprincipal in HTMLMediaElement

Comment 13

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

Attachment: Part 5: test for pageInfo.js

Comment 14

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

Attachment: Part 1: add loadingprincipal attribute on the node.

Comment 15

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

Attachment: Part 2: Query loadingprincipal attribute in image

Comment 16

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

Attachment: Part 3: add GetLoadingPrincipalForXULNode.

Comment 17

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

Attachment: Part 4: query loadingprincipal in HTMLMediaElement

Comment 18

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

Attachment: Part 5: test for pageInfo.js

Comment 19

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

Attachment: Part 5: test for pageInfo.js

Comment 20

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 21

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

Attachment: Part 2: add GetLoadingPrincipalForXULNode.

Comment 22

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

Attachment: Part 3: Query loadingprincipal attribute in image

Comment 23

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

Attachment: Part 4: query loadingprincipal in HTMLMediaElement

Comment 24

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

Attachment: Part 5: test for pageInfo.js

Comment 25

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

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

Comment 26

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

Attachment: Part 2: add GetLoadingPrincipalForXULNode.

Comment 27

[Andrea Marchesini [:baku]]

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

Comment 28

[Andrea Marchesini [:baku]]

> > +/* 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

[Andrea Marchesini [:baku]]

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.

Comment 30

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

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

Comment 31

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

(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

Comment 32

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

Attachment: Part 2: add GetLoadingPrincipalForXULNode. v2

rebase

Comment 33

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

Attachment: Part 3: Query loadingprincipal attribute in image. v2

Comment 34

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

Attachment: Part 4: query loadingprincipal in HTMLMediaElement. v2

rebase

Comment 35

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

Baku, can you check my comment 30 and comment 31?
Thanks

Comment 36

[Andrea Marchesini [:baku]]

Yes, just add MOZ_ASSERT(aLoadingNode->IsXULElement());

Comment 37

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

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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