1226823 - adblock plus does not work in latest inbound builds

Status: VERIFIED FIXED

(Firefox :: Extension Compatibility, defect)

Description

[[e-mail]]

error message:

Security Error: Content at moz-nullprincipal:{73ae3ded-929a-4671-8096-e5a3d390999f} may not load or link to chrome://adblockplus/locale/overlay.dtd.

Comment 1

[Kostas]

Extra info: 


(1) This occurs if you install either Adblock Plus or Element Hiding Helper(for ABP) in a clean inbound profile (since 2015-11-20).



(2) Screenshots (using win10x64):
at launch, after installing ABP
https://i.imgur.com/MNHRM8f.jpg
at launch, after installing EHH
https://i.imgur.com/XBFOrZq.jpg



(3) Entries in Browser Console:

at launch, after installing ABP

1448129571839	addons.xpi	WARN	Failed to remove temporary file C:\Users\Kostas\AppData\Local\Temp\tmp-a1g.xpi for addon https://addons.mozilla.org/firefox/downloads/file/354564/adblock_plus-2.6.11-sm+tb+fx+an.xpi?src=api: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: AI_removeTemporaryFile :: line 5133"  data: no] Stack trace: AI_removeTemporaryFile()@resource://gre/modules/addons/XPIProvider.jsm:5133 < AI_startInstall/<()@resource://gre/modules/addons/XPIProvider.jsm:5895 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813
Security Error: Content at moz-nullprincipal:{09a63301-5719-4ed5-848a-f68ee5c8c1d0} may not load or link to chrome://adblockplus/locale/overlay.dtd.


at launch, after installing EHH

1448130214406	addons.xpi	WARN	Failed to remove temporary file C:\Users\Kostas\AppData\Local\Temp\tmp-3qs.xpi for addon https://addons.mozilla.org/firefox/downloads/file/345472/element_hiding_helper_for_adblock_plus-1.3.4-sm+tb+fx.xpi?src=api: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: AI_removeTemporaryFile :: line 5133"  data: no] Stack trace: AI_removeTemporaryFile()@resource://gre/modules/addons/XPIProvider.jsm:5133 < AI_startInstall/<()@resource://gre/modules/addons/XPIProvider.jsm:5895 < Handler.prototype.process()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934 < this.PromiseWalker.walkerLoop()@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813



(4) Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0eab88b18c27ac2fa767725f3b6268c0aeb320e&tochange=9c2bad72beab374bc53e1e6c7fd6ede5b530b51e

Comment 2

[Gary [:streetwolf52]]

Looks like it is a result of this.  https://bugzilla.mozilla.org/show_bug.cgi?id=1226869

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to zhoubcfan from comment #0)
> Security Error: Content at
> moz-nullprincipal:{73ae3ded-929a-4671-8096-e5a3d390999f} may not load or
> link to chrome://adblockplus/locale/overlay.dtd.

Bug 1182546 is most definitely causing that breakage. The *.dtd needs to be accessible to content. Adding
> contentaccessible=yes
to the exposed chrome package should fix the problem, fore more details see:
> https://developer.mozilla.org/en/docs/Chrome_Registration#contentaccessible

Comment 4

[Wladimir Palant]

For reference, the document loading that DTD file is chrome://adblockplus/content/ui/overlay.xul. However, it is being loaded via XMLHttpRequest ever since Adblock Plus was made restartless - that no longer works it seems. Using contentaccessible is not an option for us. So, unless somebody wants to Cc me on bug 1182546 I guess I'll have to reverse engineer the changes implemented there.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Wladimir Palant from comment #4)
> For reference, the document loading that DTD file is
> chrome://adblockplus/content/ui/overlay.xul. However, it is being loaded via
> XMLHttpRequest ever since Adblock Plus was made restartless - that no longer
> works it seems. Using contentaccessible is not an option for us. So, unless
> somebody wants to Cc me on bug 1182546 I guess I'll have to reverse engineer
> the changes implemented there.

Just added you to bug 1182546.

Comment 6

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to Wladimir Palant from comment #4)
> > For reference, the document loading that DTD file is
> > chrome://adblockplus/content/ui/overlay.xul. However, it is being loaded via

What is "it" here - the dtd or the xul file? Can you expand on how this happens? If all this is chrome code and not loaded in a content tab, I don't think you should be having this problem.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 45 since this looks like a regression from bug 1182546.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> However, it is being loaded via XMLHttpRequest

