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

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: hchang)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

I think the OriginAttributes here should match that of the owner or the loadinfo, like GetChannelResultPrincipal does. I'll write up a patch.
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?
Depends on: 1209162
Flags: needinfo?(jonas)
Status: NEW → ASSIGNED
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.
Flags: needinfo?(jonas)
Assignee: bobbyholley → nobody
Status: ASSIGNED → NEW
Blocks: 1178526
Assignee: nobody → hchang
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.
Posted patch patch (obsolete) — Splinter Review
Posted patch patch (obsolete) — Splinter Review
Attachment #8700888 - Attachment is obsolete: true
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!
Attachment #8700888 - Flags: feedback?(jonas)
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!
Attachment #8700892 - Flags: review?(bobbyholley)
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.
Attachment #8700892 - Flags: review?(bobbyholley) → review?(jonas)
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.
Attachment #8700892 - Flags: review?(jonas) → review-
Attachment #8700892 - Attachment is obsolete: true
Attachment #8707386 - Attachment is obsolete: true
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!
Attachment #8707417 - Flags: review?(jonas)
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
Attachment #8707417 - Flags: review?(jonas) → review+
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
Flags: needinfo?(jonas)
My bad. I guess splinter breaks before 80 columns. Please ignore my comment.
Flags: needinfo?(jonas)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4384563b945
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.