Same-origin check in nsXMLDocument::OnChannelRedirect() can be circumvented

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: jst)

Tracking

({verified1.8.1.17})

1.8 Branch
x86
Windows XP
verified1.8.1.17
Points:
---
Bug Flags:
blocking1.8.1.17 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high xss])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
This is fx2-only.  (On trunk, the same-origin check in question compares an old
URI to a new URI.)

The same-origin check in nsXMLDocument::OnChannelRedirect() uses a principal of
an associated JS context.  It can be circumvented by loading a cross-origin
page in that context before nsXMLDocument::OnChannelRedirect() is called.

By using this trick, it's possible to change a principal of an XML document,
and thus, an attacker can perform an XSS attack.

Upcoming testcase consists of an html and a cgi script, thus it does not work
on bugzilla.mozilla.org.  Please set up it in a suitable place.
Flags: blocking1.8.1.16?
Whiteboard: [sg:high xss]
qawanted: is this fixed by the fix for bug 439035?
Keywords: qawanted
-> Johnny if we do need additional fixes
Assignee: nobody → jst
Flags: wanted1.8.1.x+
(In reply to comment #3)
> qawanted: is this fixed by the fix for bug 439035?
> 

It is not. I checked this with 2.0.0.16 when I verified bug 439035 and it still reproduces even though that one is fixed.
Keywords: qawanted
Flags: blocking1.8.1.17? → blocking1.8.1.17+
Whiteboard: [sg:high xss] → [sg:high xss][needs 1.8 patch]
(Assignee)

Comment 6

9 years ago
Created attachment 333863 [details] [diff] [review]
1.8 version of the trunk fix we took for this.

This fixes this bug by making the same change on the 1.8 branch that we took on the trunk some time back. The scary part here, if any, is the fact that we're removing the ability to do cross site loads with document.load() if the caller has elevated privileges. If need be we could leave that change out, but we haven't heard anything bad about that in Firefox 3, so I wouldn't expect any problems from it.
Attachment #333863 - Flags: superreview?(jonas)
Attachment #333863 - Flags: review?(jonas)
Comment on attachment 333863 [details] [diff] [review]
1.8 version of the trunk fix we took for this.

Could shorten to

return nsContentUtils::GetSecurityManager...
Attachment #333863 - Flags: superreview?(jonas)
Attachment #333863 - Flags: superreview+
Attachment #333863 - Flags: review?(jonas)
Attachment #333863 - Flags: review+
Whiteboard: [sg:high xss][needs 1.8 patch] → [sg:high xss]
(Assignee)

Updated

9 years ago
Attachment #333863 - Flags: approval1.8.1.17?
(Assignee)

Comment 8

9 years ago
Created attachment 334011 [details] [diff] [review]
Updated diff.

This fixes the silly return code, and fixes the refcounting problem pointed out to me by dveditz (which was a result of me copying this code from an XPCOMGC tree, duh). Also found some dead code I took out too. This one's good to go.
Attachment #334011 - Flags: superreview+
Attachment #334011 - Flags: review+
Attachment #334011 - Flags: approval1.8.1.17?
(Assignee)

Comment 9

9 years ago
Created attachment 334013 [details] [diff] [review]
Trunk version of the cleanup in the above patch.
Attachment #333863 - Attachment is obsolete: true
Attachment #334013 - Flags: superreview?(jonas)
Attachment #334013 - Flags: review?(jonas)
Attachment #333863 - Flags: approval1.8.1.17?
Comment on attachment 334011 [details] [diff] [review]
Updated diff.

Approved for 1.8.1.17, a=dveditz for release-drivers.
Attachment #334011 - Flags: approval1.8.1.17? → approval1.8.1.17+
(Assignee)

Comment 11

9 years ago
Fix landed on the branch, leaving bug open to track the trunk cleanup.
Status: NEW → ASSIGNED
Keywords: fixed1.8.1.17
Attachment #334013 - Flags: superreview?(jonas)
Attachment #334013 - Flags: superreview+
Attachment #334013 - Flags: review?(jonas)
Attachment #334013 - Flags: review+
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17) Gecko/2008082909 Firefox/2.0.0.17.
Keywords: fixed1.8.1.17 → verified1.8.1.17
Shouldn't a developer resolve this so I can mark it as verified that way?
See comment 11.

Jst: Are you still planning on doing that cleanup?
(Assignee)

Comment 15

9 years ago
Duh, that cleanup landed two weeks ago, forgot to close the bug...

http://hg.mozilla.org/mozilla-central/rev/61d40733056c
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 16

9 years ago
Comment on attachment 334011 [details] [diff] [review]
Updated diff.

a=asac for 1.8.0.15
Attachment #334011 - Flags: approval1.8.0.15+
Group: core-security

Updated

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