Bug 1295542 (CVE-2017-5427)

Firefox tries to read a non existent chrome.manifest from install directory.

RESOLVED FIXED in Firefox 52

Status

()

RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: mkaply, Assigned: glandium)

Tracking

({sec-moderate})

49 Branch
mozilla52
sec-moderate
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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.

Updated

3 years ago
Component: General → Startup and Profile System
Product: Firefox → Toolkit
See Also: → bug 1292444
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.
Keywords: sec-moderate
(Reporter)

Comment 2

3 years ago
> 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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 3

3 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 4

3 years ago
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.
Flags: needinfo?(spohl.mozilla.bugs)
(Reporter)

Comment 5

3 years ago
This works on Windows as well (that's where I tested it). Basically we try to load a chrome.manifest from the EXE location.
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
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Flags: needinfo?(spohl.mozilla.bugs)
Posted patch Remove log message (obsolete) — Splinter Review
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.
Attachment #8787505 - Flags: review?(benjamin)

Comment 8

3 years ago
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.
Attachment #8787505 - Flags: review?(benjamin) → review-
Posted patch Patch (obsolete) — Splinter Review
(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/
Attachment #8787505 - Attachment is obsolete: true
Attachment #8788746 - Flags: review?(benjamin)
Comment on attachment 8788746 [details] [diff] [review]
Patch

Mike, I think this looks righteous. Do you forsee any problems with this?
Attachment #8788746 - Flags: review?(mh+mozilla)
Attachment #8788746 - Flags: review?(benjamin)
Attachment #8788746 - Flags: review+
(Assignee)

Comment 11

3 years ago
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.
Attachment #8788746 - Flags: review?(mh+mozilla) → review-
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?
Flags: needinfo?(benjamin)
(Reporter)

Comment 13

3 years ago
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".
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.
Flags: needinfo?(benjamin)
Mike, do you still have that partial patch that you were referring to?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 16

3 years ago
I don't, but I have a simpler and less risky idea. I'll attach a patch shortly.
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 17

3 years ago
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.
Attachment #8788746 - Attachment is obsolete: true
Attachment #8790608 - Flags: feedback?(benjamin)
(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.
Assignee: spohl.mozilla.bugs → mh+mozilla

Updated

3 years ago
Attachment #8790608 - Flags: feedback?(benjamin) → feedback+
(Assignee)

Comment 19

3 years ago
(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.
(Assignee)

Updated

3 years ago
Attachment #8790608 - Flags: review?(gps)
(Assignee)

Comment 20

3 years ago
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.
Attachment #8791027 - Flags: review?(robert.strong.bugs)
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
Attachment #8791027 - Flags: review?(robert.strong.bugs) → review-
(Assignee)

Comment 22

3 years ago
Attachment #8791027 - Attachment is obsolete: true
Attachment #8791042 - Flags: review?(robert.strong.bugs)
Comment on attachment 8791042 [details] [diff] [review]
Don't remove chrome.manifest on upgrades

Thanks and I like this fix!
Attachment #8791042 - Flags: review?(robert.strong.bugs) → review+
Attachment #8790608 - Flags: review?(gps) → review+
So, for partial updates the empty chrome.manifest should always be added vs. patched. I'll submit a patch for this in a couple.
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.
Attachment #8791783 - Flags: review?(bhearsum)
Attachment #8791783 - Flags: review?(bhearsum) → review+
(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.
Flags: needinfo?(rail)

Comment 27

3 years ago
(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
Flags: needinfo?(rail)
https://hg.mozilla.org/mozilla-central/rev/ccf231e054b5
https://hg.mozilla.org/mozilla-central/rev/bc66eae9b55b
https://hg.mozilla.org/mozilla-central/rev/e84554cb1201
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Group: firefox-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Alias: CVE-2017-5427
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.