Closed Bug 418119 Opened 12 years ago Closed 12 years ago
IContent Policy not called for external DTDs of XML documents
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52) Gecko/20070914 Firefox/184.108.40.206 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:220.127.116.11) Gecko/20070914 Firefox/18.104.22.168 XML documents can source chrome and resource URLS in their DTDs without a call to nsIContentPolicy::shouldLoad. This makes it impossible for extensions such as Adblock and Torbutton to prevent websites from enumerating a user's chrome urls for vulnerable extensions, or to prevent them from using installed extension information in a fingerprint for tracking purposes. See the provided URL for a test case. From what I can tell, it appears this call should be at http://mxr.mozilla.org/firefox/source/parser/htmlparser/src/nsExpatDriver.cpp#763. I'd submit a patch, but I'm not entirely sure which principle or element I should use to hand to the content policy. I'm guessing this check definitely should be added before Bug 22942 is fixed, but it would be nice if it could be added to 22.214.171.124 as well. Reproducible: Always
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Firefox → Core
Trunk patch, the patch for 1.8.1 branch will have to be slightly different (have to pass URI instead of principal). This adds a whole bunch of new dependencies to the parser - but that is probably unavoidable. Better question is whether we can expect our sink to implement nsIContentSink - nsIExpatSink doesn't inherit from it but I don't really see any other way to get context information required. With this patch Adblock Plus receives the request to chrome://adblockplus/locale/about.dtd from content and stops it as expected.
Assignee: nobody → trev.moz
Status: NEW → ASSIGNED
Attachment #304443 - Flags: review?(dveditz)
Attachment #304443 - Flags: review?(dveditz) → review?(jonas)
To the extent security mechanism depend on content policy this is a security problem in addition to a privacy problem. Not sure we have any, yet, but we have considered it.
Comment on attachment 304443 [details] [diff] [review] Possible patch Do we really hit this code? We're not currently loading DTDs for XML documents other than for chrome.
(In reply to comment #3) > Do we really hit this code? We're not currently loading DTDs for XML documents > other than for chrome. One reason why this is a problem: Adblock Plus prevents content from accessing chrome://adblockplus/ do prevent extension detection (http://adblockplus.org/en/faq_internal#protectchrome). DTDs are a loophole in this protection.
Attachment #304443 - Flags: superreview?(peterv) → superreview+
Jonas, could you or somebody else test whether this works? I don't see why it shouldn't but I cannot compile 1.8 branch.
Attachment #304711 - Flags: review?(jonas)
Comment on attachment 304443 [details] [diff] [review] Possible patch a=beltzner for 1.9
Attachment #304443 - Flags: approval1.9? → approval1.9+
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Checking in parser/htmlparser/src/Makefile.in; /cvsroot/mozilla/parser/htmlparser/src/Makefile.in,v <-- Makefile.in new revision: 1.106; previous revision: 1.105 done Checking in parser/htmlparser/src/nsExpatDriver.cpp; /cvsroot/mozilla/parser/htmlparser/src/nsExpatDriver.cpp,v <-- nsExpatDriver.cpp new revision: 3.108; previous revision: 3.107 done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Confirmed - fixed in 2008022204 Minefield nightly.
Do we want to take this for branch?
(In reply to comment #9) > Do we want to take this for branch? There's a branch patch waiting on review, so I assume so...
This patch breaks loading TB's with a null nsCOMPtr dereference...
Joshua, can you give some more details? I have no idea what you mean...
When I last updated my TB build, it gave me the following assertion and then segfaulted: ###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRawPtr != 0', file ../../../dist/include/xpcom/nsCOMPtr.h, line 868 nsContentPolicy::CheckPolicy nsContentPolicy::ShouldLoad NS_CheckContentLoadPolicy nsExpatDriver::OpenInputStreamFromExternalDTD The latter function was changed by this patch, so I tested doing a patch -R with the given attachment and the null-nsCOMPtr disappeared. If it matters, I am testing on a shared build with a symlink chrome file format.
Joshua: have you filed a regression bug on the Thunderbird crash? If so please make it "block" this bug, if not please file one with your symptoms.
This seems to have caused bug 420609.
Given unfixed trunk regressions we're going to pass on this for '13 since it's unlikely the trunk will open in time to catch further regressions before our branch code-freeze. Moving nomination to the next release.
So this is why Verbosio went haywire... :-)
(In reply to comment #11) > This patch breaks loading TB's with a null nsCOMPtr dereference... Are you still seeing the problem, or did the fix for bug 420609 fix that too? If you're still seeing a crash _please_ file a new bug. We'd like to be forewarned before breaking Thunderbird 126.96.36.199 when we land these patches on the 1.8 branch.
Not blocking the branch, for people who need this fixed it looks like they need to jump to Firefox 3/Gecko 1.9. Wladimir is unable to backport this at this time and it's risky with regressions.
Flags: blocking188.8.131.52+ → blocking184.108.40.206-
Attachment #304711 - Flags: review?(jonas) → review+
Looking at the regressions, I am not going to push this for 1.8 branch unless somebody gives me a good reason. More than 50% of Adblock Plus users already upgraded to Firefox 3 which makes this issue unattractive for exploitation.
You need to log in before you can comment on or make changes to this bug.