Closed Bug 1385891 Opened 7 years ago Closed 7 years ago

Firefox doesn't load extension's files after upgrade

Categories

(Core :: Security: Process Sandboxing, defect, P1)

All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: julienw, Assigned: gcp)

References

Details

(Keywords: regression, Whiteboard: sb+)

Crash Data

Attachments

(1 file)

We got this error with this extension: http://julienw.github.io/forms-extension/french_holiday_forms-1.3.3-fx.xpi

This extension displays some interface with this URL: resource://jid1-ie0shncwskpkjg-at-jetpack/data/results.html

This works fine when installing the extension on a clean profile. But the issue happens when the extension is already installed and Firefox is upgraded. So maybe something is missing in extensions handling.

mozregression helped me find that the issue comes from Bug 1308400. Something very weird is that I couldn't reproduce the issue with mozregression's profile cloning, I had to copy my profile manually and use the option "--profile-persistence reuse" to be able to reproduce the problem.

Here is the logging from the sandbox (with MOZ_SANDBOX_LOGGING=1):

Sandbox: SandboxBroker: denied op=open rflags=0 perms=0 path=/home/julien/.mozilla/firefox/qvl73x2e.copy-default/extensions/jid1-Ie0ShncwSkPKJg@jetpack.xpi for pid=22017
Sandbox: Failed errno -13 op 0 flags 00 path /home/julien/.mozilla/firefox/qvl73x2e.copy-default/extensions/jid1-Ie0ShncwSkPKJg@jetpack.xpi


I believe this should be tracked because our users have installed extensions that could experience this issue.
Maybe isFileProcess in [1] should also be true for resource:// paths ?

