151933 - xml:base should not allow setting chrome URLs

Status: VERIFIED FIXED

(Core :: XML, defect, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

It seems HTML base tag will not let you set chrome URLs, but xml:base does.
Quick test did not show this to cause security issues (for example link to
chrome did still not work, nor did a script on a page get chrome privileges),
but this should be fixed nevertheless in case there are other issues that I have
not tested.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

keyword foo. Nominating, priority 2.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix

This adds a security check at the end of GetXMLBaseURI(), similar to
nsDocument::SetBaseURL(nsIURI* aURL). 

Because XML Base is calculated on demand and not cached, the side effect is
that we'll get security warnings every time someone tries to acces baseURI
property with illegal value. There is an old bug open that would fix XML Base
so that it's value is cached correctly (it is not trivial, DOM changes can
change XML Base value).

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 88582 [details] [diff] [review]
Proposed fix

Don't we want to pass back the parent element's base URI and not the document's
URI if the security manager denies access to the current element's base URI?

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 88582 [details] [diff] [review]
Proposed fix

Yeah, I thought of that at home, I'll make a new patch.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix 2

Now, if security check fails, we start from the parent of the last content
element we tried, and recursively call GetXMLBaseURI(). If there was no parent,
just get the document base.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 88676 [details] [diff] [review]
Fix 2

sr=jst

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix 3

Oops, need the _base_ URI if there is one, and only if that is null get the
document URI as baseURI.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 88712 [details] [diff] [review]
Fix 3

sr=jst

Comment 9

[harishd]

Comment on attachment 88712 [details] [diff] [review]
Fix 3

r=harishd

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed on trunk.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Sent email for ADT & drivers approval.

Comment 12

[Paul Wyskoczka]

Heikki, can you make the bugs Resolved/fixed

Chris, can you verify the fix on the trunk.  Thx.

Comment 13

[Jaime Rodriguez, Jr.]

resolving as fixed per Comment #10 From Heikki Toivonen.

Comment 14

[Judson Valeski]

can someone verify this?

Comment 15

[Chris Petersen]

Attachment: Simple test case

Comment 16

[Chris Petersen]

Verified in the 2002-06-26-08 Win ME and OS X trunk builds.

Comment 17

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 18

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) approval for checkin on the 1.0 branch. please
checkin asap, then replace the "mozilla1.0.1+" keyword and with the "fixed1.0.1".

Comment 19

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed on the branch too.

Comment 20

[Chris Petersen]

Verified with Windows June 27th (2002-06-27-08 branch) and OS X July 1
(2002-07-01-08 branch).