1042699 - (CVE-2015-4498) Firefox Addon bypass dialog and spoof vulnerability

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Bas Venis]

Attachment: poc.zip

Normally Firefox warns the user when trying to install add-on if this was page-initiated. 

When the page is redirecting or navigating by <a href link, JavaScript or server redirect, Firefox trows the 
'Firefox prevented this site (site.com) from asking you to install software on your computer.' warning at the user, which needs to explicitly be accepted for the add-on to continue installing.
There is however, a simple vulnerability that lets an attacker bypass this dialog, which allows a rather nasty attack on the user.

Basically, there is one exception in which the dialog will not be shown, which is if the user pastes or copies the direct link/URL in the URL bar.

Now the user could just click on links, redirects etc that lead to this page, because a data uri redirected to the page which itself redirects with a 'page moved header' to the location of the add-on, will disrupt that 'chain' and installation of the add-on will start without the dialog. As if the user typed it in directly.

This behavior in combination with a properly timed attack will result in a scenario where the user will download the add-on controlled by the attacker, the legit add-on webpage shown with it. The only thing not correct is the url of the pushed add-on, which the user CANNOT verify. This dialog is really only useful if the download location is from another domain. Since it will only show the https://addons.mozzila.org/firefox/download/latest/123/randomurl .. [truncated].

So the attacker will simply upload his unreviewed add-on, with the same name and icon as the desired add-on to be spoofed. Sure, this will eventually be reviewed and picked up or reported in the process. But in the meanwhile the direct link will be used. And since the reputation of the spoofed add-on that is being pushed doesn't matter (user never views that page). There is no way for the user to notice that the add-on is spoofed.

Attached are the files that would be running on the server.

Demonstration of the issue: https://www.youtube.com/watch?v=VTKblGpeW4Y (unlisted)

Comment 1

[Daniel Veditz [:dveditz]]

I just get 28 seconds of black on the youtube video.

Comment 2

[Bas Venis]

Weird, works fine for me. Nonetheless, the video is also included in the poc.zip

Comment 3

[Daniel Veditz [:dveditz]]

> Basically, there is one exception in which the dialog will not be shown, which is
> if the user pastes or copies the direct link/URL in the URL bar.

That's an intentional exception. The block is more of an anti-annoyance feature so sites can't pester people into giving in and installing (the next dialog is the actual security prompt). It's known that if you can white-wash your referer away you can mimic this, but then you're generally not at the same site so you can't keep annoying the user that way. Acceptable risk unless we see people start to abuse it, basically.

Spoofing people with the AMO origin of unreviewed add-ons, however, is clever.

Wil: would it be possible to require people to be signed into AMO to reach download links for unreviewed add-ons? Or failing that, maybe change the server to reject such links if the referer for the download doesn't come from AMO itself? Either would pretty much eliminate the potential for spoofing here.

Comment 4

[Wil Clouser [:clouserw]]

The youtube video never loads for me and the video in the zip file shows a desktop but flickers so much I can't tell what is going on.  So, I can't tell entirely what we're talking about - when you say "unreviewed add-on" are you talking about a preliminarily reviewed add-on?  Can you put in specific steps to reproduce this so I can get a better idea of what's going on?

Regarding the questions:  It's possible to require people be signed in, but I'm not sure it's something we'd want to do.  I think we used to require people be signed in to download add-ons which weren't fully reviewed (when we had the yellow/black stripes on the buttons - it was a long time ago) and we got enough of a negative reaction to it that we stopped doing it.  Jorge could probably give more info.  We'd also have to ask if that would really change anything about the scenario.

Regarding referrers, that depends what URL is being pasted in.  If it's a download link on AMO, potentially we could, but if it's a cached URL directly to the file then probably not.  We may also interfere with legitimate 3rd party sites that link to AMO directly on purpose (and add their own src=___ to the URL for tracking and such).

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Wil Clouser [:clouserw] from comment #4)
> when you say "unreviewed add-on"
> are you talking about a preliminarily reviewed add-on?  Can you put in
> specific steps to reproduce this so I can get a better idea of what's going
> on?

As I understand it, the attacker uses some trickery to directly load the URL that points to the XPI file of an unreviewed add-on on AMO. This bypasses the warning that should appear for non-AMO add-ons, and also the warnings we show on AMO for unreviewed add-ons. This would only work while the version is waiting in the queue, which can range from a few hours to a few weeks depending on the add-on and reviewer availability.

Note that this is only about unreviewed add-ons in the queue, not preliminarily approved add-ons. The latter are checked for security issues, just like fully reviewed add-ons.

> Regarding the questions:  It's possible to require people be signed in, but
> I'm not sure it's something we'd want to do.

We expose unreviewed add-ons and versions to allow enthusiastic users to be able to install them while they wait in the queue. They are only discoverable by people who know what they're doing. If that's the best solution, I don't think it's a big deal to require being logged in to be able to install these files.

Comment 6

[Daniel Veditz [:dveditz]]

