562547 - (CVE-2010-1585) ParanoidFragmentSinks allow javascript: urls in chrome documents

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Veditz [:dveditz]]

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]

taking

Comment 2

[Robert Sayre]

Attachment: null principal

I'll add tests if jst thinks this is the right way to go

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 442465 [details] [diff] [review]
null principal

Looks good to me.

Comment 4

[Reed Loden [:reed]]

*ping*... jst gave r+, so can we get this checked-in with tests?

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 442465 [details] [diff] [review]
null principal

would like to get this into the branches, too.

Comment 6

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

A testcase would be good.

Comment 7

[LegNeato]

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

[LegNeato]

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)]

Attachment: same patch, updated to trunk

Comment 10

[Ben Bucksch (:BenB)]

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)]

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

[LegNeato]

Comment on attachment 442465 [details] [diff] [review]
null principal

a=LegNeato for 1.9.2.14

Comment 13

[Ben Bucksch (:BenB)]

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)]

Attachment: same patch, for 1.9.2 branch -- commited

Comment 15

[Ben Bucksch (:BenB)]

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)]

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)]

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

Comment 18

[Ben Bucksch (:BenB)]

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 [:bzbarsky]]

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)]

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 [:bzbarsky]]

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)]

> 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 [:bzbarsky]]

> 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)]

> 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 [:bzbarsky]]

Thank you!

Please do file a followup on the general behavior of DISALLOW_INHERIT_PRINCIPAL?

Comment 26

[Reed Loden [:reed]]

Is 1.9.1 affected? If so, need a patch for it and a request for approval.

Comment 27

[Ben Bucksch (:BenB)]

reed: go ahead. I personally don't care about < FF 3.6.

Comment 28

[Reed Loden [:reed]]

Attachment: same patch, for 1.9.1 branch

Comment 29

[Daniel Veditz [:dveditz]]

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]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/612ff691af63

Comment 31

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

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?