948029 - [Download API] Downloading resources isn't driven by user consent

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Jason Smith [:jsmith]]

The downloading API currently does not distinguish between a download requested by a user vs. requested by a webpage. This risks a potential DOS attack in a couple of different ways:

1. I could create a malicious webpage that when visited, starts repeatedly clicking an anchor tag to download a 10 KB resource to the sdcard. If the content process isn't killed in enough time, then the sdcard will run out of space quickly. This would be a DOS attack against a user's space on the sdcard.

2. I could use the mozAlarms API to trigger off when a user is not actively using their phone to start downloading resources in the background by clicking an anchor tag repeatedly to download a 10 KB resource to the sdcard. In this situation, the user has a high risk of coming back to their device to find out that their sdcard space is gone, even though they didn't initiate the action. This is even worse in a case where the user muted their phone, as there will be no notification sound when a download starts & ends. This would additionally be a DOS attack against a user's space on the sdcard.

We need to ensure that downloading is only executed when user consent is granted. That means downloading should not happen in scenario #2 at all, as that was triggered by a background process via the mozAlarms API. That also means that scenario #1 shouldn't have downloading occur, as the web page triggered the download, not the user.

Comment 1

[Jason Smith [:jsmith]]

Paul & I discussed this offline. DOSs are typically sec-moderate, but this seems worse than a typical DOS mainly because the user isn't aware of what site/app initiated the download. This means the user is not aware of what site/app to avoid in the future.

On that regard, we're making this a sec-high security rating.

Comment 3

[bhavana bajaj [:bajaj]]

Blocking due to the sec-high rating.

Comment 4

[Candice Serran (:cserran)]

Will require UX feedback before we can execute on this.

Comment 5

[Candice Serran (:cserran)]

Hey Jonas, 

Can you please take a look at this and provide feedback. Jason raised in sprint planning as very high priority as it was found in security review.

Comment 6

[Francis Djabri [:djabber]]

Is there any way that the API can detect the difference between a user-initiated download and a download initiated by a web page? From a UX perspective, we should avoid prompting users to confirm downloads that they have already implicitly confirmed. E.g., if the user taps on a "Download" button within a page, this should act as a implicit confirmation. Fennec does not seem to prompt the user in this case - e.g., if I attempt to download a file on Dropbox, there is no confirmation prompt.

Comment 7

[(no longer active)]

Differentiating between a user initated download and a website initiated one can be tricky, since as far as I know we don't track where a network operation was initiated from.

What kind of UI do we show when initiating a download?  The way that this currently works on the desktop is that when a download is started, according to a user pref, we may put out a dialog to allow the user to confirm the download, but the download will progress in the background and if the user presses Cancel then we cancel the download and remove the downloaded file.  And it's possible to ask that dialog to not be shown in the future in which case the download would be silent.

Shouldn't we try to fail downloads if we're about to run out of disk space?

One other point to consider is that currently on Android, we don't prompt before downloading content, so we have the exact same problem there as well in a sense.

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #7)

> One other point to consider is that currently on Android, we don't prompt
> before downloading content, so we have the exact same problem there as well
> in a sense.

This is the reason why I didn't adding prompting when implementing the api initially. The user it notified through the notification area that something happens, it's not totally silent and can be cancelled.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #7)
> 
> > One other point to consider is that currently on Android, we don't prompt
> > before downloading content, so we have the exact same problem there as well
> > in a sense.
> 
> This is the reason why I didn't adding prompting when implementing the api
> initially. The user it notified through the notification area that something
> happens, it's not totally silent and can be cancelled.

Well, that workflow works if the phone is actively in use by the user, but it won't work if they aren't actively using their phone.

On that regard, one thing we should implement here is prevent a download from being initiated by a background process (i.e. app has to explicitly be in foreground to allow a download to be initiated). That would prevent the DOS attack in [2] in comment 0.

Comment 10

[(no longer active)]