[1] http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2415
(In reply to Julien Wajsberg [:julienw] from comment #1)
> Maybe isFileProcess in [1] should also be true for resource:// paths ?

No, a file process is used when the user initiates a load on a file:// URL.

Extensions aren't allowed to read arbitrary files from inside the content process. This was intentional. There is an exception for /extensions to support some legacy add-ons (i.e. it will be removed in Firefox 57). This exception was missed when read sandboxing was activated on Linux.
Yeah I'm in the process of converting the extension to a web extension for 57 but this isn't completely ready yet.

In this case we let the addon SDK define the URL: https://github.com/julienw/forms-extension/blob/62f951b340d3813f371bf206cfa05d0fb61dc0b7/src/lib/main.js#L103
Assignee: nobody → gpascutto
See Also: → 1384240
(In reply to Julien Wajsberg [:julienw] from comment #0)
> This works fine when installing the extension on a clean profile. But the
> issue happens when the extension is already installed and Firefox is
> upgraded. So maybe something is missing in extensions handling.

If it helps for diagnosis purposes -- in similar bug 1384240, I found that the key factor was whether or not the profile lives in my home directory (load blocked!!) vs /tmp (load not blocked).

> Something very weird is that I couldn't reproduce the issue with
> mozregression's profile cloning, I had to copy my profile manually and use
> the option "--profile-persistence reuse" to be able to reproduce the problem.

(This is because mozregression puts its profile clones in /tmp -- and as noted above, /tmp seems to be whitelisted or something.)
(In reply to Daniel Holbert [:dholbert] from comment #7)
> (This is because mozregression puts its profile clones in /tmp -- and as
> noted above, /tmp seems to be whitelisted or something.)

Indeed:
https://dxr.mozilla.org/mozilla-central/rev/36f95aeb4c77f7cf3b3366583008cd6e4b6b1dba/security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp#51

This interaction rendering mozregression ineffective is...rather unfortunate.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> > This works fine when installing the extension on a clean profile. But the
> > issue happens when the extension is already installed and Firefox is
> > upgraded. So maybe something is missing in extensions handling.
> 
> If it helps for diagnosis purposes -- in similar bug 1384240, I found that
> the key factor was whether or not the profile lives in my home directory
> (load blocked!!) vs /tmp (load not blocked).

No it doesn't help for this difference.
The extension file that doesn't work is located in /home/julien/.mozilla/firefox/qvl73x2e.default/extensions/jid1-Ie0ShncwSkPKJg@jetpack.xpi.
The extension file that do work is located in /home/julien/.mozilla/firefox/mvfiole9.work/extensions/jid1-Ie0ShncwSkPKJg@jetpack.xpi.

Both have the same permissions.

But I don't know how extensions are loaded -- is it in-memory or are they uncompressed somewhere?

Could the fact that the working installation be the symptom of another bug? I mean, should the load be forbidden in both cases?

> > Something very weird is that I couldn't reproduce the issue with
> > mozregression's profile cloning, I had to copy my profile manually and use
> > the option "--profile-persistence reuse" to be able to reproduce the problem.
> 
> (This is because mozregression puts its profile clones in /tmp -- and as
> noted above, /tmp seems to be whitelisted or something.)

Oh yeah, this makes totally sense!
Crash Signature: [@ mozalloc_abort | NS_DebugBreak | ErrorLoadingSheet]
Comment on attachment 8892072 [details]
Bug 1385891 - Whitelist things in the extension dir, not just the dir itself.

https://reviewboard.mozilla.org/r/163090/#review168912
Attachment #8892072 - Flags: review?(jld) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4b7f8bf959b8 -d 7ea87b926691: rebasing 410882:4b7f8bf959b8 "Bug 1385891 - Whitelist extensions dir in the profile. r=jld" (tip)
merging security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp
warning: conflicts while merging security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(Looks like this needs a rebase.)
Flags: needinfo?(gpascutto)
(In reply to Daniel Holbert [:dholbert] from comment #7)
> (This is because mozregression puts its profile clones in /tmp -- and as
> noted above, /tmp seems to be whitelisted or something.)

Known deficiency.  I couldn't find a bug for it, so I filed bug 1386404.
Track 56+ as this might affect the users who have installed extensions.
Flags: needinfo?(gpascutto)
https://hg.mozilla.org/mozilla-central/rev/7cf51237c6cf
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Well this is embarrassing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: -- → P1
Whiteboard: sb+
Blocks: 1385712
Comment on attachment 8892072 [details]
Bug 1385891 - Whitelist things in the extension dir, not just the dir itself.

https://reviewboard.mozilla.org/r/163090/#review170654

This is already listed as r=me.  Maybe MozReview is confused by the backout.  Anyway, r=me for that one-line fixup.
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55b494574257
Whitelist things in the extension dir, not just the dir itself. r=jld
https://hg.mozilla.org/mozilla-central/rev/55b494574257
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Please request Beta approval on this when you get a chance.
Flags: needinfo?(gpascutto)
We will uplift bug 1388046 instead.
Flags: needinfo?(gpascutto)
Removing Firefox 56 as affected given that bug 1388046 landed on beta.
This doesn't seem to be fixed for me in 56 beta/dev edition. Tried bug 1388046's fix with

mozregression --repo mozilla-beta --launch 2638feb177dd

Testing with greasemonkey, coming from bug 1385712 which was marked as dupe of this one.

That fix seems to be equivalent to changing "security.sandbox.content.level", and as other people pointed out in the greasemonkey github issue[1], that doesn't seem to help.

[1]: https://github.com/greasemonkey/greasemonkey/issues/2533#issuecomment-321747772
(In reply to dx from comment #28)
> This doesn't seem to be fixed for me in 56 beta/dev edition. Tried bug
> 1388046's fix with
> 
> mozregression --repo mozilla-beta --launch 2638feb177dd

This revision (56b02) predates bug 1388046 landing on beta, see comment 6 in that bug. As you can see in comment 7, the correct revision would be a1ac56679ed3.

> That fix seems to be equivalent to changing
> "security.sandbox.content.level", and as other people pointed out in the
> greasemonkey github issue[1], that doesn't seem to help.
> 
> [1]:
> https://github.com/greasemonkey/greasemonkey/issues/2533#issuecomment-
> 321747772

That's because bug 1386558 was required for that setting to work. Which wasn't in 56b01 that was tested in that greasemonkey issue. According to https://bugzilla.mozilla.org/show_bug.cgi?id=1386558#c10 that fix made 56b02.

So: changing the setting in 56b02 should fix the issue. In 56b03 the setting is flipped by default.
Oh, I got the hashes wrong, I honestly thought I copied it from comment 7 from the other bug, but I must have gotten it mixed up.

I can confirm testing the correct revision and changing the setting in 56b02 works.

Sorry for the noise.
See Also: → 1404298
OS: Unspecified → Linux
Hardware: Unspecified → All
You need to log in before you can comment on or make changes to this bug.