1211590 - nsIScriptSecurityManager::GetChannelURIPrincipal uses UNKNOWN_APP_ID and doesn't properly inherit OriginAttributes

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

I think the OriginAttributes here should match that of the owner or the loadinfo, like GetChannelResultPrincipal does. I'll write up a patch.

Comment 1

[Bobby Holley (:bholley)]

Though realistically, I think it makes more sense to land this on top of bug 1209162.

Once we do that, I think GetChannelURI principal should try to inherit the OriginAttributes from the following things, in order:

(1) The Owner
(2) The LoadInfo
(3) The LoadContext

Does that sound right?

Comment 2

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

Let's not inherit from the owner. Owners are a hack and I'd like to get rid of them. So unless we have to, lets not involve them. Or do we know of an instance where we have to inherit from the owner?

And all channels in gecko have a loadinfo these days, so we might not need a fallback after that. Only reason we'd not have a loadinfo is for channels initiated by addons. I would be fine with falling back to loadContext there yes.

Comment 3

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

Actually, I think what we should do here is that for document loads, we should inherit

1) LoadContext
2) use defaults

For other resources, we should inherit

1) LoadingPrincipal
2) use defaults

I.e. documents inherit from the docshell (through PrincipalOriginAttributes::InheritFromDocShellToDoc which uses the uri). All other resources inherit from their owner document.

Comment 4

[Henry Chang [:hchang]]

Attachment: patch

Comment 5

[Henry Chang [:hchang]]

Attachment: patch

Comment 6

[Henry Chang [:hchang]]

Comment on attachment 8700888 [details] [diff] [review]
patch

Hi Jonas,

I attached a patch according to comment 3. Could you suggest if it looks like what you image? Thanks!

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8700892 [details] [diff] [review]
patch

Hi Bobby,

Could you please have a review on the patch? It is implemented by following comment 3. Thanks!

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8700892 [details] [diff] [review]
patch

Review of attachment 8700892 [details] [diff] [review]:
-----------------------------------------------------------------

I'm out of the loop on the OriginAttributes stuff. Over to sicking, who can delegate if appropriate.

Comment 9

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

Comment on attachment 8700892 [details] [diff] [review]
patch

Review of attachment 8700892 [details] [diff] [review]:
-----------------------------------------------------------------

::: caps/nsScriptSecurityManager.cpp
@@ +406,4 @@
>      if (loadContext) {
> +      // Inherit OriginAttributes from document.
> +      loadContext->GetOriginAttributes(docShellAttrs);
> +      attrs.InheritFromDocShellToDoc(docShellAttrs, uri);

Only do this part when loadInfo->GetExternalContentPolicyType() returns TYPE_DOCUMENT or TYPE_SUBDOCUMENT

@@ +407,5 @@
> +      // Inherit OriginAttributes from document.
> +      loadContext->GetOriginAttributes(docShellAttrs);
> +      attrs.InheritFromDocShellToDoc(docShellAttrs, uri);
> +    } else {
> +      // Inherit origin attributes from loading principal if any.

And only do this part when the type is not TYPE_DOCUMENT or TYPE_SUBDOCUMENT.

I.e. what you want is something like:

if (loadTypeIsDocumentOrSubdocument) {
  if (loadContext) {
    DocShellOriginAttributes docShellAttrs;
    loadContext->GetOA(docShellAttrs);
    attrs.InheritFromDocShellToDoc(docShellAttrs, uri);
  }
} else {
  ...
  if (loadingPrincipal) {
    attrs = BasePrincipal::Cast(loadingPrincipal)->Ori...
  }
}

@@ -564,5 @@
>  static nsresult
>  DenyAccessIfURIHasFlags(nsIURI* aURI, uint32_t aURIFlags)
>  {
>      NS_PRECONDITION(aURI, "Must have URI!");
> -    

Please don't do full-file cleanup like this when making behavior changes. You might need to reconfigure your editor to not remove whitespace whenever you modify a file.

Comment 10

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch

Comment 11

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch

Comment 12

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8707417 [details] [diff] [review]
0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch

Hi Jonas,

I pretty much believe the new patch is what you suggested. Could you please review again? Thanks!

Comment 14

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

Comment on attachment 8707417 [details] [diff] [review]
0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch

Review of attachment 8707417 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: caps/nsScriptSecurityManager.cpp
@@ +410,5 @@
> +
> +    PrincipalOriginAttributes attrs;
> +    if (nsIContentPolicy::TYPE_DOCUMENT == contentPolicyType ||
> +        nsIContentPolicy::TYPE_SUBDOCUMENT == contentPolicyType) {
> +      // If it's document or sub-document, inherit originAttributes from

This line is over 80 columns

@@ +421,5 @@
> +    } else {
> +      // Inherit origin attributes from loading principal if any.
> +      nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +      if (loadInfo) {
> +        loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));

This line is too long. Do something like

loadInfo->
  GetLoadingPrincipal(...

@@ +424,5 @@
> +      if (loadInfo) {
> +        loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> +      }
> +      if (loadingPrincipal) {
> +        attrs = BasePrincipal::Cast(loadingPrincipal)->OriginAttributesRef();

Same here

Comment 15

[Henry Chang [:hchang]]

Thanks Jonas!

However, those lines don't exceed 80 columns. Do I miss something?

(In reply to Jonas Sicking (:sicking) from comment #14)
> Comment on attachment 8707417 [details] [diff] [review]
> 0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch
> 
> Review of attachment 8707417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: caps/nsScriptSecurityManager.cpp
> @@ +410,5 @@
> > +
> > +    PrincipalOriginAttributes attrs;
> > +    if (nsIContentPolicy::TYPE_DOCUMENT == contentPolicyType ||
> > +        nsIContentPolicy::TYPE_SUBDOCUMENT == contentPolicyType) {
> > +      // If it's document or sub-document, inherit originAttributes from
> 
> This line is over 80 columns
> 
> @@ +421,5 @@
> > +    } else {
> > +      // Inherit origin attributes from loading principal if any.
> > +      nsCOMPtr<nsIPrincipal> loadingPrincipal;
> > +      if (loadInfo) {
> > +        loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> 
> This line is too long. Do something like
> 
> loadInfo->
>   GetLoadingPrincipal(...
> 
> @@ +424,5 @@
> > +      if (loadInfo) {
> > +        loadInfo->GetLoadingPrincipal(getter_AddRefs(loadingPrincipal));
> > +      }
> > +      if (loadingPrincipal) {
> > +        attrs = BasePrincipal::Cast(loadingPrincipal)->OriginAttributesRef();
> 
> Same here

Comment 16

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=732550f476cb

Comment 17

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

My bad. I guess splinter breaks before 80 columns. Please ignore my comment.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4384563b945

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f4384563b945