Firefox doesn't load extension's files after upgrade

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: julienw, Assigned: gcp)

Tracking

({regression})

Trunk
mozilla57
All
Linux
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 fixed)

Details

(Whiteboard: sb+, crash signature)

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
Maybe isFileProcess in [1] should also be true for resource:// paths ?

[1] http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2415
(Assignee)

Comment 3

a year ago
(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.
(Reporter)

Comment 4

a year ago
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
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → gpascutto
(Assignee)

Updated

a year ago
Duplicate of this bug: 1384446
See Also: → bug 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.)
(Assignee)

Comment 8

a year ago
(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.
(Reporter)

Comment 9

a year ago
(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+

Comment 11

a year ago
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.
status-firefox56: --- → affected
tracking-firefox56: ? → +
(Assignee)

Updated

a year ago
Flags: needinfo?(gpascutto)

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cf51237c6cf
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 17

a year ago
Well this is embarrassing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1384240

Updated

a year ago
Priority: -- → P1
Whiteboard: sb+

Updated

a year ago
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.

Comment 21

a year ago
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

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55b494574257
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
Please request Beta approval on this when you get a chance.
status-firefox55: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(gpascutto)
(Assignee)

Comment 24

a year ago
We will uplift bug 1388046 instead.
Flags: needinfo?(gpascutto)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1385712
(Assignee)

Updated

a year ago
Duplicate of this bug: 1385329
(Assignee)

Comment 27

a year ago
Removing Firefox 56 as affected given that bug 1388046 landed on beta.
status-firefox56: affected → unaffected
tracking-firefox56: + → ---

Comment 28

a year ago
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
(Assignee)

Comment 29

a year ago
(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.

Comment 30

a year ago
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: → bug 1404298
(Assignee)

Updated

a year ago
OS: Unspecified → Linux
Hardware: Unspecified → All
You need to log in before you can comment on or make changes to this bug.