Decoder met the reporter at HITB and apparently this was intended to be nominated for a bug bounty (though I couldn't find any such mail to our security alias). Fixing that now.

In part this specific example spoof takes advantage of the fact that AMO mixes trusted/reviewed content with untrusted content on the same web and storage servers. Terrible, terrible idea and I've been complaining about it for years (maybe even a decade) so I give up. Signing (starting soon) will at least make the unreviewed stuff not install and break this particular example, but really the word "untrusted" should be in the domain, or path, or not use mozilla.(com/org/net) at all.

Since several client bugs conspire for the rest I'll leave this as a client bug rather than trying to break the chain on the AMO side as we started talking about in the last couple of comments. We probably need to open separate bugs on these because we may need different people to work on them.

1) bypassing the install block is annoying, but by itself not all that serious (sec-low?). We might even have a bug on it already. Without debugging it, my guess is that when the data: page loads the .xpi we're using the data: url as the referrer rather than the origin from the principal. data URLs don't have a host so they look like a file: url, which are allowed to trigger installs. We probably can't change the referrer without breaking lots of stuff so we should check scheme and not just assume based on a null host.

1a) we should check that we're still using the "internal referrer" which we always set because the http header can be stripped entirely in various ways. With the meta-referrer spec that's even easier. The "internal referrer" was something we always set.

2) The heart of this spoof is that the download doorhanger and dialog show up on a page different from the one that spawned it. Coupled with text that says "This site..." rather than the actual origin adds to the spoofing. We should kill the download/install if the progress events are coming back to a different document than the one that launched it -- can't think of a good reason for a legit page to navigate right away, and they can use the callback function to navigate after if they want to. And even if there is a good reason, we're not going to be able to come up with UI that is spoof-proof. Users are ALWAYS going to assume it's the new page doing whatever the doorhanger says.

2a) this really applies to most door-hangers

2b) do we allow non-same-origin frames to trigger an install? If so we have the same spoofing problem if there's an injected or otherwise hostile frame (ad server?). There are several other permission type things in the product where we don't even allow a non-same-origin to request the permission because it's too easy to confuse the user as to whom is asking, and we should think about that for installs.

3) Part of this spoof relies on the ability to make the one thing users can check--the URL in the install dialog--be the expected host (AMO) even though it's untrusted content. That much could be fixed on the AMO side by moving the untrusted content. But the NEW UI doesn't ever show this URL. The name from the signed install.rdf is shown which helps a bit (assuming we won't sign a dupe name) but even then we allow names that are similar to other names and that's probably good enough for a spoof. Worse, the installing site could _tell_ you its installing GoodAddon from AMO, but it's actually installing an old copy of GoodAddon (with known vulns) from its own site. You would have no idea.
  Sure, lots of people wouldn't know where GoodAddon is supposed to be loaded from, but showing the URL of the install package will give savvier users a chance to catch something scammy and report it, helping us to protect all users. If we can't show the URL in the doorhanger can we at least show it on hovering over the name, or make the name a link to the URL that the user can use the context menu to copy?

I'm raising the severity because given the new UI there's no longer the URL shown to help users figure out they've been spoofed, "the site showing the doorhanger" is the security check and that will be green-lock-secure AMO.

Comment 7

[Andrew McCreight [:mccr8]]

Dave, do you know who could look at this? It is a bug from a year ago that was recently reclassified as sec-high. Thanks!

Comment 8

[Dave Townsend [:mossop]]

This looks more necko related. The code here isn't getting a referrer for the request (both branches get a null referer) so we assume it was user-entered. Maybe jduell can recommend someone to look.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/amContentHandler.js#46

That said when add-on signing requirements are enabled we'll refuse to install these unlisted add-ons because they won't be signed so I think we should downgrade the severity of this at that point.

Comment 9

[Jason Duell]

The sec-high rating here (from comment 6) is about the new addon UI not providing the URL of the addon any more.  AFAICT that's an addon code fix, not necko.

Bypassing the install block (which is the thing the blank referrer currently enables one to do) is rated as sec-low (which is lower than I would have expected, but I'll defer to dveditz).

Note: my read of the meta-referrer support we landed early this year is that this feature now allows any arbitrary HTML page to suppress the referer header, and so we don't even need the data redirect trick to force an addon install without the user seeing the block. Is that right? Or is the addon check using the internal referrer (aka Docshell QI'd to a propertyBag("docshell.internalReferrer”), so this isn't an issue?

I'm not familiar with how "docshell.internalReferrer” gets set, so if we do want to fix the data:// redirect issue, we'll have to look into how it gets set and why it doesn't happen in that case.

Comment 10

[Jason Duell]

Punting back to mossop to find someone to fix the addon UI issue.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #9)
> The sec-high rating here (from comment 6) is about the new addon UI not
> providing the URL of the addon any more.  AFAICT that's an addon code fix,
> not necko.
> 
> Bypassing the install block (which is the thing the blank referrer currently
> enables one to do) is rated as sec-low (which is lower than I would have
> expected, but I'll defer to dveditz).
> 
> Note: my read of the meta-referrer support we landed early this year is that
> this feature now allows any arbitrary HTML page to suppress the referer
> header, and so we don't even need the data redirect trick to force an addon
> install without the user seeing the block. Is that right? Or is the addon
> check using the internal referrer (aka Docshell QI'd to a
> propertyBag("docshell.internalReferrer”), so this isn't an issue?
> 
> I'm not familiar with how "docshell.internalReferrer” gets set, so if we do
> want to fix the data:// redirect issue, we'll have to look into how it gets
> set and why it doesn't happen in that case.

We use docshell.internalReferrer but it isn't getting set here. The load here is a data: url loaded from a Location: header sent by a http request. The data: document includes a <script> that sets window.location.href.

In this case we end up here and principalURI is apparently a null principal so the loadinfo referrer never gets set.

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsLocation.cpp#161

Comment 12

[Dave Townsend [:mossop]]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> 1) bypassing the install block is annoying, but by itself not all that
> serious (sec-low?). We might even have a bug on it already. Without
> debugging it, my guess is that when the data: page loads the .xpi we're
> using the data: url as the referrer rather than the origin from the
> principal. data URLs don't have a host so they look like a file: url, which
> are allowed to trigger installs. We probably can't change the referrer
> without breaking lots of stuff so we should check scheme and not just assume
> based on a null host.
> 
> 1a) we should check that we're still using the "internal referrer" which we
> always set because the http header can be stripped entirely in various ways.
> With the meta-referrer spec that's even easier. The "internal referrer" was
> something we always set.

We are, just in this case it isn't set, see above.

> 2) The heart of this spoof is that the download doorhanger and dialog show
> up on a page different from the one that spawned it. Coupled with text that
> says "This site..." rather than the actual origin adds to the spoofing. We
> should kill the download/install if the progress events are coming back to a
> different document than the one that launched it -- can't think of a good
> reason for a legit page to navigate right away, and they can use the
> callback function to navigate after if they want to. And even if there is a
> good reason, we're not going to be able to come up with UI that is
> spoof-proof. Users are ALWAYS going to assume it's the new page doing
> whatever the doorhanger says.

Well, sites can't get the callback unless they are whitelisted so if they want to show some post-install page they pretty much have to do it immediately after triggering the install.

> 2a) this really applies to most door-hangers
> 
> 2b) do we allow non-same-origin frames to trigger an install? If so we have
> the same spoofing problem if there's an injected or otherwise hostile frame
> (ad server?). There are several other permission type things in the product
> where we don't even allow a non-same-origin to request the permission
> because it's too easy to confuse the user as to whom is asking, and we
> should think about that for installs.

I think so, and I think we've even abused this feature in the past :(

> 3) Part of this spoof relies on the ability to make the one thing users can
> check--the URL in the install dialog--be the expected host (AMO) even though
> it's untrusted content. That much could be fixed on the AMO side by moving
> the untrusted content. But the NEW UI doesn't ever show this URL. The name
> from the signed install.rdf is shown which helps a bit (assuming we won't
> sign a dupe name) but even then we allow names that are similar to other
> names and that's probably good enough for a spoof. Worse, the installing
> site could _tell_ you its installing GoodAddon from AMO, but it's actually
> installing an old copy of GoodAddon (with known vulns) from its own site.
> You would have no idea.
>   Sure, lots of people wouldn't know where GoodAddon is supposed to be
> loaded from, but showing the URL of the install package will give savvier
> users a chance to catch something scammy and report it, helping us to
> protect all users. If we can't show the URL in the doorhanger can we at
> least show it on hovering over the name, or make the name a link to the URL
> that the user can use the context menu to copy?
> 
> I'm raising the severity because given the new UI there's no longer the URL
> shown to help users figure out they've been spoofed, "the site showing the
> doorhanger" is the security check and that will be green-lock-secure AMO.

