Last Comment Bug 562547 - (CVE-2010-1585) ParanoidFragmentSinks allow javascript: urls in chrome documents
(CVE-2010-1585)
: ParanoidFragmentSinks allow javascript: urls in chrome documents
Status: RESOLVED FIXED
[sg:vector-critical] [qa-examined-191...
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert Sayre
:
: Andrew Overholt [:overholt]
Mentors:
http://www.security-assessment.com/fi...
: 568395 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-28 19:39 PDT by Daniel Veditz [:dveditz]
Modified: 2015-10-16 11:52 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
needed
.14-fixed
needed
.17-fixed


Attachments
null principal (7.34 KB, patch)
2010-04-29 11:57 PDT, Robert Sayre
jst: review+
christian: approval1.9.2.14+
christian: approval1.9.2.9-
christian: approval1.9.1.12-
Details | Diff | Splinter Review
same patch, updated to trunk (7.71 KB, patch)
2010-12-14 10:37 PST, Ben Bucksch (:BenB)
ben.bucksch: review+
Details | Diff | Splinter Review
same patch, for 1.9.2 branch -- commited (7.28 KB, patch)
2010-12-14 12:07 PST, Ben Bucksch (:BenB)
ben.bucksch: review+
Details | Diff | Splinter Review
same patch, for 1.9.1 branch (7.29 KB, patch)
2010-12-21 17:10 PST, Reed Loden [:reed] (use needinfo?)
reed: review+
dveditz: review+
dveditz: approval1.9.1.17+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2010-04-28 19:39:03 PDT
The two ns(X)HTMLParanoidFragmentSink classes are used by nsIScriptableUnescapeHTML to sanitize (X)HTML by stripping attributes and tags not on a built-in whitelist. Internally to Firefox this is only used by the feed processor, but it's fairly popular with add-on authors and recommended as a best-practice.

The sinks attempt to sanitize URLs by calling CheckLoadURI[...]DISALLOW_INHERIT_PRINCIPAL), but unfortunately when the target document is a chrome document (as is common with add-ons) this check allows any URI. In particular malicious href="javascript:evil()" or <iframe src="data:evil"> can slip through and create sg-critical bugs.

http://hg.mozilla.org/mozilla-central/annotate/c753325a40ff/content/html/document/src/nsHTMLFragmentContentSink.cpp#l967

and ditto content/xml/document/src/nsXMLFragmentContentSink.cpp

Would it be OK to simply always pass a nsNullPrincipal in instead?

This problem was recently published in a whitepaper referenced at
http://www.securityfocus.com/archive/1/archive/1/510883/100/0/threaded

Cross Context Scripting with Firefox - Roberto Suggi Liverani
Link: http://www.security-assessment.com/files/whitepapers/Cross_Context_Scrip
ting_with_Firefox.pdf

MITRE has apparently assigned this CVE-2010-1585
Comment 1 Robert Sayre 2010-04-28 19:57:11 PDT
taking
Comment 2 Robert Sayre 2010-04-29 11:57:06 PDT
Created attachment 442465 [details] [diff] [review]
null principal

I'll add tests if jst thinks this is the right way to go
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2010-04-29 12:03:25 PDT
Comment on attachment 442465 [details] [diff] [review]
null principal

Looks good to me.
Comment 4 Reed Loden [:reed] (use needinfo?) 2010-07-01 01:15:17 PDT
*ping*... jst gave r+, so can we get this checked-in with tests?
Comment 5 Daniel Veditz [:dveditz] 2010-08-09 12:12:54 PDT
Comment on attachment 442465 [details] [diff] [review]
null principal

would like to get this into the branches, too.
Comment 6 Al Billings [:abillings] 2010-08-11 10:30:02 PDT
A testcase would be good.
Comment 7 christian 2010-08-11 10:30:10 PDT
Comment on attachment 442465 [details] [diff] [review]
null principal

a=LegNeato for 1.9.2.9 and 1.9.1.12.

Please note that for these releases code-freeze is this Thursday, 2010-08-12 @ 11:59 pm PST. This needs to be landed as soon as possible.
Comment 8 christian 2010-09-01 09:56:41 PDT
Comment on attachment 442465 [details] [diff] [review]
null principal

Removing .9 approval as this missed landing before freeze. Feel free to nominate again, though the bar for approval will be higher.
Comment 9 Ben Bucksch (:BenB) 2010-12-14 10:37:07 PST
Created attachment 497538 [details] [diff] [review]
same patch, updated to trunk
Comment 10 Ben Bucksch (:BenB) 2010-12-14 10:39:19 PST
We really dropped the ball here, this is a critical security hole and should have been commited immediately, not 8 months (!) later.

I'd like to commit this NOW, so that it makes beta 8.
I also think it should be commited to stable branches, esp. considering Thunderbird 3.1.
Comment 11 Ben Bucksch (:BenB) 2010-12-14 10:42:38 PST
Comment on attachment 442465 [details] [diff] [review]
null principal

See above. Critical hole, needed for Firefox 3.6, Thunderbird 3.1, and extensions.
Comment 12 christian 2010-12-14 10:57:52 PST
Comment on attachment 442465 [details] [diff] [review]
null principal

a=LegNeato for 1.9.2.14
Comment 13 Ben Bucksch (:BenB) 2010-12-14 12:00:38 PST
BTW: Why is that?

dveditz wrote in comment 0:
> The sinks attempt to sanitize URLs by calling
> CheckLoadURI[...]DISALLOW_INHERIT_PRINCIPAL), but unfortunately when
> the target document is a chrome document (as is common with add-ons)
> this check allows any URI.

