458883 - Make Document.documentURI and .textContent noAccess in mailnews

Status: VERIFIED FIXED

(MailNews Core :: Security, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 453928 comment 5.

We need to fix this on the 1.8 branch and 1.9.0 branch no matter what we're doing on trunk.

Comment 1

[Samuel Sidler (old account; do not CC)]

Boris, there are no shipping mailnews products from 1.9.0. Are you sure we need to fix it there?

CCing dmose to look at this (or assign it).

Comment 2

[Boris Zbarsky [:bzbarsky]]

No idea.  I just flagged this as blocking? for all stable branches where this would be a problem.  If you don't ever plan to ship Thunderbird or Seamonkey from 1.9.0.x, then no need to worry about it there too much, but it might be better to just fix and not worry than try to guess.  It's a one-liner...

Oh, I'll happily r+sr the fix, of course.  ;)

Comment 3

[Phil Ringnalda (:philor)]

It's actually a two-liner, since I realized this afternoon that we never blocked .textContent either, which means that we only stopped bug 66938 for a bit under two years, and have been completely open to it since June 2003.

And we want to take it for 1.9.0 because even though *we* aren't shipping from it, we don't actually know that, for instance, kreeger isn't going to ship a Correo from 1.9.0, or that someone we don't even know is already shipping from it. What's that thing mscott's been doing, and what Gecko does it ship?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, I'd assumed we prevented innerHTML because of getting resolved URIs or something.

If we're preventing textContent we probably also need to kill nsIDOM3Text.wholeText.

Times like this I really wish we had brendan's tainting stuff working...

Comment 5

[Boris Zbarsky [:bzbarsky]]

And we prevent *.attributes.get, *.getAttribute, and *.getAttributeNS, but not getAttributeNode or getAttributeNodeNS, as long as we're tightening stuff up here.

Comment 6

[Phil Ringnalda (:philor)]

Attachment: test mbox

Be nice if we had Mochitest-alike infrastructure (well, and had it on 1.8, since we're not doing script on the trunk right now), but at least this makes ad-hoc testing a little easier: drop it in Mail/Local Folders in a Tb2.x profile and view the message with javascript.allow.mailnews enabled, and it'll alert the documentURI (including the path to your profile, for easier attacking), then the textContent of the body to demonstrate that bug 66938 has been wide open again since 2003.

(I do have a nice automatic wiretap message, that will send my web server everything that the first recipient says to the person they forward it to, along with the path to both their profiles, but I figured nobody would really want to test with that.)

Comment 7

[Phil Ringnalda (:philor)]

Attachment: Whack-some-moles, v.1

Against 1.8, since that's what worries me most, this is what I want to land on mozilla-central, 1.9.0, and 1.8.

With wholeText being 1.9.1-only, and 1.9.1 not having any content JS in mailnews at the moment, I'll get it separately (hopefully along with a reordering that will make it clear what we're actually doing, so things like the missing getAttributeNode(NS) will stand out better).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 342203 [details] [diff] [review]
Whack-some-moles, v.1

That should probably be *.documentURI, not HTMLDocument.documentURI.  Otherwise if we somehow enable creation of XML documents with the current URI the caller will be able to extract it.

Comment 9

[Phil Ringnalda (:philor)]

Comment on attachment 342203 [details] [diff] [review]
Whack-some-moles, v.1

* it is, then - I wasn't sure how expensive they are.

Drivers: sorry, no place other than 1.8.1 it *can* bake.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 342203 [details] [diff] [review]
Whack-some-moles, v.1

>+pref("capability.policy.mailnews.HTMLDocument.documentURI", "noAccess");

Sorry to nitpick but can we get a patch with the "*" as you intend to check it in? Often times downstream vendors work from the bug patch rather than bonsai so it's important to approve what's actually being checked in on the old branches.

Comment 11

[Phil Ringnalda (:philor)]

Attachment: For checkin, 1.8 branch

Comment 12

[Phil Ringnalda (:philor)]

Attachment: For checkin, 1.9.0 branch

Second verse, same as the first (other than 31 lines of offset, but to me you are describing possible users of it as both psychotic and utterly random, so maybe avoiding the offset is a good thing).

Comment 13

[Phil Ringnalda (:philor)]

Attachment: For checkin, mozilla-central trunk

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 342539 [details] [diff] [review]
For checkin, 1.8 branch

Approved for 1.8.1.18, a=dveditz for release-drivers

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 342540 [details] [diff] [review]
For checkin, 1.9.0 branch

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 16

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

Any suggestions on how QA can verify this fix?

Comment 17

[Phil Ringnalda (:philor)]

Comment 6 - feel free to email me if you need more explicit directions.

Comment 18

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

Ah, right, I was skimming too quickly and didn't process your comment (obviously). This should be fine for verification.

Comment 19

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

Verified for 1.8.1.18 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008110403 Thunderbird/2.0.0.18pre. I reproduced the bug in 1.8.1.17 using Phil's test mbox.

Comment 20

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

Verified for 1.9.0.4 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081104 Shredder/3.0b1pre.

Comment 21

[Alexander Sack]

Comment on attachment 342539 [details] [diff] [review]
For checkin, 1.8 branch

a=asac for 1.8.0 (needs a bit context adjusting to apply cleanly)