Make Document.documentURI and .textContent noAccess in mailnews

VERIFIED FIXED

Status

MailNews Core
Security
--
major
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: bz, Assigned: philor)

Tracking

({verified1.8.1.18, verified1.9.0.4})

unspecified
verified1.8.1.18, verified1.9.0.4
Bug Flags:
blocking1.9.0.4 +
blocking1.8.1.18 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high])

Attachments

(4 attachments, 1 obsolete attachment)

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.
Flags: blocking1.9.0.4?
Flags: blocking1.8.1.18?
Group: core-security
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).
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.  ;)
(Assignee)

Comment 3

9 years ago
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?
Assignee: nobody → philringnalda
OS: Mac OS X → All
Hardware: PC → All
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...
And we prevent *.attributes.get, *.getAttribute, and *.getAttributeNS, but not getAttributeNode or getAttributeNodeNS, as long as we're tightening stuff up here.
(Assignee)

Comment 6

9 years ago
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.)
(Assignee)

Comment 7

9 years ago
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).
Attachment #342203 - Flags: superreview?(bzbarsky)
Attachment #342203 - Flags: review?(bzbarsky)
Attachment #342203 - Flags: superreview?(bzbarsky)
Attachment #342203 - Flags: superreview+
Attachment #342203 - Flags: review?(bzbarsky)
Attachment #342203 - Flags: review+
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.
(Assignee)

Comment 9

9 years ago
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.
Attachment #342203 - Flags: approval1.9.0.4?
Attachment #342203 - Flags: approval1.8.1.18?
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.4?
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.18?
Flags: blocking1.8.1.18+
Whiteboard: [sg:high]
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.
Attachment #342203 - Flags: approval1.9.0.4?
Attachment #342203 - Flags: approval1.9.0.4-
Attachment #342203 - Flags: approval1.8.1.18?
Attachment #342203 - Flags: approval1.8.1.18-
(Assignee)

Comment 11

9 years ago
Created attachment 342539 [details] [diff] [review]
For checkin, 1.8 branch
Attachment #342203 - Attachment is obsolete: true
Attachment #342539 - Flags: approval1.8.1.18?
(Assignee)

Comment 12

9 years ago
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).
Attachment #342540 - Flags: approval1.9.0.4?
(Assignee)

Updated

9 years ago
Attachment #342540 - Attachment is patch: true
Attachment #342540 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 13

9 years ago
Created attachment 342542 [details] [diff] [review]
For checkin, mozilla-central trunk
(Assignee)

Updated

9 years ago
Summary: Make Document.documentURI noAccess in mailnews → Make Document.documentURI and .textContent noAccess in mailnews
Comment on attachment 342539 [details] [diff] [review]
For checkin, 1.8 branch

Approved for 1.8.1.18, a=dveditz for release-drivers
Attachment #342539 - Flags: approval1.8.1.18? → approval1.8.1.18+
Comment on attachment 342540 [details] [diff] [review]
For checkin, 1.9.0 branch

Approved for 1.9.0.4, a=dveditz for release-drivers
Attachment #342540 - Flags: approval1.9.0.4? → approval1.9.0.4+
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.8.1.18, fixed1.9.0.4
Resolution: --- → FIXED
Any suggestions on how QA can verify this fix?
(Assignee)

Comment 17

9 years ago
Comment 6 - feel free to email me if you need more explicit directions.
Ah, right, I was skimming too quickly and didn't process your comment (obviously). This should be fine for verification.
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.
Keywords: fixed1.8.1.18 → verified1.8.1.18
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.4 → verified1.9.0.4

Comment 21

9 years ago
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)
Attachment #342539 - Flags: approval1.8.0.15+
Group: core-security

Updated

8 years ago
Flags: blocking1.8.0.next+
You need to log in before you can comment on or make changes to this bug.