418119 - nsIContentPolicy not called for external DTDs of XML documents

Status: RESOLVED FIXED

(Core :: Security, defect, P3)

Description

[Mike Perry]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7

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 2.0.0.13 as well.

Reproducible: Always

Comment 1

[Wladimir Palant]

Attachment: Possible patch

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.

Comment 2

[Daniel Veditz [:dveditz]]

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 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[Wladimir Palant]

(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.

Comment 5

[Wladimir Palant]

Attachment: Patch for 1.8 branch

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.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304443 [details] [diff] [review]
Possible patch

a=beltzner for 1.9

Comment 7

[Reed Loden [:reed]]

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

Comment 8

[Wladimir Palant]

Confirmed - fixed in 2008022204 Minefield nightly.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Do we want to take this for branch?

Comment 10

[Reed Loden [:reed]]

(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...

Comment 11

[Joshua Cranmer [:jcranmer]]

This patch breaks loading TB's with a null nsCOMPtr dereference...

Comment 12

[Wladimir Palant]

Joshua, can you give some more details? I have no idea what you mean...

Comment 13

[Joshua Cranmer [:jcranmer]]

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.

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

This seems to have caused bug 420609.

Comment 16

[Daniel Veditz [:dveditz]]

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.

Comment 17

[Alex Vincent [:WeirdAl]]

So this is why Verbosio went haywire... :-)

Comment 18

[Daniel Veditz [:dveditz]]

(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 2.0.0.14 when we land these patches on the 1.8 branch.

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 20

[Wladimir Palant]

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.