Closed Bug 1213646 Opened 9 years ago Closed 9 years ago

Possible Regression? moz-binding NS_ERROR_DOM_BAD_URI in AdBlock Plus

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox42 --- unaffected
firefox43 + fixed
firefox44 + fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: evengard, Assigned: sicking)

References

Details

(Keywords: qawanted)

Attachments

(1 file)

Allow URI_IS_UI_RESOURCE and safe about: URIs when SEC_ALLOW_CHROME is set
1.49 KB, patch
bzbarsky
: review+
ritu
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0 Build ID: 20151009004029 Steps to reproduce: After some update, AdBlock Plus started to work incorrectly without apparent reason. I found the issue on the AdBlock Plus issue tracker (https://issues.adblockplus.org/ticket/3108), but it is stated that it is unsure if it is a ABP bug or a Firefox regression. On pages with AdBlock Firefox throws following error: Попытка нарушения системы безопасности: содержимое «http://www.twitch.tv/shadowchorus» не имеет права загружать данные из about:abp-elemhidehit?824719624226. (russian browser version, states that "Security system breach detected - content from http://www.twitch.tv/shadowchorus can't load data from about:abp-elemhidehit?824719624226") NS_ERROR_DOM_BAD_URI: Access to restricted URI denied. The addon works by placing a CSS option to needed elements, such as: -moz-binding: url("about:abp-elemhidehit?843236156634#dummy") !important; As about research from the ABP issue tracker, it MAY be a regression from bug 1204703, but I'm unsure. Problem still present in 43.0a2 (2015-10-09) Actual results: The blocking mecanism doesn't work correctly, ignoring some rules, an error in browser console. Expected results: No errors, everything should just work like before.
Blocks: 1204703, abp
Status: UNCONFIRMED → NEW
Ever confirmed: true
Note that about:abp-elemhidehit is a custom about: handler registered by Adblock Plus. It is being referenced by a user stylesheet which should have the necessary privileges to access it. However, I didn't have much success by adding Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT and Ci.nsIAboutModule.URI_CAN_LOAD_IN_CHILD flags to the handler either. So far I was unable to figure out whether this is a bug in the Adblock Plus code or in the new security checks implemented by Firefox.
Component: Untriaged → DOM
Product: Firefox → Core
Version: 43 Branch → Trunk
Are you sure this is a regression from bug 1204703? I don't see how that bug would have affected the behavior here, since it just adds an API channels can call to be security-checked in the new way and I assume ABP is not calling it... What this bug needs is a regression range.
Keywords: qawanted, regressionwindow-wanted
[Tracking Requested - why for this release]: Breaking adblock plus is a bad idea.
status-firefox43: --- → affected
tracking-firefox43: --- → ?
Yes, part of this is bug 1204703, but the security checks generally seem to have changed here. I worked around one part of this by implementing our own security check in asyncOpen2 (https://hg.adblockplus.org/adblockplus/rev/9d2fcc878593), now I am trying to get rid of the compatibility shim that replaces our channel in content processes and implements a different security check (that part comes from bug 1204703).
Ah, this is the RemoteAddonsChild.jsm AboutProtocolChannel bit? Christoph, is it possible we're now doing incorrect security checks on some XBL loads that end up doing the AsyncOpen2 thing? Note that XBL has some weirdness in its security checks precisely to allow privileged sheets to apply privileged XBL to non-privileged documents...
Flags: needinfo?(mozilla)
In particular, nsXBLService::FetchBindingDocument in that situation will have aOriginPrincipal set to the principal of the stylesheet, but seems to be using aBoundDocument (the untrusted thing) for the triggering principal. And nsContentSecurityManager::doContentSecurityCheck will call DoCheckLoadURIChecks which will check the _triggering_ principal too. That's wrong for XBL. Looks like this was basically broken by <http://hg.mozilla.org/mozilla-central/rev/cf37257f81d2>: the old code enforced a quite different security policy which got totally lost in that checkin, afaict. That even caused test failures, as described in 1195162 comment 37 and bug 1195162 comment 43, and instead of fixing the code we changed the tests. :( The reasoning in bug 1195162 comment 64 doesn't apply to non-chrome:// URI schemes as here, of course.
Blocks: 1195162
Jonas, see comment 6.
Flags: needinfo?(jonas)
(In reply to Boris Zbarsky [:bz] from comment #5) > Ah, this is the RemoteAddonsChild.jsm AboutProtocolChannel bit? > > Christoph, is it possible we're now doing incorrect security checks on some > XBL loads that end up doing the AsyncOpen2 thing? Note that XBL has some > weirdness in its security checks precisely to allow privileged sheets to > apply privileged XBL to non-privileged documents... I think it's more likely that Bug 1195162, where we start using AsyncOpen2() within dom/xbl/nsXBLService.cpp, causes that regression. Saying that, it's still possible that we have updated the security checks incorrectly, even though I doubt that. In similar scenarios we had to e.g. make chrome://mozapps accessible to content by adding 'contentaccessible=yes'. Jonas, you have a better understanding of XBL than I do, can you help move this forward?
Flags: needinfo?(mozilla)
Yeah, we used to enable loading XBL from packages which did *not* have "contentaccessible=yes". We are now more strict about require the XBL to live in a package which has "contentaccessible=yes" if you want to use it in a web content. This even if the stylesheet which links to the XBL has chrome privileges. The upshot of this is that XBL which lives in a package which does not have "contentaccessible=yes", it is guaranteed not to be used in web content.
Flags: needinfo?(jonas)
This XBL doesn't live in a chrome package at all, so that's a completely useless answer.
Flags: needinfo?(jonas)
Where does it live? The critical thing is that CheckLoadURIWithPrincipal checks need to pass, independent of who links to the XBL. If it's loaded from an about: module, then that module needs to set URI_SAFE_FOR_UNTRUSTED_CONTENT.
Flags: needinfo?(jonas)
> Where does it live? about: URI. > If it's loaded from an about: module, then that module needs to set > URI_SAFE_FOR_UNTRUSTED_CONTENT. Hmm. That seems to require that we make things globally loadable just to be able to load them in XBL. Maybe that's ok....
It does make the system much more easy to reason about. We'll know that if CheckLoadURI returns false, then it won't be exposed to content no matter what. At least that was my reasoning...
(In reply to Jonas Sicking (:sicking) from comment #11) > Where does it live? The critical thing is that CheckLoadURIWithPrincipal > checks need to pass, independent of who links to the XBL. If it's loaded > from an about: module, then that module needs to set > URI_SAFE_FOR_UNTRUSTED_CONTENT. See comment #1 - I've tried that already and it doesn't seem to change anything.
Oooh... I bet I know what's going on. We're enforcing same-origin for XBL content. But we're also setting the ALLOW_CHROME flag which means that chrome:// URLs are allowed as long as they pass CheckLoadURI. However this is an about: URL and so it's probably getting blocked by the same-origin policy. Bz, do you have any suggestions for a sane policy here? I'd really like to have a general policy which is sane and can be applied equally to all resource types. Should we say that ALLOW_CHROME allows both chrome-urls and about:-urls (and maybe resource:-urls) as long as they pass CheckLoadURI? Or should we somehow apply special rules when the triggering-principal is a systemprincipal (similar to what we did before) rather than look at the ALLOW_CHROME flag. The consequences of that are somewhat harder to reason about though since they depend heavily on how the response is used by the webpage.
> We're enforcing same-origin for XBL content. Hmm. So we used to only do that when the loading principal was not system, right? Maybe we should go back to that?
Right, that's what I was referring to in the last sentence of comment 15. I'd like to avoid having XBL-specific rules for what can and can't be loaded though. On the other hand, if we always allow any load when the triggeringPrincipal is the system principal, that makes it hard to reason about what the security properties are, since the loadingDocument will do very different things with the loaded resource depending on what type of thing we're loading.
I'm just saying that if aOriginPrincipal is system we can pass SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS instead of SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS to NS_NewChannelWithTriggeringPrincipal in nsXBLService::FetchBindingDocument. That's XBL-specific, but no more so than the fact that CSS passing SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS all the time is CSS-specific, right? > On the other hand, if we always allow any load when the triggeringPrincipal is the system > principal We shouldn't do that.
I've definitely tried to stay away from having callsites dynamically choose security policy based on things like calling principals, since that basically would get us back to the chaos where every callsite implements its own security policy. That said, this is not a particularly bad instance of "doing its own policy". So I could live with that. But is there a reason you prefer that over not allowing about: when ALLOW_CHROME is set? I'm happy to rename ALLOW_CHROME to something more generic, like ALLOW_SYSTEM_PROTOCOLS.
Probably ALLOW_INTERNAL_PROTOCOLS, system gives the impression it have something to do with the OS
(In reply to Boris Zbarsky [:bz] from comment #6) > In particular, nsXBLService::FetchBindingDocument in that situation will > have aOriginPrincipal set to the principal of the stylesheet, but seems to > be using aBoundDocument (the untrusted thing) for the triggering principal. I actually tested that yesterday when experimenting - and it was definitely the other way round. The triggering principal was the system principal (our asyncOpen2 implementation actually enforces that), the loading principal belonged to the document that the user stylesheet applied to. As far as Adblock Plus goes, our custom asyncOpen2 implementation seems to do the job, and I've successfully got that one into the content process as well (will land shortly). So as far as we are concerned there is no need to create inconsistent security policies. However, I can imagine some user stylesheets (applied via Stylish and the like) loading XBL from file: or data:.
> But is there a reason you prefer that over not allowing about: when ALLOW_CHROME is set? Hmm. I guess we could make SEC_ALLOW_CHROME generally closer in spirit to nsIScriptSecurityManager::ALLOW_CHROME, sure.
Sounds like a regression range isn't needed here at this point. Feel free to add it back if I'm mistaken.
Keywords: regressionwindow-wanted
> Hmm. I guess we could make SEC_ALLOW_CHROME generally closer in spirit to > nsIScriptSecurityManager::ALLOW_CHROME, sure. This would be my preferred solution. I'll hack this up if you don't feel that the other approach is better.
Assignee: nobody → jonas
Making SEC_ALLOW_CHROME more like ALLOW_CHROME makes sense to me, yes.
Hmm.. doesn't look like ALLOW_CHROME allows access to about: though.
Tracking since this sounds like a regression in 43. I assume it also may be affecting 44.
status-firefox42: --- → unaffected
status-firefox44: --- → affected
tracking-firefox43: ? → +
tracking-firefox44: --- → +
Wladimir: Would you be able to test a build with a potential fix? Alternatively if you could point me to a version of the addon which uses an about: URI with URI_SAFE_FOR_UNTRUSTED_CONTENT. A build should show up in a few hours here https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/sicking@mozilla.com-81e4a023d7c6
I already fixed that issue on our end (development builds work correctly) but - sure, I can test the release version with the try builds once they show up.
Comment on attachment 8682279 [details] [diff] [review] Allow URI_IS_UI_RESOURCE and safe about: URIs when SEC_ALLOW_CHROME is set >+URIHasFlags(nsIURI* aURI, uint32_t aURIFlags) Should be static, right? > DoSOPChecks(nsIURI* aURI, nsILoadInfo* aLoadInfo) And this. r=me
Attachment #8682279 - Flags: review?(bzbarsky) → review+
Comment on attachment 8682279 [details] [diff] [review] Allow URI_IS_UI_RESOURCE and safe about: URIs when SEC_ALLOW_CHROME is set Approval Request Comment [Feature/regressing bug #]:bug 1213646 [User impact if declined]:Will break addons which host XBL files on resource:// and about: URLs [Describe test coverage new/current, TreeHerder]: There's no tests for this yet. I'm hoping to hear back from the adblockplus developers that the patch fixes the regression that they are seeing. [Risks and why]: The risk should be quite low since this only affects XBL files loaded into content documents, which is mainly done by addons, and the built-in functionality that does it is well tested. And since it expands what XBL files can be loaded, the risk is even lower. [String/UUID change made/needed]: None
Attachment #8682279 - Flags: approval-mozilla-beta?
Attachment #8682279 - Flags: approval-mozilla-aurora?
Wladimir: Apparently the way that I linked to the tryserver build doesn't work these days. The next nightly should work though. I'll keep you up to date.
Sure, nightly is easier to test.
https://hg.mozilla.org/mozilla-central/rev/a7581915fe47
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Wladimir: if you grab a nightly now, it should hopefully work?
Flags: needinfo?(trev.moz)
2015-11-05 nightly has this fix, I tested with this one. I went back to Adblock Plus 2.6.11 release: * In E10S mode, the XBL is allowed to load if Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT flag is specified. Otherwise I see NS_ERROR_DOM_BAD_URI being thrown (that happens with the unmodified release). * Even though the XBL seems to be loading just fine, I see "cpow is null" errors every time, originating from http://hg.mozilla.org/mozilla-central/file/cc48981c026c/toolkit/components/addoncompat/RemoteAddonsParent.jsm#l252. That's an issue with the compatibility shims rather than security checks, unrelated. * If E10S is disabled, things will still fail. I think that's expected given that the channel in the released version doesn't implement asyncOpen2 yet. Only the first bullet point should be relevant in the context of this bug, and I think that the behavior is correct now.
Flags: needinfo?(trev.moz)
Sounds great! Thanks! (Sorry that this new security infrastructure is causing some churn, hopefully we should be through soon)
Comment on attachment 8682279 [details] [diff] [review] Allow URI_IS_UI_RESOURCE and safe about: URIs when SEC_ALLOW_CHROME is set Since adblock plus is our #1 addon and the fact that the fix was verified (see comment 39), this seems safe to uplift to Aurora44. We could also take it in Beta once it has stabilized a bit on Aurora.
Attachment #8682279 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8682279 [details] [diff] [review] Allow URI_IS_UI_RESOURCE and safe about: URIs when SEC_ALLOW_CHROME is set Approved for uplift to beta, should improve addon functioning. I'd like this in beta 3 so we can test it out there.
Attachment #8682279 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Hello, I'm trying to verify this bug and I have to reproduce the initial behavior using an older affected build. I used the Nightly from 2015.10.10 and Adblock plugin (2.6.11.4012-beta, Version 2.6.11.4024-beta and 2.6.11) but I could not reproduce the issue described in comment 0: 'NS_ERROR_DOM_BAD_URI: Access to restricted URI denied.' Using latest Nightly from 2015.11.16 I do get: 'NS_ERROR_DOM_BAD_URI: Component returned failure code: 0x805303f4 [nsIContentSecurityManager.performSecurityCheck] RemoteAddonsChild.jsm:283:0'. But from what I see in comment 39 this is not the issue here. Am I missing something when I try to reproduce this in order to verify that the fix arrived in beta?
Flags: needinfo?(jonas)
As mentioned in comment 39, this fix by itself isn't sufficient to make current Adblock Plus release (or older devbuilds) work. Newer Adblock Plus devbuilds on the other hand will work regardless (different solution applied there). So verifying this with Adblock Plus won't work...
I think comment 39 is enough verification here.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #48) > I think comment 39 is enough verification here. Thanks. Dropping qe-verify.
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: