Last Comment Bug 458883 - Make Document.documentURI and .textContent noAccess in mailnews
: Make Document.documentURI and .textContent noAccess in mailnews
: verified1.8.1.18, verified1.9.0.4
Product: MailNews Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
: -- major (vote)
: ---
Assigned To: Phil Ringnalda (:philor)
Depends on:
Blocks: 453928
  Show dependency treegraph
Reported: 2008-10-07 06:39 PDT by Boris Zbarsky [:bz]
Modified: 2009-01-05 13:51 PST (History)
8 users (show)
dveditz: blocking1.9.0.4+
dveditz: blocking1.8.1.18+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

test mbox (1.37 KB, text/plain)
2008-10-07 23:14 PDT, Phil Ringnalda (:philor)
no flags Details
Whack-some-moles, v.1 (2.79 KB, patch)
2008-10-07 23:20 PDT, Phil Ringnalda (:philor)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.18-
dveditz: approval1.9.0.4-
Details | Diff | Splinter Review
For checkin, 1.8 branch (2.79 KB, patch)
2008-10-09 20:47 PDT, Phil Ringnalda (:philor)
dveditz: approval1.8.1.18+
Details | Diff | Splinter Review
For checkin, 1.9.0 branch (2.77 KB, patch)
2008-10-09 20:58 PDT, Phil Ringnalda (:philor)
dveditz: approval1.9.0.4+
Details | Diff | Splinter Review
For checkin, mozilla-central trunk (2.58 KB, patch)
2008-10-09 21:08 PDT, Phil Ringnalda (:philor)
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2008-10-07 06:39:04 PDT
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) 2008-10-07 08:29:53 PDT
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 [:bz] 2008-10-07 09:43:42 PDT
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) 2008-10-07 20:49:05 PDT
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 [:bz] 2008-10-07 21:07:07 PDT
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 [:bz] 2008-10-07 21:09:48 PDT
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) 2008-10-07 23:14:09 PDT
Created attachment 342202 [details]
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) 2008-10-07 23:20:48 PDT
Created attachment 342203 [details] [diff] [review]
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 [:bz] 2008-10-08 06:29:09 PDT
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) 2008-10-08 08:51:25 PDT
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] 2008-10-09 09:46:19 PDT
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) 2008-10-09 20:47:21 PDT
Created attachment 342539 [details] [diff] [review]
For checkin, 1.8 branch
Comment 12 Phil Ringnalda (:philor) 2008-10-09 20:58:38 PDT
Created attachment 342540 [details] [diff] [review]
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) 2008-10-09 21:08:12 PDT
Created attachment 342542 [details] [diff] [review]
For checkin, mozilla-central trunk
Comment 14 Daniel Veditz [:dveditz] 2008-10-10 11:05:42 PDT
Comment on attachment 342539 [details] [diff] [review]
For checkin, 1.8 branch

Approved for, a=dveditz for release-drivers
Comment 15 Daniel Veditz [:dveditz] 2008-10-10 11:05:55 PDT
Comment on attachment 342540 [details] [diff] [review]
For checkin, 1.9.0 branch

Approved for, a=dveditz for release-drivers
Comment 16 Al Billings [:abillings] 2008-10-23 17:00:50 PDT
Any suggestions on how QA can verify this fix?
Comment 17 Phil Ringnalda (:philor) 2008-10-23 17:29:23 PDT
Comment 6 - feel free to email me if you need more explicit directions.
Comment 18 Al Billings [:abillings] 2008-10-23 17:31:50 PDT
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] 2008-11-04 13:39:19 PST
Verified for with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008110403 Thunderbird/ I reproduced the bug in using Phil's test mbox.
Comment 20 Al Billings [:abillings] 2008-11-04 13:42:42 PST
Verified for 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 2008-11-10 09:18:23 PST
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)

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