Bug 562547 (CVE-2010-1585)

ParanoidFragmentSinks allow javascript: urls in chrome documents

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: dveditz, Assigned: Robert Sayre)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 needed, status1.9.2 .14-fixed, blocking1.9.1 needed, status1.9.1 .17-fixed)

Details

(Whiteboard: [sg:vector-critical] [qa-examined-191] [qa-examined-192] [qa-needs-STR], URL)

Attachments

(4 attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
Whiteboard: [sg:vector-critical]
(Assignee)

Updated

7 years ago
Assignee: nobody → sayrer
(Assignee)

Comment 1

7 years ago
taking
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

7 years ago
Created attachment 442465 [details] [diff] [review]
null principal

I'll add tests if jst thinks this is the right way to go
Attachment #442465 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #442465 - Flags: review? → review?(jst)
Comment on attachment 442465 [details] [diff] [review]
null principal

Looks good to me.
Attachment #442465 - Flags: review?(jst) → review+
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
*ping*... jst gave r+, so can we get this checked-in with tests?
blocking2.0: --- → ?

Updated

7 years ago
blocking2.0: ? → final+
(Reporter)

Comment 5

7 years ago
Comment on attachment 442465 [details] [diff] [review]
null principal

would like to get this into the branches, too.
Attachment #442465 - Flags: approval1.9.2.9?
Attachment #442465 - Flags: approval1.9.1.12?
A testcase would be good.
Keywords: testcase-wanted

Comment 7

7 years ago
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.
Attachment #442465 - Flags: approval1.9.2.9?
Attachment #442465 - Flags: approval1.9.2.9+
Attachment #442465 - Flags: approval1.9.1.12?
Attachment #442465 - Flags: approval1.9.1.12+

Comment 8

7 years ago
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.
Attachment #442465 - Flags: approval1.9.2.9-
Attachment #442465 - Flags: approval1.9.2.9+
Attachment #442465 - Flags: approval1.9.1.12-
Attachment #442465 - Flags: approval1.9.1.12+
Keywords: checkin-needed

Comment 9

7 years ago
Created attachment 497538 [details] [diff] [review]
same patch, updated to trunk
Attachment #497538 - Flags: review+

Comment 10

7 years ago
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

7 years ago
Comment on attachment 442465 [details] [diff] [review]
null principal

See above. Critical hole, needed for Firefox 3.6, Thunderbird 3.1, and extensions.
Attachment #442465 - Flags: approval1.9.2.14?

Updated

7 years ago
Attachment #497538 - Flags: approval2.0?

Comment 12

7 years ago
Comment on attachment 442465 [details] [diff] [review]
null principal

a=LegNeato for 1.9.2.14
Attachment #442465 - Flags: approval1.9.2.14? → approval1.9.2.14+

Comment 13

7 years ago
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

7 years ago
Created attachment 497566 [details] [diff] [review]
same patch, for 1.9.2 branch -- commited
Attachment #497566 - Flags: review+

Comment 15

7 years ago
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>
Attachment #497566 - Attachment description: same patch, for 1.9.2 branch → same patch, for 1.9.2 branch -- commited

Updated

7 years ago
Attachment #497538 - Flags: approval2.0?

Comment 16

7 years ago
Comment on attachment 497538 [details] [diff] [review]
same patch, updated to trunk

Commited as <http://hg.mozilla.org/mozilla-central/rev/e952221c3251>

Updated

7 years ago

Comment 17

7 years ago
I'd like to have comment 13 answered before I close this as FIXED.

Comment 18

7 years ago
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.
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

7 years ago
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.
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

7 years ago
> 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...
Keywords: checkin-needed

Updated

7 years ago
status1.9.2: wanted → .14-fixed
status2.0: ? → ---
> 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

7 years ago
> 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>
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Thank you!

Please do file a followup on the general behavior of DISALLOW_INHERIT_PRINCIPAL?
Is 1.9.1 affected? If so, need a patch for it and a request for approval.

Comment 27

7 years ago
reed: go ahead. I personally don't care about < FF 3.6.
Created attachment 499199 [details] [diff] [review]
same patch, for 1.9.1 branch
Attachment #499199 - Flags: review+
Attachment #499199 - Flags: approval1.9.1.17?
(Reporter)

Comment 29

7 years ago
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!
Attachment #499199 - Flags: review+
(Reporter)

Updated

7 years ago
Attachment #499199 - Flags: approval1.9.1.17? → approval1.9.1.17+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/612ff691af63
status1.9.1: wanted → .17-fixed
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?
Whiteboard: [sg:vector-critical] → [sg:vector-critical] [qa-examined-191] [qa-examined-192] [qa-needs-STR]
Duplicate of this bug: 568395
(Reporter)

Updated

7 years ago
Group: core-security
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.