We took the URL out of the UI because we decided it generally wasn't useful information to the user, more useful was the installing origin, which does show up for http sites but not for data or in this case where we don't know it. As you say the URL is indistinguishable from a regular add-on's URL so I don't think adding the URL back helps us much here.

I think the right fix is to not drop install requests on the floor if they come from somewhere other than the top-level origin and then bug 1147812 to cancel ongoing installs. We probably need the docshell referrer info to be available to do that.

Comment 13

[Jason Duell]

Valentin, can you take a look and see if there's a reasonably clean way to get the internal referrer field to get set correctly for redirects to data: uris?

(If it's a mess, I'm happy to push back against the whole architecture here of relying on referer for addon security, which strikes me as more than a little bit kludgy.)

Comment 14

[Dave Townsend [:mossop]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #13)
> (If it's a mess, I'm happy to push back against the whole architecture here
> of relying on referer for addon security, which strikes me as more than a
> little bit kludgy.)

To be clear regardless of the referrer the user is always asked if they want to install an add-on. We use the referrer to tell us if a site is allowed to start the download automatically or if the user has to even confirm at that point. If there is a better way from a content handler to get to the origin that triggered the request that that would be great.

Comment 15

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #13)
> Valentin, can you take a look and see if there's a reasonably clean way to
> get the internal referrer field to get set correctly for redirects to data:
> uris?

I think the problem here is the data URI. If I'm not mistaken, with a URI such as 'data:text/html,<script> window.location.href = "https://addons.mozilla.org" </script>' we don't get referrer headers, because it's different than clicking on a link or a redirect. It just sets the location, which makes it more similar to the user writing the URL in the URL bar.

> Basically, there is one exception in which the dialog will not be shown, which is if the user pastes or
> copies the direct link/URL in the URL bar.

We should probably get rid of this exception, since there is no difference between JavaScript using document.location to navigate to another page, and the user pasting in a URL.

Comment 16

[Jason Duell]

> If there is a better way from a content handler to get to the origin that triggered 
> the request that that would be great.

Tanvi and/or :ckerschb may have ideas here.  They've been adding this kind of info to channels.

Comment 17

[Dave Townsend [:mossop]]

(In reply to Valentin Gosu [:valentin] from comment #15)
> (In reply to Jason Duell [:jduell] (needinfo? me) from comment #13)
> > Basically, there is one exception in which the dialog will not be shown, which is if the user pastes or
> > copies the direct link/URL in the URL bar.
> 
> We should probably get rid of this exception, since there is no difference
> between JavaScript using document.location to navigate to another page, and
> the user pasting in a URL.

There is a difference as far as the user is concerned. In one case the user explicitely performed an action, in the other the website did it possible automatically.

The whitelist is intended to stop websites from being able to automatically spam the user with add-on install requests.

Comment 18

[Tanvi Vyas[:tanvi]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #16)
> > If there is a better way from a content handler to get to the origin that triggered 
> > the request that that would be great.
> 
> Tanvi and/or :ckerschb may have ideas here.  They've been adding this kind
> of info to channels.

Dave said we use the referrer to tell us if a site is allowed to start the download automatically.  We want to allow the download to start automatically when the user has typed in or pasted in the url, but not when script within a page has changed document.location.

I'm not sure this is possible.  In loadinfo, we have a loadingPrincipal and a triggeringPrincipal.  In bug https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c12 we are going to change the way we set those principals for top level loads.  Given the proposal outlined there, the triggeringPrincipal (which is essentially equivalent to the referrer) will not be null when a user types in a url but instead will be the codebase principal from the url the user typed.  For now, we are only fixing the loading/triggeringPrincipals.  But eventually we will try and fold the referrer logic into triggeringPrincipal, in which case checking for a null referrer will not work.

Perhaps we need to rework the proposal in bug 1105556 to cover this case and have a slightly different triggeringPrincipal (or flag set on loadinfo) when a user types in a url for a top level load.

Is there another way to determine that the load was initiated by the user outside of using referrer/loadInfo/triggeringPrincipals?

Comment 19

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

I think this could probably be done in nsLocation::CheckURL()
I don't think we can use the referrer for it, but it might be OK to pass along the triggeringPrincipal, and fix this for data URI redirects ( I don't know that code well enough to do it myself )
I haven't checked, but HTML meta http-equiv="refresh" might also suffer from the same problem.

Comment 21

[Jason Duell]

Ollie,

Do you know someone who could make the changes in dom/nsLocation for this?  Talking to Valentin, it sounds like passsing in the triggeringPrincipal is easy enough, but he's worried about whether we'd be changing some of the assumptions about window.location, so we should have someone who knows that code well.

Comment 22

[Jason Duell]

Valentin: "my concerns with this bug is if passing the principal every time, there could be some XSS attack vectors I'm not aware of since nsLocation is usable in JavaScript."

Comment 23

[Daniel Veditz [:dveditz]]

Jason: Olli is on vacation this week, who else can we ask?

Comment 24

[Dave Townsend [:mossop]]

I'd like to be sure we know what "FIXED" looks like here. I believe it is correctly applying the whitelist and showing the doorhanger to allow the download to proceed instead of proceeding directly to the install, but is there something else we expect?

Comment 25

[Andrew McCreight [:mccr8]]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> Jason: Olli is on vacation this week, who else can we ask?

Maybe Blake? Looks like bholley is mostly out this week.

Comment 26

[Anne (:annevk)]

The current setup is broken from a security perspective. We shouldn't rely on ambient authority as we'll keep patching holes. Referrers are interesting for analysis, but not security decisions.

We should definitely have follow up bugs to fix AMO per comment 6.

Also, if we want to pursue this referrer madness, I think we want a follow up bug there too to make sure it gets standardized, otherwise Servo sure is out of luck. And are we sure all the other ways to programmatically navigate are covered? E.g.,

  data:text/html,<a href=...></a><script>document.links[0].click()</script>
  data:text/html,<form action=...></form><script>document.forms[0].submit()</script>
  data:text/html,<meta http-equiv=refresh content=0;url=...>

Soon you can navigate a window through a service worker too...

Comment 27

[Dave Townsend [:mossop]]

(In reply to Anne (:annevk) from comment #26)
> The current setup is broken from a security perspective. We shouldn't rely
> on ambient authority as we'll keep patching holes. Referrers are interesting
> for analysis, but not security decisions.

Do we have a better option available at the moment?

Comment 28

[Anne (:annevk)]

I tend to think we should just ask permission even if you navigate directly to an XPI. Social media has seen its share of attacks using "bookmarklets". I don't see why that would not be similarly abused for this. "Use Firefox? Copy this URL into your address bar to change the Facebook logo into a kitten!"

Comment 29

[Dave Townsend [:mossop]]

(In reply to Anne (:annevk) from comment #28)
> I tend to think we should just ask permission even if you navigate directly
> to an XPI. Social media has seen its share of attacks using "bookmarklets".
> I don't see why that would not be similarly abused for this. "Use Firefox?
> Copy this URL into your address bar to change the Facebook logo into a
> kitten!"

The "allow" doorhanger is mainly designed to make it impossible for a website to spam a user with add-on install requests. AMO is the only site that can bypass it by default, that and user interaction (except here we can't tell it wasn't user interaction). Regardless of whether we show the allow doorhanger users still have to approve the installation so it isn't directly abusable to skip this doorhanger for that case

Comment 30

[Anne (:annevk)]

Maybe I'm doing something wrong, but on AMO I get the doorhanger. If I copy-and-paste a URL I don't. (Nightly, OS X)

Comment 31

[Dave Townsend [:mossop]]

Go to preferences - security - exceptions. AMO and Marketplace sites should be listed in there. If they aren't then maybe you cleared those at some point?

Comment 32

[Anne (:annevk)]

Seems like it. Sorry.

Comment 33

[Jason Duell]

If we need a fix for this quickly, could we just always show the doorhanger?  It doesn't seem like an intolerable UX experience for people who directly paste in a URI.  This seems like a lot of security hole-age for a small amount of UX win.

Comment 34

[Daniel Veditz [:dveditz]]

There are two key parts here. The worse one would not be addressed by your proposal and that's showing the doorhanger(s) on a different site after a navigation. Even in a legit http site to http site transition that's spoofy.

But yes, the other part is we're doing a lot of work to support developers being able to type in .XPI URLs into the URL bar. Always showing a (different) prompt would be one fix. That is, it shouldn't say "This site" is doing anything! It should say something that means "You appear to be manually installing an add-on, is that what you wanted to do?"

Another option I proposed on a mail thread was kill that feature. It's super obscure and only legitimately used by developers. Useful, but dangerous if non-devs get tricked into it. So stop it: if we can't get an internal referrer then we drop the install on the floor. We may find some legit case we break and can fix bugs later, but we're intentionally removing the feature that lets people type in .XPI urls.

Replacement functionality of a sort already exists -- if developers first download the .xpi they can drag-n-drop the file onto a browser window, or use the "Install from File" menu item on the Addon Manager. (We'd have to make sure those both continue to work, they won't have internal referrers either.)

We could also add a GCLI install command for developers. It'd be the same functionality they're using now, but they'd have to open up the command-line first (Shift-F2) and prefix the URL with a command like "install" or "addon".

Not directly implicated in this bug's testcase, but we should also prevent non-same-origin frames from triggering an install. Yes, the domain at the top of the prompt will be different but users are just going to see "This Site" and miss those fine details. The top level site might have put the iframe there, or the iframe could have been injected or maliciously navigated. We also might want this block to apply only to the pre-install permission prompt, and let whitelisted domains (AMO, marketplace) trigger an install itself.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

It appears that Mossop will be working on a solution that has been discussed in an email thread, so I'm giving the bug to him.

We should definitely file a followup bug on switching to triggeringPrincipal and fixing nsLocation to pass through the triggering principal correctly. I don't see any reason not to (responding to the concern in comment 21).

Comment 37

[Dave Townsend [:mossop]]

Attachment: patch

This implements the main fixes discussed in the email. I did end up using the triggering principal instead of the internal referrer since it made things easier in the end.

* Non-same-origin frames are denied the ability to initiate installs.
* Installs triggered by the system principal are still allowed, it turns out that dragging a url or XPI into the content area does this so that keeps working. I've only tested that on OSX though, drag and drop is fickle so it might be broken on other platforms.
* Installs triggered by a moznull principal are denied, the spoof testcase here and entering an XPI url into the address bar fall into that category. It seems to me that we could make entering a url into the address bar use the system principle which would mean we could keep that working, I didn't bother trying that though as I have no idea about the implications.
* If the browser navigates to a new origin any pending installs are immediately cancelled and there is code there to make the doorhangers and install prompts hide themselves in that case.

The browser_navigateaway3 test I added seems to be racy. I'm pretty sure it is a test problem so shouldn't need to hold review while I try to fix that. Worst case we can probably disable that test in the cases where it is failing.

I've kept API changes to a minimum, the AddonManager APIs that were public of sorts still accept an nsIURI and convert to a principal that will behave as things used to. I don't expect many will be using this but wanted to keep it compatible for the uplift, we can drop the API support on trunk.

A number of tests that call browser.loadURI with a direct link to an XPI (which ends up using the system principal) had to be altered to load some page first, this is because in e10s browser.contentPrincipal is undefined until a page load has happened.

When we block the user entering a url in the address bar I think we need to show some UI or it will be confusing. For now this patch just shows the standard blocked message (https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#22) but without the allow button. Ideally we'd get a new string but I'm assuming that would be hard for uplifts. It also shows for this spoofing case which isn't ideal. It also shows the cross-origin install attempts right now but we can turn that off if we want.

Comment 38

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #37)
> * If the browser navigates to a new origin any pending installs are
> immediately cancelled and there is code there to make the doorhangers and
> install prompts hide themselves in that case.

Is this something that will be an issue for Fennec?

Comment 39

[Daniel Veditz [:dveditz]]

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

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

The comments below are questions or about comments. If you want to address them you can carry over my r+
r=dveditz

::: browser/base/content/browser-addons.js
@@ +259,5 @@
>                                action, null, options);
>        break; }
> +    case "addon-install-origin-blocked": {
> +      // The URI doesn't make sense in this case
> +      if (!installInfo.originatingURI.schemeIs("moz-nullprincipal"))

Is this the only edge case? What if some page frames about:home and there's a snippet trying to launch an install? Not sure that could actually happen, but there might be other weird combos.

Would it be a better test to see if the URI has a host instead? Maybe not, I think blob: urls have hosts now, although if your frame was loaded from a blob: presumably the _principal_ would be your origin and not the literal blob: (just as the moz-nullprincipal: is not the actual URL in the frame, it's the principal).

I guess the odd schemes will mostly only come up for add-ons or internal functionality--like if the PDF viewer tries to launch installs (unlikely!). moz-nullprincipal can come up commonly in web content in sandboxed iframes.

@@ +277,5 @@
>        messageString = gNavigatorBundle.getFormattedString("xpinstallPromptMessage",
>                          [brandShortName]);
>  
>        let secHistogram = Components.classes["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry).getHistogramById("SECURITY_UI");
> +      if (aTopic == "addon-install-blocked") {

how could aTopic have changed between evaluating the switch and here three statements later?

::: toolkit/mozapps/extensions/test/xpcshell/test_permissions.js
@@ +25,5 @@
>  
>    startupManager();
>  
>    do_check_false(AddonManager.isInstallAllowed(XPI_MIMETYPE,
> +                                               newPrincipal("http://test1.com")));

Most of these appear flush left like the old NetUtil.newURI() calls but three of them are argument-aligned. (I don't feel strongly, just wanted to point it out in case you did and it was a mistake.)

::: toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js
@@ +1,2 @@
> +// ----------------------------------------------------------------------------
> +// Ensure that an inner frame from a different origin can't initiate an install

Is this comment misdirection? We won't know if this install is blocked because it's triggered from a data: url or because it's a non-same-origin frame. Maybe the comment is just copy-pasta from browser_unsigned_trigger_xorigin.js?

::: toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js
@@ +3,1 @@
>  // This verifies bugs 473060 and 475347

Doesn't this test now test the opposite? Does this un-fix the above two bugs (or at least 473060)? It may be what we have to do but if so we should change the rest of the comment.

Or, do we consider them still fixed because it's canceled cleanly, not with the errors (or crash) described in those bugs?

::: toolkit/mozapps/extensions/test/xpinstall/browser_unsigned_trigger_xorigin.js
@@ +1,2 @@
> +// ----------------------------------------------------------------------------
> +// Ensure that an inner frame from a different origin can't initiate an install

Since this is installing an unsigned package, how can you test whether the install fails because it was cross-origin or because it was unsigned? I see the install_blocked() event handler but it's not saving or checking anything. Would it throw an unsigned error (if it got that far) because there's no handler defined for that one?

Comment 40

[Dave Townsend [:mossop]]

(In reply to Daniel Veditz [:dveditz] from comment #39)
> Comment on attachment 8650158 [details] [diff] [review]
> patch
> 
> Review of attachment 8650158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The comments below are questions or about comments. If you want to address
> them you can carry over my r+
> r=dveditz
> 
> ::: browser/base/content/browser-addons.js
> @@ +259,5 @@
> >                                action, null, options);
> >        break; }
> > +    case "addon-install-origin-blocked": {
> > +      // The URI doesn't make sense in this case
> > +      if (!installInfo.originatingURI.schemeIs("moz-nullprincipal"))
> 
> Is this the only edge case? What if some page frames about:home and there's
> a snippet trying to launch an install? Not sure that could actually happen,
> but there might be other weird combos.
> 
> Would it be a better test to see if the URI has a host instead? Maybe not, I
> think blob: urls have hosts now, although if your frame was loaded from a
> blob: presumably the _principal_ would be your origin and not the literal
> blob: (just as the moz-nullprincipal: is not the actual URL in the frame,
> it's the principal).
> 
> I guess the odd schemes will mostly only come up for add-ons or internal
> functionality--like if the PDF viewer tries to launch installs (unlikely!).
> moz-nullprincipal can come up commonly in web content in sandboxed iframes.

Looks like PopupNotifications already handles the case where the uri has no valid host so I just removed this check.

> @@ +277,5 @@
> >        messageString = gNavigatorBundle.getFormattedString("xpinstallPromptMessage",
> >                          [brandShortName]);
> >  
> >        let secHistogram = Components.classes["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry).getHistogramById("SECURITY_UI");
> > +      if (aTopic == "addon-install-blocked") {
> 
> how could aTopic have changed between evaluating the switch and here three
> statements later?

Left over from a previous version of the patch, removed.

> ::: toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js
> @@ +1,2 @@
> > +// ----------------------------------------------------------------------------
> > +// Ensure that an inner frame from a different origin can't initiate an install
> 
> Is this comment misdirection? We won't know if this install is blocked
> because it's triggered from a data: url or because it's a non-same-origin
> frame. Maybe the comment is just copy-pasta from
> browser_unsigned_trigger_xorigin.js?

Yeah bad copy-paste, corrected.

> ::: toolkit/mozapps/extensions/test/xpinstall/browser_navigateaway2.js
> @@ +3,1 @@
> >  // This verifies bugs 473060 and 475347
> 
> Doesn't this test now test the opposite? Does this un-fix the above two bugs
> (or at least 473060)? It may be what we have to do but if so we should
> change the rest of the comment.
> 
> Or, do we consider them still fixed because it's canceled cleanly, not with
> the errors (or crash) described in those bugs?

Removed that piece of the comment.

> toolkit/mozapps/extensions/test/xpinstall/browser_unsigned_trigger_xorigin.js
> @@ +1,2 @@
> > +// ----------------------------------------------------------------------------
> > +// Ensure that an inner frame from a different origin can't initiate an install
> 
> Since this is installing an unsigned package, how can you test whether the
> install fails because it was cross-origin or because it was unsigned? I see
> the install_blocked() event handler but it's not saving or checking
> anything. Would it throw an unsigned error (if it got that far) because
> there's no handler defined for that one?

Added a check that the install_blocked handler is called. We should also be covered because signing requirements are disabled in these tests and we have other checks that installing the same file works normally.

Comment 41

[Dave Townsend [:mossop]]

Attachment: patch

Addressed the comments and fixed browser_navigateawa3.js by splitting out the checking of the install UI closing itself into browser_navigateaway4.js

Comment 42

[Daniel Veditz [:dveditz]]

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

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

Thanks! r=dveditz

Comment 43

[Daniel Veditz [:dveditz]]

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

sec-approval = dveditz

Comment 44

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/9d2d9b4a95b9

Comment 45

[:Margaret Leibovic]

(In reply to Dave Townsend [:mossop] from comment #38)
> (In reply to Dave Townsend [:mossop] from comment #37)
> > * If the browser navigates to a new origin any pending installs are
> > immediately cancelled and there is code there to make the doorhangers and
> > install prompts hide themselves in that case.
> 
> Is this something that will be an issue for Fennec?

I haven't looked at this patch closely - are you concerned that your changes will break Fennec in some way, or are you worried that we'll need additional patches to also fix this issue on Fennec?

Is there something I can do to test if this is a problem on Fennec? I'm also happy to write a patch if we need to update our notification UI as well.

Comment 46

[Dave Townsend [:mossop]]

(In reply to :Margaret Leibovic from comment #45)
> (In reply to Dave Townsend [:mossop] from comment #38)
> > (In reply to Dave Townsend [:mossop] from comment #37)
> > > * If the browser navigates to a new origin any pending installs are
> > > immediately cancelled and there is code there to make the doorhangers and
> > > install prompts hide themselves in that case.
> > 
> > Is this something that will be an issue for Fennec?
> 
> I haven't looked at this patch closely - are you concerned that your changes
> will break Fennec in some way, or are you worried that we'll need additional
> patches to also fix this issue on Fennec?
> 
> Is there something I can do to test if this is a problem on Fennec? I'm also
> happy to write a patch if we need to update our notification UI as well.

The specific part I was talking about is that if the user navigates to a new site from the tab that started an extension install the install will be cancelled. I don't really remember what Fennec's install UI looks like so I'm not sure if that is a problem or not.

Comment 47

[:Margaret Leibovic]

Attachment: Fennec add-on install UI

Our add-on install UI looks like this. I just tested what happens if I change window.location while this prompt is showing, and it dismisses the prompt, so I think we're okay?

Comment 48

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/9d2d9b4a95b9

Comment 49

[Dave Townsend [:mossop]]

Attachment: ESR patch

This version applies to ESR code.

Comment 50

[Sylvestre Ledru [:Sylvestre]]

Dave, can we have the uplifts request asap? thanks
ESR 31 is now dead but we will have the question, is that affected?

Comment 51

[Daniel Veditz [:dveditz]]

Yes, ESR 31 is affected. The ESR38 patch, or something close to it, might work on 31.

Comment 52

[Dave Townsend [:mossop]]

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

Approval Request Comment
[Risks and why]: Average risk, there is a lot of code changing here but it is all well tested. Some functionality is intentionally disabled.
[String/UUID change made/needed]: One existing string is re-used in a similar context.

Comment 53

[Dave Townsend [:mossop]]

Attachment: beta patch

Same patch but applies cleanly to beta

Comment 54

[Dave Townsend [:mossop]]

Attachment: release patch

Same patch but applies cleanly to release

Comment 55

[Dave Townsend [:mossop]]

Comment on attachment 8652099 [details] [diff] [review]
ESR patch

Forgot that there is one difference here. The string displayed when we block is different on ESR and attempts to include the hostname blocked, but in many cases we don't have one so we get a weird popup like this:

https://www.dropbox.com/s/anc5c3tdzasybk2/Screenshot%202015-08-25%2012.49.34.png?dl=0

Changing that would mean a string change and I don't know how feasible that is.

Comment 56

[Daniel Veditz [:dveditz]]

Comment on attachment 8652099 [details] [diff] [review]
ESR patch

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

r=dveditz

Comment 57

[Dave Townsend [:mossop]]

Comment on attachment 8652099 [details] [diff] [review]
ESR patch

Same as previous patches but applies to esr38

Comment 58

[Daniel Veditz [:dveditz]]

Comment on attachment 8652099 [details] [diff] [review]
ESR patch

a=dveditz for ESR-38

Comment 59

[Daniel Veditz [:dveditz]]

Comment on attachment 8652513 [details] [diff] [review]
beta patch

a=dveditz for beta, aurora, and release

Comment 60

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/05f2f11077f5

Comment 61

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f20947224b7

Comment 62

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-release/rev/d1dc3b231922

Comment 63

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/5713bf3a152e (ESR 38.2.x)
https://hg.mozilla.org/releases/mozilla-esr38/rev/4abc250aa55c (ESR 38.3.x)

Comment 64

[Dave Townsend [:mossop]]

Attachment: ESR tests

Test cases missing from the ESR patch

Comment 65

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/573127a8200a
https://hg.mozilla.org/releases/mozilla-esr38/rev/60aec55a27cc

Comment 66

[Sylvestre Ledru [:Sylvestre]]

Do we know if thunderbird/seamonkey are affected by this issue?

Comment 67

[Nick Thomas [:nthomas] (UTC+12)]

Transplanted onto the Firefox relbranch for esr38 (GECKO3820esr_2015080613_RELBRANCH):
http://hg.mozilla.org/releases/mozilla-esr38/rev/f1c2ced15323
http://hg.mozilla.org/releases/mozilla-esr38/rev/99ef2acc329b  (tests)

Comment 68

[Tanvi Vyas[:tanvi]]

I'm a little bit worried about the use of triggeringPrincipal.

Since I'm not too familiar with this area, I'll first summarize the behavior as I understand it.

* If you are on a whitelisted site, the user needs to go through one dialog when they try and install and addon.

* If a user tries to install an addon from a non-whitelisted site, they will get 2 dialogs (one before the actual download and one after)

* If a user uses the addons manager to download an addon, there is no top level domain to check against the whitelist.  So they check if the triggeringPrincipal is systemPrincipal.

* If a user tries to install an addon in any other way (copy/paste a url, data: uri, etc) it should fail.

The problem is that the triggeringPrincipal for top level loads could be system for other reasons.  Examples:
1) When opening a new blank tab
* loadingPrincipal is systemPrincipal - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is set to systemPrincipal (no aOwner)

2) When launching a bookmark OR refreshing a page.
* loadingPrincipal is systemPrincipal - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is set to systemPrincipal (no aOwner)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c10 and https://etherpad.mozilla.org/docShellPrincipals.

I want to change the value of triggerPrincipal for top level loads as described here https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c12, but in the meantime there may still be attack vectors.

When using the addons manager, is the referrer null?  Is there any other way to tell that this is where the load came from?

Comment 69

[Dave Townsend [:mossop]]

(In reply to Tanvi Vyas [:tanvi] from comment #68)
> I'm a little bit worried about the use of triggeringPrincipal.
> 
> Since I'm not too familiar with this area, I'll first summarize the behavior
> as I understand it.
> 
> * If you are on a whitelisted site, the user needs to go through one dialog
> when they try and install and addon.
> 
> * If a user tries to install an addon from a non-whitelisted site, they will
> get 2 dialogs (one before the actual download and one after)
> 
> * If a user uses the addons manager to download an addon, there is no top
> level domain to check against the whitelist.  So they check if the
> triggeringPrincipal is systemPrincipal.

No, the add-ons manager bypasses the code that handles installs from web pages.

> * If a user tries to install an addon in any other way (copy/paste a url,
> data: uri, etc) it should fail.

Dragging the XPI file or the url to one into the browser should still work.

> The problem is that the triggeringPrincipal for top level loads could be
> system for other reasons.  Examples:
> 1) When opening a new blank tab
> * loadingPrincipal is systemPrincipal - comes from
> mScriptGlobal->GetFrameElementInternal
> * triggeringPrincipal is set to systemPrincipal (no aOwner)

This would never cause an XPI install though right?

> 2) When launching a bookmark OR refreshing a page.
> * loadingPrincipal is systemPrincipal - comes from
> mScriptGlobal->GetFrameElementInternal
> * triggeringPrincipal is set to systemPrincipal (no aOwner)

That's surprising, so if the user bookmarked an url or refreshed a page that later changed to serving an XPI then it would bypass the whitelist. That's not great though I suspect that that might have been the case before the fix for this bug (the bookmark case for sure, I'm not sure what docshell.internalReferrer did on reloads).

> When using the addons manager, is the referrer null?  Is there any other way
> to tell that this is where the load came from?

When using the add-ons manager we install using a different path, the triggering principal is not checked in this case.

Comment 70

[Tanvi Vyas[:tanvi]]

(In reply to Dave Townsend [:mossop] from comment #69)
> (In reply to Tanvi Vyas [:tanvi] from comment #68)
> > * If a user uses the addons manager to download an addon, there is no top
> > level domain to check against the whitelist.  So they check if the
> > triggeringPrincipal is systemPrincipal.
> 
> No, the add-ons manager bypasses the code that handles installs from web
> pages.
>

> > When using the addons manager, is the referrer null?  Is there any other way
> > to tell that this is where the load came from?
> 
> When using the add-ons manager we install using a different path, the
> triggering principal is not checked in this case.

If we aren't checking triggeringPrincipal = systemPrincipal to preserve the behavior of add-ons manager, then what are we using that check for?  What use case does it support?

Thanks!

Comment 71

[Dave Townsend [:mossop]]

(In reply to Tanvi Vyas [:tanvi] from comment #70)
> (In reply to Dave Townsend [:mossop] from comment #69)
> > (In reply to Tanvi Vyas [:tanvi] from comment #68)
> > > * If a user uses the addons manager to download an addon, there is no top
> > > level domain to check against the whitelist.  So they check if the
> > > triggeringPrincipal is systemPrincipal.
> > 
> > No, the add-ons manager bypasses the code that handles installs from web
> > pages.
> >
> 
> > > When using the addons manager, is the referrer null?  Is there any other way
> > > to tell that this is where the load came from?
> > 
> > When using the add-ons manager we install using a different path, the
> > triggering principal is not checked in this case.
> 
> If we aren't checking triggeringPrincipal = systemPrincipal to preserve the
> behavior of add-ons manager, then what are we using that check for?  What
> use case does it support?

It preserves drag and drop of an XPI file into the browser. I was also hoping it might be a way to get the url bar method working again sometime in the future.

Comment 72

[Dave Townsend [:mossop]]

(In reply to Tanvi Vyas [:tanvi] from comment #70)
> If we aren't checking triggeringPrincipal = systemPrincipal to preserve the
> behavior of add-ons manager, then what are we using that check for?  What
> use case does it support?

Also to be clear we never check that the triggeringPrincipal is a system principal in particular, we just check that the triggeringPrincipal subsumes the principal of the document loaded in the tab that started the install.

In the case of the url bar entered install and the poc for this bug the triggering principal is a null principal so that test fails. For drag and drop it is the system principal and so passes. For any same-origin frame in the tab that test passes too.