827023 - The App Install Prompt isn't modal to an app - which allows for a webpage to forcibly annoy a user across the phone until an app is installed

Status: VERIFIED FIXED

(Core Graveyard :: DOM: Apps, defect, P3)

Description

[Jason Smith [:jsmith]]

Attachment: Proof of Concept Test Case

Steps:

1. Load http://jds2501.github.com/webapi-permissions-tests/repeatedinstall.html in the browser
2. Cancel the install
3. Exit to the homescreen

Expected:

The app install prompt shouldn't appear on top of the homescreen - it should remain modal to the browser.

Actual:

The app install prompt appears on top of the homescreen or generally, any part of the phone every time you try to cancel install.

This is a security problem - an evil web developer could essentially force a user into installing their evil app if they setup a test page like what's seen attached to repeatedly annoy the user across the phone until they install the app.

This is the app install prompt equivalent of bug 826023. The more I see these type of bugs, the more I think we need to audit our system prompts to make sure they are modal.

Comment 1

[Jason Smith [:jsmith]]

Actually this doesn't need to be security sensitive given that the other equivalent bug isn't either.

Comment 2

[Jason Smith [:jsmith]]

Actually let's stick this back under security - it's sec-high because it's forcing the user to do an install per talking with Paul.

Comment 3

[Etienne Segonzac (:etienne)]

Talked with Fabrice, his opinion is: this is more of a global issue (same for starting web activities and popups) that shouldn't block.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Etienne Segonzac (:etienne) from comment #3)
> Talked with Fabrice, his opinion is: this is more of a global issue (same
> for starting web activities and popups) that shouldn't block.

Why? If it's a global issue that would be more likely to imply that it blocks.

The fact that you can force a user into installing apps diminishes a user's choice onto how they control their device. Which is why it was marked sec-high.

I can definitely through together a more dangerous PoC pretty easily that could probably start an endless loop of installing apps I want on the user's device until I take control of all of the storage on the device.

Comment 5

[Jason Smith [:jsmith]]

I should note that this does *not* happen with alerts/confirms because we made sure those were modal.

Comment 6

[Etienne Segonzac (:etienne)]