It seems like DISALLOW_INHERIT_PRINCIPAL would forbid exactly that, and it would be most important to work in the case of chrome. There may be other code which wrongly relies on that. Did anybody search addons code and other thirdparty code?
<http://www.google.com/codesearch?q=DISALLOW_INHERIT_PRINCIPAL> shows a few other uses. Or am I misunderstanding the API?
Comment 14 Ben Bucksch (:BenB) 2010-12-14 12:07:22 PST
Created attachment 497566 [details] [diff] [review]
same patch, for 1.9.2 branch -- commited
Comment 15 Ben Bucksch (:BenB) 2010-12-14 12:27:31 PST
Comment on attachment 497566 [details] [diff] [review]
same patch, for 1.9.2 branch -- commited

Commited as <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1e85c1a49573>
Comment 16 Ben Bucksch (:BenB) 2010-12-15 09:15:20 PST
Comment on attachment 497538 [details] [diff] [review]
same patch, updated to trunk

Commited as <http://hg.mozilla.org/mozilla-central/rev/e952221c3251>
Comment 17 Ben Bucksch (:BenB) 2010-12-15 09:18:57 PST
I'd like to have comment 13 answered before I close this as FIXED.
Comment 18 Ben Bucksch (:BenB) 2010-12-15 13:04:50 PST
This might have caused test failure bug 619436, maybe also bug 619437 and bug 619438. Please see bug 619436 - I don't want to delude this security bug here with discussion about tests.

Particularly, I think we may need a more general fix here which fixes DISALLOW_INHERIT_PRINCIPAL to actually do what it says.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2010-12-16 10:38:24 PST
Fundamentally, using DISALLOW_INHERIT_PRINCIPAL is an abuse of what's a security check API.  And chrome code should be able to pass any security check...

This bug, if it's fixed, should be resolved accordingly.  If you think we should change the behavior of DISALLOW_INHERIT_PRINCIPAL for the system principal, that should be a separate bug.
Comment 20 Ben Bucksch (:BenB) 2010-12-17 05:18:01 PST
bz, I don't understand... The word DISALLOW_INHERIT_PRINCIPAL for me says: Ignore the passed in principal, do not use it to calculate the permissions. In other words, exactly what this patch did.
The description says: "Don't allow URLs which would inherit the caller's principal (such as javascript: or data:) to load." Here, javascript: URLs *did* load, directly violating what the flag promised.
Again, if I misunderstood something, please tell me where/why.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2010-12-21 14:48:31 PST
DISALLOW_INHERIT_PRINCIPAL was added to an existing security check which always returned "ok" for system principals.  Perhaps the check should have been redefined in the process; I'm happy to have a separate bug on that.  But why is THIS bug still open, given that the patch has landed?  Why is the checkin-needed keyword still present?  Why are the branch status flags indicating nothing has landed given that branch fixes have laded?  Am _I_ misunderstanding something about the state of this bug?
Comment 22 Ben Bucksch (:BenB) 2010-12-21 14:55:35 PST
> But why is THIS bug still open, given that the patch has landed?

Because I wanted this question to be resolved. As shown in the comments above, there are other third-party users of this flag, and they likely misunderstood this, too, in the same way.
From my point of view, this bug is not resolved until that flag behaves differently, but apparently you disagree, and that's fine.

Also, this bug may or may not have caused several test failures, specifically bug 619436. Given that the failure is intermittent, I don't know what (and don't want) to do with it. I can't see a problem in the app.

Leaving that to you...
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2010-12-21 15:29:19 PST
> Because I wanted this question to be resolved.

THIS BUG is about a particular issue in the paranoid fragment sink.  How about not hijacking it?

Did a patch for this bug land or not?  If so, which branches did it land on?
Comment 24 Ben Bucksch (:BenB) 2010-12-21 16:52:18 PST
> THIS BUG is about a particular issue in the paranoid fragment sink.

OK, marking FIXED.

> Did a patch for this bug land or not?  If so, which branches did it land on?

See comment 15 and 16:
Commited as <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1e85c1a49573>
Commited as <http://hg.mozilla.org/mozilla-central/rev/e952221c3251>
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2010-12-21 16:58:59 PST
Thank you!

Please do file a followup on the general behavior of DISALLOW_INHERIT_PRINCIPAL?
Comment 26 Reed Loden [:reed] (use needinfo?) 2010-12-21 16:59:13 PST
Is 1.9.1 affected? If so, need a patch for it and a request for approval.
Comment 27 Ben Bucksch (:BenB) 2010-12-21 17:03:01 PST
reed: go ahead. I personally don't care about < FF 3.6.
Comment 28 Reed Loden [:reed] (use needinfo?) 2010-12-21 17:10:18 PST
Created attachment 499199 [details] [diff] [review]
same patch, for 1.9.1 branch
Comment 29 Daniel Veditz [:dveditz] 2010-12-21 17:32:30 PST
Comment on attachment 499199 [details] [diff] [review]
same patch, for 1.9.1 branch

r=dveditz

approved for 1.9.1.17, a=dveditz

Thanks for the merge to 1.9.1!
Comment 30 Reed Loden [:reed] (use needinfo?) 2010-12-21 17:40:42 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/612ff691af63
Comment 31 Al Billings [:abillings] 2011-01-06 11:24:39 PST
No manual testcase, no STR, and no automated test.

What is QA supposed to do here to verify this since it was based ona whitepaper?
Comment 32 Boris Zbarsky [:bz] (still a bit busy) 2011-03-07 17:48:02 PST
*** Bug 568395 has been marked as a duplicate of this bug. ***

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