Closed Bug 1251729 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Distributions, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

Details

(Keywords: sec-want, Whiteboard: [adv-main47-])

Attachments

(2 files)

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.
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.
Attachment #8724239 - Flags: review?(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.
Flags: needinfo?(mozilla)
We probably also should get someone to take a look at the security aspect of this, and whether it needs to uplift or not.
Keywords: sec-audit
(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("extensions.installedDistroAddon.firefoxbingsearch.full@microsoft.com")) {
 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=extensions.installedDistroAddon.firefoxbingsearch.full@microsoft.com

but I don't think that's a good idea...
Flags: needinfo?(mozilla)
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 on attachment 8724239 [details] [diff] [review]
Use Preferences.jsm

If this needs uplift, lets try for a smaller patch.
Attachment #8724239 - Flags: review?(mixedpuppy) → review+
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.
Keywords: sec-audit
Attached patch Minimal patchSplinter Review
Saving Preferences.jsm for another day.

This is a minimal patch that just removes eval
Attachment #8724239 - Attachment is obsolete: true
Attachment #8724256 - Flags: review?(mixedpuppy)
Comment on attachment 8724256 [details] [diff] [review]
Minimal patch

minimal is good too :)
Attachment #8724256 - Flags: review?(mixedpuppy) → review+
Attachment #8724256 - Flags: review+
Attachment #8724239 - Attachment is obsolete: false
(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).
Group: firefox-core-security
Keywords: sec-want
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.
https://hg.mozilla.org/mozilla-central/rev/2f7cae37286e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Whiteboard: [adv-main47-]
You need to log in before you can comment on or make changes to this bug.