(In reply to Jason Smith [:jsmith] from comment #9)
> On that regard, one thing we should implement here is prevent a download
> from being initiated by a background process (i.e. app has to explicitly be
> in foreground to allow a download to be initiated). That would prevent the
> DOS attack in [2] in comment 0.

This makes sense to me.  I'm not sure if we should do anything else in the foreground case, but if we want to change anything we should loop the UX team in.

Comment 11

[Jason Smith [:jsmith]]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #10)
> (In reply to Jason Smith [:jsmith] from comment #9)
> > On that regard, one thing we should implement here is prevent a download
> > from being initiated by a background process (i.e. app has to explicitly be
> > in foreground to allow a download to be initiated). That would prevent the
> > DOS attack in [2] in comment 0.
> 
> This makes sense to me.  I'm not sure if we should do anything else in the
> foreground case, but if we want to change anything we should loop the UX
> team in.

Agreed. In [1]'s use case in comment 0, if a user saw a frantic amount of downloads starting to happen, then I think they would probably try to get out of the app as quick as possible. With preventing background processes from initiating a download, we'll then be able to get the user out of the continuous download situation as soon as they exit the app.

Looks like we've got a path forward here, so I'm going to clear needinfo on Jonas at this point, although he can feel free to add any additional input he has here if he wants to.

Comment 12

[Gregor Wagner [:gwagner]]

So the scope of this bug is to prevent a background process from starting a download?

Comment 13

[[:fabrice] Fabrice Desré]

If so, checking if the docshell is active should be good enough. See eg. https://mxr.mozilla.org/mozilla-central/source/dom/activities/src/Activity.cpp#43

Comment 14

[Jason Smith [:jsmith]]

(In reply to Gregor Wagner [:gwagner] from comment #12)
> So the scope of this bug is to prevent a background process from starting a
> download?

yes.

Comment 15

[Aus Lacroix [:aus]]

OK, now that we have the correct scope and goal for this bug, I should be able to get this done during this sprint.

Comment 16

[Stephany Wilkes]

Ehsan, I'm on the UX team. I wasn't able to see this bug before today, so I apologize for any delay. Please let me know if anything is still needed here from the UX side.

Comment 17

[(no longer active)]

Thanks Stephany!  Please see the previous comments in the bug, although I'm not sure if we're going to change the UX here...

Comment 18

[(no longer active)]

(I mean, not in this bug!)

Comment 19

[Stephany Wilkes]

Hi Ehsan: I've read all the comments but the UX need is still not clear to me. Flagging Francis on download manager anyway.

Comment 20

[(no longer active)]

The summary is that Jason was arguing that we should not allow websites to download files to the disk without the user's consent, and I made the case that this is what we already do on desktop and Android.  If you think that is not the right thing to do, we should probably coming up with the UX of how confirming the download before it starts should work.

Comment 21

[Stephany Wilkes]

Hence Francis being flagged, since this is his domain.

Comment 22

[Francis Djabri [:djabber]]

I agree with the approach that's been discussed - that is, preventing background processes from initiating a download will, but not changing anything in the foreground case.

Comment 23

[Aus Lacroix [:aus]]

:fabrice, :bz -- I understand how to accomplish this check, but I'm unsure at which level it should happen. It seems to me that since the URILoader will know if it's calling the ExternalHelperAppService it could perform the check there prior to starting the process of calling SaveToDisk.

Does that seem like the correct approach?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Should helper apps work for stuff in a background process?

Comment 25

[Aus Lacroix [:aus]]

I would say, yes, as long as when the helper app was requested to handle the content, the doc shell was active. I think it's reasonable to have the operation complete successfully afterwards even if the docshell is no longer active when the download completes.

Comment 26

[Aus Lacroix [:aus]]

The URILoader kicks off the whole process of dispatching the content to the ExternalHelperAppService here: http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#539

It looks like we could effectively do our check there?

Comment 27

[Boris Zbarsky [:bzbarsky]]

I think you misunderstood my question.

If a background app tries to trigger some content that we'd handle with a helper app, not just via downloading, what should happen?  Should we open it in a helper app or suppress it?

Comment 28

[Aus Lacroix [:aus]]

Oh! My apologies. That's a very good question. Let's see what UX thinks... I would be inclined to say that that type of scenario may need to work.

Comment 29

[Jason Smith [:jsmith]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #28)
> Oh! My apologies. That's a very good question. Let's see what UX thinks... I
> would be inclined to say that that type of scenario may need to work.

I'd actually say that would be more of a security question, but UX can weigh in here as well if they would like.

Comment 30

[Aus Lacroix [:aus]]

(In reply to Jason Smith [:jsmith] from comment #29)
> (In reply to Ghislain Aus Lacroix [:aus] from comment #28)
> > Oh! My apologies. That's a very good question. Let's see what UX thinks... I
> > would be inclined to say that that type of scenario may need to work.
> 
> I'd actually say that would be more of a security question, but UX can weigh
> in here as well if they would like.

Just chatted with Francis in person, and from his perspective, we could allow background apps to route content to external apps for handling, however, let's also see what security's perspective is.

Comment 31

[[:fabrice] Fabrice Desré]

Note that we have no external apps on b2g - but we should still do the right thing for other platforms where we could enable this api.

Comment 32

[Aus Lacroix [:aus]]

I asked Rob to chime in also, since he works right next to me it might be easier to discuss. :)

Comment 33

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jason Smith [:jsmith] from comment #29)
> (In reply to Ghislain Aus Lacroix [:aus] from comment #28)
> > Oh! My apologies. That's a very good question. Let's see what UX thinks... I
> > would be inclined to say that that type of scenario may need to work.
> 
> I'd actually say that would be more of a security question, but UX can weigh
> in here as well if they would like.

I have to admit I don't know much about apps on platforms other than b2g. I'm also not exactly sure what helper apps are, and the mdn page is from 2002 [1]. But it sounds like we are talking about either external programs that are launched based on MIME info. I assume for regular web pages, they can launch helper apps from the background (hidden tabs, or when Firefox doesnt have focus), so apps should be the same? In any case the original risk here related to exhausting disk space on the phone, and I can't see that same risk applying to desktop (especially given that on desktop we already automatically start some downloads automatically as discussed in 960739)

Just curious: do Apps even have a concept of foreground/background on desktop? I thought this was a gaia window manager concept.







[1] https://developer.mozilla.org/en/docs/Helper_Apps_%28and_a_bit_of_Save_As%29#_Helper_App_dialog_

Comment 34

[Rob Fletcher [:omerta]]

After having an awesome chat today with aus, I understand the issue more clearly
and wanted to summarize and get some feedback.

The first proposed mitgation is to only allow active docshell's to intiate
downloads. This sounds good, assuming we understand completely what an 'active
docshell' means.

Then bz brought up the good point at [1]:
If a background app tries to trigger some content that we'd handle with a helper
app, not just via downloading, what should happen?  Should we open it in a
helper app or suppress it?

After chatting with aus, I *think* I have a better understanding of the
situation and it seems like the answer should be:
We should suppress it.

As I understand it, external handling is more OS specific content handling and
suppressing background apps should not affect how apps commonly handle things
now; e.g. this is not WebActivities which is used mostly for that kind of thing.

A potential exploit scenario would be a malicious app requesting to open a PDF
in the Browser. If there is a known PDF exploit in the browser, this behavior
can be used to chain together vulnerabilities and exploit the user.

Seems like there are a lot of security wins in suppressing background apps from
opening content that will be handled by a helper app. If this does not greatly
affect developers coding process and existing application behavior, it seems
like a no brainer.

I'm needinfo'ing pauljt to get his opinion.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=948029#c27

Comment 35

[Boris Zbarsky [:bzbarsky]]

OK.  If you want to suppress both downloading and helper apps, the right way to do that is to not hand the content to the external helper app service at all, yes.

Comment 36

[Aus Lacroix [:aus]]

As soon as I get confirmation from Paul I will be writing up the patch. Shouldn't take too long.

Comment 37

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #36)
> As soon as I get confirmation from Paul I will be writing up the patch.
> Shouldn't take too long.

Sounds good to me.

Comment 38

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Ensure active DocShell before allowing external handling on B2G/Gonk

I'm unsure if returning NS_ERROR_DOM_SECURITY_ERR is correct.

Comment 39

[Aus Lacroix [:aus]]

Attachment: Patch - v1 - Ensure active DocShell before allowing external handling on B2G/Gonk

Comment 40

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Ensure active DocShell before allowing external handling on B2G/Gonk

* Added extra logging for debugging purposes.
* This was tested with a page that automatically triggers a download by simulating a click on a link in the document every 5 seconds. When I to another browser tab, or go a different application on the phone, the downloads stop being triggered as the DocShell is no longer active (and I get the expected failure on line 266 in nsURILoader.cpp).

Comment 41

[Aus Lacroix [:aus]]

Attachment: Test Page - Example test page that was used while testing with a device

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8390858 [details] [diff] [review]
Patch - v2 - Ensure active DocShell before allowing external handling on B2G/Gonk

>+#if defined(MOZ_WIDGET_GONK)

I'd prefer we just made this a pref.  Then we can set it in whatever apps want this behavior.

And fix the comments accordingly, of course.

>+  // See bug 948029 (http://bugzilla.mozilla.org/show_bug.cgi?id=948029).

That information will be in the commit message; no need to put it in the code too.

>+  nsCOMPtr<nsPIDOMWindow> window(do_GetInterface(m_originalContext));
>+  if (!window || !window->GetDocShell()) {

If we just want the docshell, why not:

  nsCOMPtr<nsIDocShell> docShell = do_GetInterface(m_originalContext);

?  In practice, m_originalContext _is_ a docshell, in fact.

If you do want to go via the window, the nsPIDOMWindow->IsBackground() should do what you want here.

The actual error code here doesn't matter too much, I suspect.  The one you're using is fine.

r=me with the above issues fixed.

Comment 43

[Aus Lacroix [:aus]]

Attachment: Patch - v3 - Ensure active DocShell before allowing external handling when disable background handling is true.

r+ carries from bz.

Comment 44

[Aus Lacroix [:aus]]

This additional security check can be turned on for all platforms if desired at a later time. For now, it is enabled on FxOS only by default (the pref name is "security.exthelperapp.disable_background_handling" for those of you who care).

Comment 45

[Aus Lacroix [:aus]]

Here's a snippet from the logging output showing it in action:

/PRLog   (18157): 1074799976[40322080]: [0x444f4dc0] nsDocumentOpenInfo::OnStopRequest
I/PRLog   (18157): 1074799976[40322080]: [0x44244ac0] nsDocumentOpenInfo::OnStartRequest
I/PRLog   (18157): 1074799976[40322080]:   HTTP response status: 200
I/PRLog   (18157): 1074799976[40322080]: [0x44244ac0] nsDocumentOpenInfo::DispatchContent for type ''
I/PRLog   (18157): 1074799976[40322080]:   Got type from channel: 'audio/mpeg'
I/PRLog   (18157): 1074799976[40322080]:   forceExternalHandling: yes
I/PRLog   (18157): 1074799976[40322080]:   Check for active DocShell returned false. Aborting hand off to helper app service.
I/PRLog   (18157): 1074799976[40322080]:   After dispatch, m_targetStreamListener: 0x0, rv: 0x80530012
I/Gecko   (18157): [Child 18157] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80530012: file /home/aus/Projects/mozilla-central/uriloader/base/nsURILoader.cpp, line 269

Comment 46

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/df384c7c679a

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 48

[Al Billings [:abillings - ex-MoCo]]

Did this bug only affect 30?

Comment 49

[Al Billings [:abillings - ex-MoCo]]

Do we need to take this on older branches? Did this affect ESR24 or was this particular to Firefox OS really?

Comment 50

[[:fabrice] Fabrice Desré]

It's fxOS specific, we never exposed this api in other products.

Comment 51

[Aus Lacroix [:aus]]

Indeed, the Downloads API is only available in FxOS.

This issue itself could, in theory, be reproducible on v28 or less by enabling automatic downloads (it's possible to do this pretty easily afaik and it's on by default in Firefox 28). A malicious webpage could trigger as many downloads as it wants despite not having user focus.

I'm not sure it's worth taking though as I don't think it's been a reported issue.

Comment 52

[Kamil Jozwiak [:kjozwiak]]

I went through this on fx desktop and noticed that if a user has already selected to "automatically download" that specific file type, a malicious website can keep triggering the download despite not having any user focus. If a user has downloads set to "Ask Me", they'll just get bombarded with download prompts.

- had multiple tabs opened (download kept triggering despite tab not being in focus)
- minimized fx desktop and used another application (download kept triggering despite fx desktop not being in focus)
- switched over to the Metro environment and used another metro application (download kept triggering despite fx desktop not being in focus)

If "browser.download.animateNotifications" happens to be turned off, the user would have a difficult time noticing what was happening until they browsed to the download directory. 

When I tried this on Chrome/Canary, I received a notification at the top that let me know that this particular website is attempting to download multiple files and gave me a choice of Allow/Deny. If you select "Allow", all the ones that have been downloaded in the background are saved.

Should we implement something similar? If a website requests multiple downloads before x amount of seconds, we'll prompt the user and let them know even though they have automatic downloads enabled?