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

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 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)

(Reporter)

Description

3 years ago
I think the OriginAttributes here should match that of the owner or the loadinfo, like GetChannelResultPrincipal does. I'll write up a patch.
(Reporter)

Comment 1

3 years ago
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)

Updated

3 years ago
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)
(Reporter)

Updated

3 years ago
Assignee: bobbyholley → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

3 years ago
Blocks: 1178526
(Assignee)

Updated

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8700888 [details] [diff] [review]
patch
(Assignee)

Comment 5

3 years ago
Created attachment 8700892 [details] [diff] [review]
patch
Attachment #8700888 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
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)
(Assignee)

Comment 7

3 years ago
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)
(Reporter)

Comment 8

3 years ago
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-
(Assignee)

Comment 10

3 years ago
Created attachment 8707386 [details] [diff] [review]
0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch
Attachment #8700892 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Created attachment 8707416 [details] [diff] [review]
0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch
Attachment #8707386 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8707417 [details] [diff] [review]
0001-Bug-1211590-Inherits-OriginAttributes-from-loading-p.patch
Attachment #8707416 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
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+
(Assignee)

Comment 15

3 years ago
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)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f4384563b945
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.