1082764 - [e10s] Entering the url to an XPI in the address bar will use the current page's URL as the referrer for security checking the install

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

We do a security check assuming the about: page is attempting to do the load when that isn't the case.

Comment 1

[Dave Townsend [:mossop]]

Morphing this as this is a more general problem. Any time we try to load an XPI URL the current page is used as the referrer even if entered into the url bar directly (shouldn't be a referrer at all in this case) or if loadURI is passed a referrer directly. This causes us to do security checks against the wrong domains.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch

Using the current page's InstallTrigger to install XPIs detected in the content handler is a mistake. It means we assume the current page is the installer which is false in many cases. Instead just send the install message to the parent process with the correct referrer. This makes the code simpler too!

Comment 6

[Dave Townsend [:mossop]]

Forgot to mention, this patch applies on top of bug 1084558.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

::: toolkit/content/browser-child.js
@@ +198,5 @@
>        CrashReporter.annotateCrashReport("URL", uri);
>  #endif
> +    if (referrer)
> +      referrer = Services.io.newURI(referrer, null, null);
> +    this._webNavigation.loadURI(uri, flags, referrer, null, null);

Directly related test for this?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Blair McBride [:Unfocused] from comment #7)
> Comment on attachment 8515254 [details] [diff] [review]
> patch
> 
> Review of attachment 8515254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/browser-child.js
> @@ +198,5 @@
> >        CrashReporter.annotateCrashReport("URL", uri);
> >  #endif
> > +    if (referrer)
> > +      referrer = Services.io.newURI(referrer, null, null);
> > +    this._webNavigation.loadURI(uri, flags, referrer, null, null);
> 
> Directly related test for this?

Splitting out this change as it probably makes sense as a separate issue.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch

Same patch but with the RemoteWebNavigation stuff split out.

Comment 10

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/33f386c49a64

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33f386c49a64