1046941 - Disable downloads and extension installs in guest mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

With the lockscreen widget, I don't think if we should allow you to download anything (or install extensions that could do malicious things) when you're in Guest mode.

I'm not sure if this lockscreen guest mode is different than normal guest mode or not in that sense though...

Comment 1

[:Margaret Leibovic]

I think this makes sense.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

This is the same thing we do for webapps.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebappRT.js#28

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch 2 - Disable downloads

Comment 4

[:Margaret Leibovic]

Comment on attachment 8471340 [details] [diff] [review]
Patch

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

::: mobile/android/chrome/content/browser.js
@@ +448,5 @@
>  
>      if (this._startupStatus)
>        this.onAppUpdated();
>  
> +    if (isGuest) {

I think this needs to be `this.isGuest`.

Did you test this? :)

@@ +452,5 @@
> +    if (isGuest) {
> +      // Disable extension installs
> +      Services.prefs.setIntPref("extensions.enabledScopes", 1);
> +      Services.prefs.setIntPref("extensions.autoDisableScopes", 1);
> +      Services.prefs.setBoolPref("xpinstall.enabled", false);

I suppose it's fine to have prefs that persist, since they'll be written to the guest profile we're throwing away anyway.

Do we just create a new regular profile for guest mode? Is there any special logic specific to creating guest profiles? If so, maybe it would be better to put this closer to that logic, but I'm not sure if that's possible.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8471341 [details] [diff] [review]
Patch 2 - Disable downloads

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

Looks good to me. Make sure you get UX review on this toast behavior/string.

::: mobile/android/chrome/content/browser.js
@@ +448,5 @@
>  
>      if (this._startupStatus)
>        this.onAppUpdated();
>  
> +    if (this.isGuest) {

Oh, heh, you fixed it here.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8471340 [details] [diff] [review]
Patch

Bumping to r+, since you fixed the issue in the next patch. You can just fold them together when you land.

Comment 7

[Richard Newman [:rnewman]]

If rely on prefs to block extension installs, you'd better make sure I can't go to about:config in Guest Mode… otherwise I'm just going to turn off your blocks and install my malicious add-on.

I noticed in Bug 1052254 that we should probably have some guest mode short-circuiting in the file analysis code in HelperAppDialog. Linking these two bugs.

Comment 8

[:Margaret Leibovic]

(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #7)
> If rely on prefs to block extension installs, you'd better make sure I can't
> go to about:config in Guest Mode… otherwise I'm just going to turn off your
> blocks and install my malicious add-on.

That is a very good point. Do we have logic in place to block changing prefs in a guest profile? Can we block about:config? We also wouldn't want a malicious person to be able to turn on remote debugging, although hopefully in this lock screen mode the device would block USB debugging.

Comment 9

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #8)
> Can we block about:config?

That's why I suggested blocking about: pages in Bug 1052254.

Comment 10

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/12e582732409

I'm going to file a follow up for doing this extension blocking more robustly (and with some better error reporting).

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/12e582732409

Comment 12

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in builds:
Firefox for Android 34.0a2 (2014-09-29)
Firefox for Android 35.a01 (2014-09-29)

Device: Nexus 4 Android 4.4.4

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

I just installed LastPass using the lockscreen/guest-mode. From the homescreen, I clicked on Add-ons, then installed LastPass. The browser restarted, and I went to about:support and saw that LastPass was installed and enabled.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

But then when I exit guest mode, the extension is no longer listed. Are we sandboxing extension installs?

Comment 15

[Richard Newman [:rnewman]]

The guest profile is deleted when you exit guest mode, along with its add-ons.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Richard Newman [:rnewman] from comment #15)
> The guest profile is deleted when you exit guest mode, along with its
> add-ons.

So the bug summary (disable ... extension installs in Guest Mode) is a bit misleading. They're not truly disabled, they just don't live past exiting guest mode. Was this intentional, or did we want to actually block installation of them in guest mode too?

Comment 17

[Richard Newman [:rnewman]]

They're *supposed* to be blocked, so this bug is not resolved. I just reproduced in current Nightly 35.

Wes, would you rather reopen this or file another bug?

Comment 18

[Wesley Johnston (:wesj)]

Reopen is fine. They should be blocked. Something is up... maybe RestrictedProfiles broke this.

Comment 19

[Richard Newman [:rnewman]]

See also Bug 1077706, which would help avoid the temptation!

Comment 20

[Cristina Madaras, QA [:CristinaM]]

On Aurora (2014-10-05), when I try to download a file in gust mode 'Downloads are disabled in guest mode' toast notification is displayed. When I tap on 'Add to Firefox' for installing an add-on, nothing happens, the add-on is not downloaded.
Nightly (2014-09-30) has the same behavior. 

Regression window:
Last good: 2014-09-30
First bad: 2014-10-01
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c24470b6b3a&tochange=14665b1de5ee

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch

Spelling error. I also renamed the java restriction so that we were at least consistent between the two. Happy to go the other way if you want.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8502066 [details] [diff] [review]
Patch

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

::: mobile/android/chrome/content/browser.js
@@ +464,5 @@
>  
>      if (this._startupStatus)
>        this.onAppUpdated();
>  
> +    if (!ParentalControls.isAllowed(ParentalControls.INSTALL_EXTENSION)) {

Should http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/RestrictedProfiles.java?rev=be045ef85f3f#147 fail if it is passed 0 (or whatever the undefined ENUM value is)? Line 152 looks like it silently says that it is allowed.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1080304 for this.

Comment 24

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/9901d2c678c7

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9901d2c678c7

Comment 26

[Flaviu Cos]

Verified as fixed in build 36.0a1 (2014-11-19);
Device: Nexus 4 (Android 4.4.4).