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)
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.
Reporter | ||
Comment 1•7 years ago
|
||
Maybe isFileProcess in [1] should also be true for resource:// paths ?
[1] http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2415
Reporter | ||
Comment 2•7 years ago
|
||
Permalink for previous URL: http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/dom/ipc/ContentParent.cpp#2415
Assignee | ||
Comment 3•7 years 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•7 years 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•7 years ago
|
Assignee: nobody → gpascutto
Updated•7 years ago
|
Comment 7•7 years ago
|
||
(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•7 years 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•7 years 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!
Updated•7 years ago
|
Crash Signature: [@ mozalloc_abort | NS_DebugBreak | ErrorLoadingSheet]
Comment 10•7 years ago
|
||
mozreview-review |
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•7 years 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)
Comment 13•7 years 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.)
Known deficiency. I couldn't find a bug for it, so I filed bug 1386404.
Assignee | ||
Comment 14•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf51237c6cf93221675e5f6303b147f377dc447
Bug 1385891 - Whitelist extensions dir in the profile. r=jld
Comment 15•7 years ago
|
||
Track 56+ as this might affect the users who have installed extensions.
status-firefox56:
--- → affected
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(gpascutto)
Comment 16•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 17•7 years ago
|
||
Well this is embarrassing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: sb+
Comment 20•7 years ago
|
||
mozreview-review |
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•7 years 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•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 23•7 years ago
|
||
Please request Beta approval on this when you get a chance.
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 27•7 years ago
|
||
Removing Firefox 56 as affected given that bug 1388046 landed on beta.
tracking-firefox56:
+ → ---
Comment 28•7 years 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•7 years 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•7 years 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.
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
OS: Unspecified → Linux
Hardware: Unspecified → All
You need to log in
before you can comment on or make changes to this bug.
Description
•