1182540 - Use channel->ascynOpen2 in dom/html/HTMLTrackElement.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182540_asyncopen2_htmltrackelement.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182540_asyncopen2_htmltrackelement.patch

Simplified the NodeType() checking within contentsecurityManager. Carrying over r+ from Jonas.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90e59cd616ae

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/7f458c4bd2713cfaea02eddcb48470d535c9b815
changeset:  7f458c4bd2713cfaea02eddcb48470d535c9b815
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sat Jul 25 10:29:22 2015 -0700
description:
Bug 1182540 - Use channel->ascynOpen2 in dom/html/HTMLTrackElement.cpp (r=sicking)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f458c4bd271

Comment 6

[Tanvi Vyas[:tanvi]]

Comment on attachment 8637398 [details] [diff] [review]
bug_1182540_asyncopen2_htmltrackelement.patch

Was out last week, so just seeing this now.

-  rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_INTERNAL_TRACK,
-                                 uri,
-                                 NodePrincipal(),
-                                 static_cast<Element*>(this),

   rv = NS_NewChannel(getter_AddRefs(channel),
                      uri,
                      static_cast<Element*>(this),
-                     nsILoadInfo::SEC_NORMAL,
+                     nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,
                      nsIContentPolicy::TYPE_INTERNAL_TRACK,
                      loadGroup);

Are NodePrincipal and the principal of static_cast<Element*>(this) the same?  With this change, Content Policies will get the loadingPrincipal from 'this'.  Previously, it was passed in a loadingPrincipal of "NodePrinicipal()".  If they are the same, then we are good. If they are different, we may need to investigate more.

My guess is that NodePrincipal will be the principal of the document that the track element is in.  'this' will refer to the track element itself and have a principal matching the track's src attribute.  Content Policy API should then look for the track elements parent and pass it as requestPrincipal when doing security checks.  I don't know if the API gets the parent before calling the individual policies.

Example: if you have https://a.com with <track src=https://b.com> then the call to Content Policy would have previously received a requestingPrincipal of a.com and will now receive a requestingPrincipal of b.com.

I recall discussing this with Jonas before; we said that requestingContext is sometimes the element itself and sometimes the document.  Jonas said we should pass the element (as we do here) whenever we can to provide the most information possible, and let Content Policies find the appropriate parent node when necessary.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Tanvi Vyas [:tanvi] from comment #6)
> Are NodePrincipal and the principal of static_cast<Element*>(this) the same?

Yes. The way we get the principal from a node is that we call the NodePrincipal() function.

> My guess is that NodePrincipal will be the principal of the document that
> the track element is in.

That is correct.

> 'this' will refer to the track element itself

That is correct.

> and have a principal matching the track's src attribute.

That is not correct. The principal of a node is never affected by what content it is loading. It is only affected by what document the node is in. So an <img src> element always has the principal of the node's ownerDocument. It is completely unaffected by what image it has loaded.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Indeed.  The image _data_ has its own separate principal, but that has nothing to do with the principal of the node.