So this is fairly similar to the situation in bug 1226869 comment 7.  XHR from chrome also nukes the principal of the document being fetched so it won't load subresources, but we used to not consider DTDs subresources, effectively.

Comment 13

[Wladimir Palant]

Yes, that's how I understood it as well. My impression is that this behavior isn't going to change any more, and that there is no real workaround (other than contentaccessible which isn't really an option for us). So the plan right now is to convert the locale file into .properties and do localization dynamically - both for Adblock Plus and Element Hiding Helper.

(In reply to :Gijs Kruitbosch from comment #6)
> What is "it" here - the dtd or the xul file? Can you expand on how this
> happens? If all this is chrome code and not loaded in a content tab, I don't
> think you should be having this problem.

Yes, this is privileged code loading a XUL file via XMLHttpRequest. However, as Boris explained above a document loaded by XMLHttpRequest is never privileged - so now it isn't allowed to load DTDs.

Comment 14

[Marco Castelluccio [:marco]]

The first error I'm seeing in the browser console is "TypeError: Ci is null utils.js:178:9":
  /**
   * If a protocol using nested URIs like jar: is used - retrieves innermost
   * nested URI.
   */
  unwrapURL: function(/**nsIURI or String*/ url) /**nsIURI*/
  {
    if (!(url instanceof Ci.nsIURI))
      url = Utils.makeURI(url);

Comment 15

[Nils Maier [:nmaier]]

Attachment: Quick and dirty workaround patch against ABP tip

I’ve been running into the same issue with my own add-ons right now. Same situation: loading DTDs in an XML via XHR.

There is actually a work-around which requires less headache than completely removing DTDs (or maybe even moving DTDs into a new package):
Just set the channel.owner to the system principal before firing off request.

    let sec = Cc['@mozilla.org/scriptsecuritymanager;1'].getService(Ci.nsIScriptSecurityManager);
    request.channel.owner = sec.getSystemPrincipal();

This is possible since chrome-privileged code has access to the |.channel|. Of course, this elevates the whole request, so it should be done carefully. But loading chrome:// channels in the first place, this shouldn't be an issue.

Works for my own addons, as well, as ABP.

Comment 16

[Wladimir Palant]

Comment on attachment 8691884 [details] [diff] [review]
Quick and dirty workaround patch against ABP tip

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

::: lib/ui.js
@@ +285,5 @@
>      let request = new XMLHttpRequest();
>      request.mozBackgroundRequest = true;
>      request.open("GET", "chrome://adblockplus/content/ui/overlay.xul");
> +    let sec = Cc['@mozilla.org/scriptsecuritymanager;1'].getService(Ci.nsIScriptSecurityManager);
> +    request.channel.owner = sec.getSystemPrincipal();

Thanks a lot, this works! This should allow us to have a quick follow-up release. The fix is under review in https://issues.adblockplus.org/ticket/3347.

Comment 17

[:Gijs (he/him)]

(In reply to Nils Maier [:nmaier] from comment #15)
> But loading chrome:// channels in the first place, this shouldn't be an
> issue.

It won't be for the original channel, but it will be for any dependent loads, right? I mean, that's the point of this fix - the file can now load things it didn't use to be able to load. That's fine if the document doesn't load anything (and we don't load things into it afterwards by appending tags that source external resources and whatnot) that we expect to be privilege-restricted, but it's not going to be great otherwise...

With what principal does script in that document get executed before/after this change? (I don't know offhand whether the "owner" principal affects the principal we then use for JS running in its DOM and so on, but it would be good to check that, too.)

(To be clear, it's not that I know this is a problem, I'm just trying to make sure we're all being cautious about elevating the principal here - it would be ironic if a security issue fixed in bug 1182546 would now trigger new (potentially worse) security issues in add-ons that it broke...)

Comment 18

[Boris Zbarsky [:bzbarsky]]

> With what principal does script in that document get executed before/after this change? 

It's an XHR response document; script is not executed in it at all.

The main issue with setting the principal as described is that <img> elements in the document will start loading images.  I _think_ <script> and <link rel="stylesheet"> elements won't perform any loads, but I'm not 100% sure.

For what it's worth, as I told Christoph over email, I think we should revert the security policy to what it used to be ASAP, then figure out what the security policy should be, then if that requires changes to addons communicate what those changes need to be and _then_ change the security policy.

Comment 19

[Nils Maier [:nmaier]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> > With what principal does script in that document get executed before/after this change? 
> 
> It's an XHR response document; script is not executed in it at all.

Indeed.

> The main issue with setting the principal as described is that <img>
> elements in the document will start loading images.  I _think_ <script> and
> <link rel="stylesheet"> elements won't perform any loads, but I'm not 100%
> sure.

Are you sure this is actually true? It will be parsed as into an XMLDocument, not HTMLDocument or XULDocument.

> For what it's worth, as I told Christoph over email, I think we should
> revert the security policy to what it used to be ASAP, then figure out what
> the security policy should be, then if that requires changes to addons
> communicate what those changes need to be and _then_ change the security
> policy.

Very good idea ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Are you sure this is actually true? 

I'm pretty sure it's true if the <img> elements are in the XHTML namespace.  The type of the document doesn't matter; just the namespace and localName of the elements.

Comment 22

[Tony Mechelynck [:tonymec]]

Duplicate bug 1227955 was found on SeaMonkey. Shouldn't it be in a different Product than "Firefox" then? Or else, if this bug is fixed in some Firefox-specific source, then the patch should be ported to comm-central, probably by some SeaMonkey developer?

@Wladimir Palant: Bug 1227955 was found after upgrading Adblock Plus to 2.6.12 and part of it still happens after disabling Element hiding Helper for Adblock Plus 1.3.4.

Comment 23

[Wladimir Palant]

Adblock Plus 2.6.13 and Element Hiding Helper 1.3.5 have been released, waiting for AMO review right now. The inline options are still broken (see bug 1227981) but at least they don't cause any catastrophic failure any more.

Feel free to resolve this bug.

(In reply to Boris Zbarsky [:bz] from comment #18)
> The main issue with setting the principal as described is that <img>
> elements in the document will start loading images.  I _think_ <script> and
> <link rel="stylesheet"> elements won't perform any loads, but I'm not 100%
> sure.

Luckily, what we are loading there is a simple XUL file, without anything unexpected like HTML images or scripts.

(In reply to Tony Mechelynck [:tonymec] from comment #22)
> @Wladimir Palant: Bug 1227955 was found after upgrading Adblock Plus to
> 2.6.12

We merely happened to push out a release yesterday, at the same time when bug 1182546 landed. The issue isn't related to the update, it simply affects both Adblock Plus and Element Hiding Helper.

Comment 27

[Tony Mechelynck [:tonymec]]

(In reply to Wladimir Palant from comment #23)
> Adblock Plus 2.6.13 and Element Hiding Helper 1.3.5 have been released,
> waiting for AMO review right now. The inline options are still broken (see
> bug 1227981) but at least they don't cause any catastrophic failure any more.
> 
> Feel free to resolve this bug.

After updating Adblock Plus to 2.6.13 and Element Hiding Helper to 1.3.5, the bug has disappeared from the following SeaMonkey build, which was affected before (bug 1227955):

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 SeaMonkey/2.42a1" ID:20151125003001 c-c:d45abb4b289717f94a91e82800326ea294b4811a m-c:099f695d31326c39595264c34988a0f4b7cbc698 en-US

I'm leaving this bug open, waiting for confirmation on Firefox.

Comment 28

[Marco Castelluccio [:marco]]

Fixed in Firefox Nightly.

Comment 29

[pawel]

yes Fixed

Comment 30

[Tony Mechelynck [:tonymec]]

Setting FIXED according to comment #23 to 29. The fix is in the code for Adblock Plus 2.6.13 and Adblock Element Hiding Helper 1.3.5.

Comment 31

[2ljpswpih]

Attachment: still.PNG

I still see this.

Comment 32

[Tony Mechelynck [:tonymec]]

(In reply to to_du from comment #31)
> Created attachment 8692127 [details]
> still.PNG
> 
> I still see this.

Which version of Adblock Plus is installed? Check it either in the addons detail pane ("More" for Adblock Plus in the Add-ons Manager) or on about:support (Help → Troubleshooting Information).

Comment 33

[Tony Mechelynck [:tonymec]]

oh, I forgot to set NEEDINFO when I asked a question in comment #32 (Which Adblock Plus version?)

Comment 34

[2ljpswpih]

2.6.13, but this has gone already. I don't see this anymore.

Comment 35

[Wladimir Palant]

(In reply to to_du from comment #31)
> I still see this.

The message won't go away on update, new browser windows should be fine however.

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

verified it worked for me

Comment 38

[2ljpswpih]

Everything now is ok, apart of the message "AdBlock Plus makes Nightly run slowly".

Comment 39

[Wladimir Palant]

That message is bug 1161798 for most part.