1295542 - (CVE-2017-5427) Firefox tries to read a non existent chrome.manifest from install directory.

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Mike Kaply [:mkaply]]

When you open Firefox, you'll see this message on the console:

Could not read chrome manifest 'file:///Applications/Firefox.app/Contents/Resources/chrome.manifest'.

Because of this, you can drop a chrome.manifest file and a Javascript component into the Firefox install directory and have it loaded at startup without modifying any existing files.

I've verified this on Windows.

Comment 1

[Daniel Veditz [:dveditz]]

If you could drop a chrome.manifest there you could modify/replace omni.ja, correct? Although that would get overwritten at the next update and the extra chrome.manifest might not.

Comment 2

[Mike Kaply [:mkaply]]

> If you could drop a chrome.manifest there you could modify/replace omni.ja, correct? Although that would get overwritten at the next update and the extra chrome.manifest might not.

Per Gijs, there are different permissions for adding files versus modifying/replacing files. So this would be an easier thing per say than replacing omni.ja.

I honestly don't know enough about it.

Comment 3

[:Gijs (he/him)]

(In reply to Mike Kaply [:mkaply] from comment #2)
> > If you could drop a chrome.manifest there you could modify/replace omni.ja, correct? Although that would get overwritten at the next update and the extra chrome.manifest might not.
> 
> Per Gijs, there are different permissions for adding files versus
> modifying/replacing files.

Well, there could be. I don't know what our installers and the osx dmg (and linux distros) do here. Either way, the update argument from comment #1 still also applies.

Comment 4

[Benjamin Smedberg]

spohl, can you look into this since I don't have a mac? I remember when we did mac signing work we moved a bunch of stuff from Contents/MacOS to Contents/Resources, but if we really don't need to read this it should be easy to just stop.

Comment 5

[Mike Kaply [:mkaply]]

This works on Windows as well (that's where I tested it). Basically we try to load a chrome.manifest from the EXE location.

Comment 6

[Stephen A Pohl [:spohl]]

Sure, I'll take a look.

We started printing this message on 3/18/2013, but nothing jumps out at me. But this does confirm that it's unrelated to Mac v2 signing:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b03bb3ce8cee&tochange=e23e43a2c14e

I believe that based on [1], we remove a dropped chrome.manifest during updates. So we might have the same severity as replacing omni.ja (comment 1) here.

[1] https://hg.mozilla.org/mozilla-central/annotate/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/browser/installer/removed-files.in#l78

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Remove log message

Bug 1063052 and bug 1059567 made it so that we remove chrome.manifest files in appDir and greDir during updates. So the risk here is equivalent to someone replacing omni.ja. Also, note that mucking with any of the contents in the Firefox.app dir on OSX will break the signature on the .app bundle.

I don't think we want to remove the ability to read chrome.manifest files from these locations directly though, since (at least on OSX) we don't build the omni.ja during local builds. It is only the |make package| step that actually creates these.

Since the security risk here is not greater than having the omni.ja replaced, the log message in the console is expected on all but local builds, and because the log message is therefore confusing, I suggest that we remove this log message (this patch). Another option is to rewrite the |else if| condition for the log message to:

> else if (aType != NS_BOOTSTRAPPED_LOCATION && aType != NS_APP_LOCATION)

or the equivalent:

> else if (aType == NS_EXTENSION_LOCATION || aType == NS_SKIN_LOCATION)

I couldn't figure out how helpful the log message is when aType is set to either NS_EXTENSION_LOCATION or NS_SKIN_LOCATION, so I'm not sure if we gain anything in keeping the log message for these.

Comment 8

[Benjamin Smedberg]

Comment on attachment 8787505 [details] [diff] [review]
Remove log message

That doesn't solve the bug being reported here with a security flag. If we were going to take this patch, we might as well WONTFIX the bug.

If the reason we're reading chrome.manifest is for non-omnijarred builds only, can we make it so that we only read one or the other? That way file-injection attacks will fail properly.

Comment 9

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Benjamin Smedberg [:bsmedberg] from comment #8)
> That doesn't solve the bug being reported here with a security flag.

Right, since we clean out injected greDir/chrome.manifest files the same way we do with greDir/omni.ja files during updates, I don't think there is much of a security bug here. 

> If we were going to take this patch, we might as well WONTFIX the bug.

This would leave us with the confusing console message, which I was trying to fix.

> If the reason we're reading chrome.manifest is for non-omnijarred builds
> only, can we make it so that we only read one or the other? That way
> file-injection attacks will fail properly.

This new patch reverses the order in which we read files in greDir, and makes one conditional on the other. We now try to read greDir's omni.ja first. If greDir's omni.ja (typically a release build) could not be found, we try to read greDir's chrome.manifest (typically a local build). And we keep the console message in place in case neither omni.ja nor chrome.manifest could be found (which is a legitimate problem to display in the console).

Note that I did not change the read order for appDir's chrome.manifest and appDir's omni.ja, since both are currently used by release builds and we don't print a confusing console message.

For reference, on OSX:
greDir: Firefox.app/Contents/Resources/
appDir: Firefox.app/Contents/Resources/browser/

Comment 10

[Benjamin Smedberg]

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

Mike, I think this looks righteous. Do you forsee any problems with this?

Comment 11

[Mike Hommey [:glandium]]

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

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

This will undoubtedly break Thunderbird and Seamonkey (and third-party gecko-based apps) because their top-level non-omni.ja chrome.manifest contains "manifest components/components.manifest", and components/components.manifest contains the registration of their binary components. (actually checking, it's only theoretical for Thunderbird, it surprisingly doesn't have binary components at the moment ; it's still true for Seamonkey)

I'll reiterate here what I suggested a long time ago: we could entirely remove the code that "manually" reads chrome.manifest from omni.ja, and add "manifest jar:omni.ja!/chrome.manifest" in the top-level chrome.manifest. That would make chrome.manifest always exist.

I probably even have half a patch somewhere for that. I can dig it out or reimplement it from scratch if we decide to go with this proposal.

Comment 12

[Stephen A Pohl [:spohl]]

So, to summarize: there is not really a security issue here (i.e. no greater than other security issues that we're willing to live with like injecting omni.ja files until an update cleans them out). We do have a confusing log message in the console.

We can either:
1. Remove the confusing log message (comment 7).
2. Implement the solution suggested by Mike in comment 11.
3. Do nothing.

Benjamin, do you have a preferred path forward here?

Comment 13

[Mike Kaply [:mkaply]]

The reason this is marked as a security issue is because apparently adding files on some OSes can be a different (lesser) permission than replacing files.

So being able to drop files in is "easier" than replacing files.

So by dropping two files into the directory, you can turn off extension signing.

Whereas replacing omni.ja requires more permissions.

The reason this came up is because you can use autoconfig to turn off signing by dropping in two files and my response was "you can also drop a chrome manifest and a JS file in to turn off signing".

Comment 14

[Benjamin Smedberg]

I feel like we should fix bugs where drop-in files can be used to own Firefox. I'm happy for glandium's suggestion or other reasonable technical ways to accomplish this. I don't think we should suppress the log message because it points to something real.

Comment 15

[Stephen A Pohl [:spohl]]

Mike, do you still have that partial patch that you were referring to?

Comment 16

[Mike Hommey [:glandium]]

I don't, but I have a simpler and less risky idea. I'll attach a patch shortly.

Comment 17

[Mike Hommey [:glandium]]

Attachment: Ensure at least an empty chrome.manifest next to each omnijar file

My proposal in comment 11 is something that might be desirable long term[1], but it requires modifications to C++ code as well as changes to the packager code.

This alternative approach only requires smaller changes to the packager code, by just creating an empty chrome.manifest (and filling it if it needs to be filled), so that there's always a chrome.manifest file. Simpler, and less risky.

Note: This patch doesn't cover the updater, which may or may not remove the file (I actually don't know ; who does or can check?).

1. Actually, if we do end up folding browsercomps into libxul, that's actually not very desirable, except maybe for comm-central and other apps.

Comment 18

[Stephen A Pohl [:spohl]]

(In reply to Mike Hommey [:glandium] from comment #17)
> Created attachment 8790608 [details] [diff] [review]
> This alternative approach only requires smaller changes to the packager
> code, by just creating an empty chrome.manifest (and filling it if it needs
> to be filled), so that there's always a chrome.manifest file. Simpler, and
> less risky.

We will have to make sure to not regress bug 1063052.

> Note: This patch doesn't cover the updater, which may or may not remove the
> file (I actually don't know ; who does or can check?).

Bug 1063052 and bug 1059567 made it so we do remove chrome.manifest from greDir. Also see comment 7, but note that I was mistaken about removing chrome.manifest from appDir. The meaning of greDir has changed, so we're removing chrome.manifest from the old and the new greDir location during update. But that's independent of appDir.

Comment 19

[Mike Hommey [:glandium]]

(In reply to Stephen A Pohl [:spohl] from comment #18)
> (In reply to Mike Hommey [:glandium] from comment #17)
> > Created attachment 8790608 [details] [diff] [review]
> > This alternative approach only requires smaller changes to the packager
> > code, by just creating an empty chrome.manifest (and filling it if it needs
> > to be filled), so that there's always a chrome.manifest file. Simpler, and
> > less risky.
> 
> We will have to make sure to not regress bug 1063052.

Bug 1063052 was caused by the lack of a chrome.manifest file in the first place. People who would unpack omni.ja would then get a chrome.manifest that would be used forever. Here, we would ship an empty chrome.manifest. If people unpack omni.ja, they would overwrite it, and an upgrade would re-overwrite that with a new empty file.

Comment 20

[Mike Hommey [:glandium]]

Attachment: Don't remove chrome.manifest on upgrades

The other patch in this bug makes it to that a chrome.manifest file always exists, even empty. Bug 1063052 was caused by a chrome.manifest file not existing in the first place but being used by Firefox when it existed, so the workaround was to delete it on upgrade. But now that upgrades are always going to provide a chrome.manifest, there is no need to remove it on upgrade.

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 8791027 [details] [diff] [review]
Don't remove chrome.manifest on upgrades

The following should also be removed
https://dxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/shared.nsh#806

Comment 22

[Mike Hommey [:glandium]]

Attachment: Don't remove chrome.manifest on upgrades

Comment 23

[Robert Strong (they/them - no direct email)]

Comment on attachment 8791042 [details] [diff] [review]
Don't remove chrome.manifest on upgrades

Thanks and I like this fix!

Comment 24

[Robert Strong (they/them - no direct email)]

So, for partial updates the empty chrome.manifest should always be added vs. patched. I'll submit a patch for this in a couple.

Comment 25

[Robert Strong (they/them - no direct email)]

Attachment: patch for update packaging

Ben, I think this is all that needs to be done for update packaging though I have no idea if anything needs to be done to the funsize code.

Comment 26

[[e-mail] (:bhearsum)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #25)
> Created attachment 8791783 [details] [diff] [review]
> patch for update packaging
> 
> Ben, I think this is all that needs to be done for update packaging though I
> have no idea if anything needs to be done to the funsize code.

I _think_ Funsize grabs these files from whatever branch it is generating partials for. Rail, can you confirm? If Funsize has its own special handling of forced files, it will need to be updated.

Comment 27

[Rail Aliiev [:rail]]

(In reply to Ben Hearsum (:bhearsum) from comment #26)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #25)
> > Created attachment 8791783 [details] [diff] [review]
> > patch for update packaging
> > 
> > Ben, I think this is all that needs to be done for update packaging though I
> > have no idea if anything needs to be done to the funsize code.
> 
> I _think_ Funsize grabs these files from whatever branch it is generating
> partials for. Rail, can you confirm? If Funsize has its own special handling
> of forced files, it will need to be updated.

Correct, we fetch those files per branch: https://dxr.mozilla.org/mozilla-central/source/release/docker/funsize-update-generator/scripts/funsize.py#132-137

Comment 28

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf231e054b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc66eae9b55b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e84554cb1201

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccf231e054b5
https://hg.mozilla.org/mozilla-central/rev/bc66eae9b55b
https://hg.mozilla.org/mozilla-central/rev/e84554cb1201