1251729 - Distribution.js uses eval, allowing for chrome access from distribution.ini

Status: RESOLVED FIXED

(Firefox :: Distributions, defect)

Description

[Mike Kaply [:mkaply]]

Distribution.js uses eval on the strings in distribution.ini with no sanitizing.

As a result, something like foo="Services.prompt.alert(null,"",Components.classes)" works.

Need to change distribution.js to not use eval.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Use Preferences.jsm

This switches us to using Preferences.jsm and uses JSON.parse to convert things from the distribution.ini.

I did a quick search through the partners and no one was relying on this behavior (and they shouldn't have been).

You'll wonder about the setComplexPref to set change for the distribution.about. Preferences.jsm defaults to setComplexValue/nsISupportsString for Strings, so it just works.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Bing distro makes use of the eval here:

https://github.com/mozilla-partners/bing/blob/master/desktop/bing/distribution/distribution.ini#L17

I'm not sure if that is removed in the rewrite you're working on, as well not sure if other distro's do anything like this.  I'd like to be certain that we don't need some solution for what the bing distro is doing.

Comment 3

[Shane Caraveo (:mixedpuppy)]

We probably also should get someone to take a look at the security aspect of this, and whether it needs to uplift or not.

Comment 4

[Mike Kaply [:mkaply]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Bing distro makes use of the eval here:
> 
> https://github.com/mozilla-partners/bing/blob/master/desktop/bing/
> distribution/distribution.ini#L17
> 
> I'm not sure if that is removed in the rewrite you're working on, as well
> not sure if other distro's do anything like this.  I'd like to be certain
> that we don't need some solution for what the bing distro is doing.

I totally missed that.

And man that is ugly. 

Cu.import("resource://gre/modules/Services.jsm");
if (!Services.prefs.prefHasUserValue("[e-mail]")) {
 Services.prefs.clearUserPref("extensions.lastAppVersion");
  Cc["@mozilla.org/toolkit/app-startup;1"].getService(Ci.nsIAppStartup).quit(Ci.nsIAppStartup.eAttemptQuit | Ci.nsIAppStartup.eRestart);
}
throw {};

So what that is effectively doing is if the user already has the bing add-on installed, clear the extensions.lastAppVersion and then restart which causes it to get reinstalled from the distro.

Since we're going to remove the Bing extension from the distro, I don't think this is needed anymore (I hope).

Alternatively, I could add code to distribution.js that handled this case, like:

forceReinstall=[e-mail]

but I don't think that's a good idea...

Comment 5

[Mike Kaply [:mkaply]]

I have all the partners installed and I just did a quick grep and only found bing. I grepped for Cu and Services.

I also checked China and it doesn't use it.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8724239 [details] [diff] [review]
Use Preferences.jsm

If this needs uplift, lets try for a smaller patch.

Comment 7

[Shane Caraveo (:mixedpuppy)]

quick chat with dveditz, since this would require system access to drop a distribution.ini file in, omni.jar could simply be messed with directly.  uplift not necessary.

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Minimal patch

Saving Preferences.jsm for another day.

This is a minimal patch that just removes eval

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8724256 [details] [diff] [review]
Minimal patch

minimal is good too :)

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> We probably also should get someone to take a look at the security aspect of
> this, and whether it needs to uplift or not.

Although we're trying to harden things (this bug, for example), if someone can write into the install directory it's still game over. I wouldn't mind this being uplifted to Aurora, but I don't think it needs to be on security grounds. It's way too late to try uplifting to Beta-45 for anything short of a critical stop-ship kind of bug (security or regression).

Comment 11

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/fx-team/rev/2f7cae37286ec8aa65e5d90802e2dfe6b93d8742
Bug 1251729 - Don't use eval for distribution.js; r=mixedpuppy

Comment 12

[Mike Kaply [:mkaply]]

I've decided to do the two patches separately to make it more clear.

As a side note, there is a pref used for testing, distribution.testing.loadFromProfile, that allows distribution.ini to be loaded from the profile.

I did verify that this preference can't be used to allow this code to run in the profile; it's not read early enough. I tested user.js and prefs.js.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f7cae37286e