(In reply to Jason Smith [:jsmith] from comment #5)
> I should note that this does *not* happen with alerts/confirms because we
> made sure those were modal.

True.
Sadly the installation prompts come from a mozChromeEvent (as opposed to a mozbrowser event on the iframe calling it) so we don't know with app/browser tab to attach it to.

This should be triaged in platform triage.

Comment 7

[Julien Wajsberg [:julienw]]

Could the work on Bug 826023 fix this bug too ? Or am I completely out of here ?

Comment 8

[Doug Turner (:dougt)]

Not blocking v.1 on this bug.

We probably will want UX input on this bug so that we know how to correct address this problem (maybe it is just making this dialog modal to the browser app)

Comment 9

[Jason Smith [:jsmith]]

(In reply to Doug Turner (:dougt) from comment #8)
> Not blocking v.1 on this bug.
> 
> We probably will want UX input on this bug so that we know how to correct
> address this problem (maybe it is just making this dialog modal to the
> browser app)

The fact that you can forcibly get app installations on a user's device in a repeated chain with uncontrolled user consent doesn't sound like something we could ship with. I still think this blocks.

And I have a feeling I could pull off this bug in an app as well.

Renom.

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Jason Smith [:jsmith] from comment #9)

> The fact that you can forcibly get app installations on a user's device in a
> repeated chain with uncontrolled user consent doesn't sound like something
> we could ship with. I still think this blocks.

You don't get the app installed, but only the install dialog to show up, right?

Comment 11

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> (In reply to Jason Smith [:jsmith] from comment #9)
> 
> > The fact that you can forcibly get app installations on a user's device in a
> > repeated chain with uncontrolled user consent doesn't sound like something
> > we could ship with. I still think this blocks.
> 
> You don't get the app installed, but only the install dialog to show up,
> right?

The install dialog, right. It doesn't automatically install the app. The user would still have to hit install. In the case of the PoC I'm describing, if I hit cancel, I'll just prompt to install again across any part of the phone.

If I was to do a repeated chain, I would just start another app install immediately after the first app is install. So the evil web developer once a user visits their target page can control what the user installs onto their device with uncontrolled user consent (i.e. cancel always reprompts).

I'll experiment to see what work-arounds are available. If I'm able to kill the phone easily when this happens, then I think it's worth reanalyzing to see if this blocks (in other words, how easily can you hit the eject button if you end up in this state).

Comment 12

[Jason Smith [:jsmith]]

Actually the eject button does exist here - if the user entered this state and wanted to hit eject, they could hold the power button down and power down or restart the phone.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #12)
> Actually the eject button does exist here - if the user entered this state
> and wanted to hit eject, they could hold the power button down and power
> down or restart the phone.

Talked with Lucas.

There's two eject buttons here - power down or restart the phone or kill the app process through the task manager.

Comment 14

[Lucas Adamski [:ladamski]]

I tested this on my phone and its pretty obnoxious.  The task manager is the easiest workaround but its clearly not obvious.  I think the gecko part of the fix for bug 826023 will solve most of the problem, and then we just need to implement the getVisible() check in Gaia.  So I'm going to renominate this as a Gaia bug and recommend basecamp-blocking, dependent on 826023 sticking and Etienne's feedback to confirm/deny my theory.

Comment 15

[Etienne Segonzac (:etienne)]

(In reply to Lucas Adamski from comment #14)
> I tested this on my phone and its pretty obnoxious.  The task manager is the
> easiest workaround but its clearly not obvious.  I think the gecko part of
> the fix for bug 826023 will solve most of the problem, and then we just need
> to implement the getVisible() check in Gaia.  So I'm going to renominate
> this as a Gaia bug and recommend basecamp-blocking, dependent on 826023
> sticking and Etienne's feedback to confirm/deny my theory.

Not prompting for install if the frame is hidden should be pretty straightforward (and I'm happy to do it), but I think it'll still need to live in dom/app (not gaia).

Fabrice will confirm.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just discussed with Fabrice and this is likely dom/apps.

Comment 17

[[:fabrice] Fabrice Desré]

Attachment: patch

This patch prevents install() and installPackage() to work on background pages.

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

r+ if you rename the isForeground -> to ensureForeground (otherwise the method is lying)

Comment 19

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65517a277d6b

Comment 20

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/26b810ee919d

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Hasn't hit m-c yet.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Ryan VanderMeulen from comment #21)
> Hasn't hit m-c yet.

For b2g work week we are closing bugs when they hit inbound + b2g18. Reclosing.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Ryan VanderMeulen from comment #21)
> > Hasn't hit m-c yet.
> 
> For b2g work week we are closing bugs when they hit inbound + b2g18.
> Reclosing.

FWIW, I don't know what sense this makes. What does resolving the bug do (and by extension throwing out conventions that have been around for years) that status flags can't accomplish? We have status-b2g18:fixed specifically for tracking branch uplifts.

https://hg.mozilla.org/mozilla-central/rev/65517a277d6b

Comment 24

[Jason Smith [:jsmith]]

(In reply to Ryan VanderMeulen from comment #23)
> (In reply to Jason Smith [:jsmith] from comment #22)
> > (In reply to Ryan VanderMeulen from comment #21)
> > > Hasn't hit m-c yet.
> > 
> > For b2g work week we are closing bugs when they hit inbound + b2g18.
> > Reclosing.
> 
> FWIW, I don't know what sense this makes. What does resolving the bug do
> (and by extension throwing out conventions that have been around for years)
> that status flags can't accomplish? We have status-b2g18:fixed specifically
> for tracking branch uplifts.
> 
> https://hg.mozilla.org/mozilla-central/rev/65517a277d6b

It's some work week special case for tracking - they wanted bugs closed if they hit b2g18 (the branch that we ship with). I agree this doesn't work on normal circumstances though.

Comment 25

[Julien Wajsberg [:julienw]]

I agree with Ryan here; focusing on numbers is so bad. Well well.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jason Smith [:jsmith] from comment #24)
> It's some work week special case for tracking - they wanted bugs closed if
> they hit b2g18 (the branch that we ship with). I agree this doesn't work on
> normal circumstances though.

I guess what I'm failing to understand is what this is even trying to accomplish. Presumably they're trying to track their remaining blockers. Presumably they're using bug queries for that. A bug query can just as easily use the status flag as the bug resolution. I can understand the need to streamline the landing & uplift process under the tight deadlines posed here, but I fail to see how changing this specific practice does anything to aid that cause.

Comment 27

[Jason Smith [:jsmith]]

Verified on today's build. Looks good!