Last Comment Bug 490790 - (CVE-2010-0182) XMLDocument::load() doesn't check nsIContentPolicy
(CVE-2010-0182)
: XMLDocument::load() doesn't check nsIContentPolicy
Status: VERIFIED FIXED
[sg:low+]
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on:
Blocks: abp CSP
  Show dependency treegraph
 
Reported: 2009-04-30 01:32 PDT by Wladimir Palant
Modified: 2014-08-22 12:42 PDT (History)
11 users (show)
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.2-fixed
.9-fixed


Attachments
Patch to fix (5.65 KB, patch)
2010-02-25 17:33 PST, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: review+
Details | Diff | Splinter Review
Patch v2 (5.72 KB, patch)
2010-02-26 17:04 PST, Jonas Sicking (:sicking) PTO Until July 5th
dveditz: review+
dveditz: approval1.9.2.2+
dveditz: approval1.9.1.9+
Details | Diff | Splinter Review

Description Wladimir Palant 2009-04-30 01:32:29 PDT
nsXMLDocument::load() will do same-origin checks on the URL to be loaded but won't check content policies. I think it should call content policies with TYPE_XMLHTTPREQUEST and use calling document as context.
Comment 1 Daniel Veditz [:dveditz] 2009-11-06 21:02:47 PST
All kinds of things use content policies, and some of them are security related.
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2009-11-06 22:53:31 PST
I guess I can take this. Not sure if using TYPE_XMLHTTPREQUEST is ok. I agree it's conceptually the same, i just worry that people expect to be able to get to an XHR object.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2010-02-25 17:33:53 PST
Created attachment 429031 [details] [diff] [review]
Patch to fix

Wladimir, it would be great if you wanna try to run with this patch to see if things work properly.
Comment 4 Daniel Veditz [:dveditz] 2010-02-25 18:30:34 PST
Comment on attachment 429031 [details] [diff] [review]
Patch to fix

r=dveditz

In the old code, lack of codebase is not equal to system principal!
Comment 5 Wladimir Palant 2010-02-26 04:33:57 PST
I don't think I will be able to test before the patch lands.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2010-02-26 17:04:03 PST
Created attachment 429272 [details] [diff] [review]
Patch v2

Turns out that the first patch breaks a bunch of mochitests. The problem is that if we use the document itself as context, the nsDataDocumentContentPolicy content policy blocks the load. Instead we should using the *calling* document as the context.

This patch does that.
Comment 7 Daniel Veditz [:dveditz] 2010-03-02 11:32:14 PST
Comment on attachment 429272 [details] [diff] [review]
Patch v2

r=dveditz
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2010-03-02 11:43:57 PST
Checked in. Thanks for finding this!

http://hg.mozilla.org/mozilla-central/rev/ed1612a1ffa8
Comment 9 Daniel Veditz [:dveditz] 2010-03-05 13:22:38 PST
Comment on attachment 429272 [details] [diff] [review]
Patch v2

Approved for 1.9.2.2, and 1.9.1.9, a=dveditz for release-drivers
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2010-03-08 12:05:18 PST
Checked in to 1.9.1 branch for 1.9.1.9
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4d4f3726d884

And to 1.9.2 branch for 1.9.2.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/46d94cbc195f
Comment 11 Al Billings [:abillings] 2010-03-15 17:07:54 PDT
Is there a testcase or some sort of manual reproduction steps for this?
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2010-03-15 17:16:42 PDT
I don't think we have any tests for contentpolicies. The best thing would be if Wladimir could check as I believe he has good tests for contentpolicies.
Comment 13 Wladimir Palant 2010-03-16 02:24:03 PDT
I added this test to my test suite: https://hg.adblockplus.org/adblockplus/rev/235e750203cb (tested without the version check however, will remove it in the test suite once the releases are out).

Fails in:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.9pre) Gecko/20100307 Shiretoko/3.5.9pre
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2pre) Gecko/20100307 Namoroka/3.6.2pre
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100301 Minefield/3.7a3pre

Succeeds in:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.9pre) Gecko/20100315 Shiretoko/3.5.9pre
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.2pre) Gecko/20100315 Namoroka/3.6.2pre
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100312 Minefield/3.7a3pre

Marking as verified.
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2010-03-16 02:29:52 PDT
Thanks Wladimir!
Comment 15 Al Billings [:abillings] 2010-03-16 08:29:16 PDT
Adding verified1.9.1 and verified1.9.2 keywords.

Note You need to log in before you can comment on or make changes to this bug.