291377 - [FIX]JavaScript exception when accessing e.lineNumber on an exception from accessing window.document.domConfig

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[hhalkin]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-us) AppleWebKit/312.1 (KHTML, like Gecko) Safari/312
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

the javascript code:

function jack(mm) {
	try { 
		eval(mm);
	}
	catch(e) {
		if(e.lineNumber != null) alert("e.lineNumber is " + e.lineNumber);
	}
}
jack("window.document.domConfig")

Reproducible: Always

Steps to Reproduce:
1.Run the javascipt

function jack(mm) {
	try { 
		eval(mm);
	}
	catch(e) {
		if(e.lineNumber != null) alert("e.lineNumber is " + e.lineNumber);
	}
}
jack("window.document.domConfig")
Actual Results:  
firefox crashes and reloads.

Expected Results:  
produce an alert with the message:

e.lineNumber is xxx

where xxx is the number of the line containing
eval(mm);

With this bug it is impossible to create a general "object_inspector" for firefox.

Related (but fixed) bug : 226193

Comment 1

[Boris Zbarsky [:bzbarsky]]

Please attach a testcase showing the problem (using
https://bugzilla.mozilla.org/attachment.cgi?bugid=291377&action=enter )

Comment 2

[hhalkin]

Attachment: html with some javascript

Load the page in Firefox and click on the link.
Repeat in Internet Explorer and Safari.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Confirming.  Basically, domConfig throws an NS_ERROR_NOT_IMPLEMENTED and trying
to get the lineNumber of the resulting exception object throws:

  Error: uncaught exception: Permission denied to get property
    UnnamedClass.lineNumber

That seems wrong.

Comment 4

[alexander :surkov (:asurkov)]

Looks like dupe of https://bugzilla.mozilla.org/show_bug.cgi?id=226193

Comment 5

[Boris Zbarsky [:bzbarsky]]

It's not.  This is a bug about an exception when accessing the lineNumber
property of an Exception object.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So the problem here is that NS_ERROR_NOT_IMPLEMENTED is not a DOM module exception.  So the exception object we return is an nsXPCException, not an nsDOMException.

When we go to do a security check on the getter we get an UNDEFINED access, which means DENIED for an XPConnect access to a non-DOM object (otherwise it means SAME_ORIGIN).  Now an nsXPCException is not a DOM object per its classinfo (unlike a nsDOMException).  So the access is DENIED.  This property ("lineNumber") is not whitelisted in nsXPCException::CanGetProperty, so we deny access and get this bug.

The thing is that we can't do SAME_ORIGIN once we get to the security checked component point...

So ideally, an nsXPCException would be a DOM object per its classinfo.  That would make it behave just like a nsDOMException security-wise (with the addition of some properties that are allAccess due to its nsISecurityCheckedComponent impl).  Failing that, perhaps we should switch to a DOM error code here so that we get DOM exceptions?  Or perhaps we should do something else?

Comment 7

[Boris Zbarsky [:bzbarsky]]

sicking, any thoughts on this one?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I talked with Jst and the right solution seems to be to give nsXPCException a proper classinfo that makes it possible to access all properties.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: So something like this?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly expanded testcase

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm not quite sure how to test this... I could easily write a mochitest for it, but said mochitest would depend on some property being not implemented (document.domConfig in this case)...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Test for this

I didn't want to rely on the NOT_IMPLEMENTED thing, and discovered that actual XPConnect exceptions (NOT_ENOUGH_ARGS here) lose their line number as of Blake's fix for bug 321831.  We might want this patch on the 1.8 branch, though there trying to get the lineNumber from untrusted code throws anyway, so...

Comment 13

[Johnny Stenback (:jst)]

I wonder if there was a real reason for only allowing access to a small set of the properties on XPConnect exceptions in the first place. I.e. if we take this change are we risking leaking data to untrusted callers that we previously thought was bad to expose to untrusted code?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Possibly.  The properties that used to not be exposed are filename, lineNumber, columnNumber, location, inner, and data.

Our DOM exceptions expose these already (and I thought we had someone complaining about it?), so this is not really adding anything much new.  Assuming we have the right object principal on our exceptions, it should only be possible to get this stuff same-origin anyway, no? 

In any case, the original checkin comment for this code is:

1.15 <[e-mail]> 2000-12-07 00:14
fix bug 62069 and bug 68538 by using nsISecurityCheckedComponent to allow unfettered access from JavaScript to Components.interfaces and the safer methods on nsXPCException. r=brendan r=mstoltz sr=hyatt

Before that you couldn't get anything at all off an nsXPCException in untrusted code.

Comment 15

[Johnny Stenback (:jst)]

Same origin doesn't really apply here, as exception wrappers don't have a helper with a PreCreate hook that can ensure creation in the right scope, so if an exception is thrown in a chrome scope and somehow makes it out to an untrusted scope (through C++), it'll get re-wrapped and its object principals will become that of the scope in which it's being wrapped in (i.e. unsafe code).

The potential problem here seems to be that we could leak out the location of a chrome files where an exception was thrown, and that way leak out what extensions are installed or what not (or profile directory location?). Not sure how much we care but I'd like to understand why there was a need to only expose some of the information in XPConnect exceptions in the first place.

Jband, any of this still stuck in your memory? :)

Comment 16

[John Bandhauer]

IIRC, the original assumption was that anyone wanting to do anything interesting with an nsXPCException would already have XPConnect access privileges. That assumption broke down as people seemed to find situations where they claimed to need to get at particular exception methods and properties. So we allowed some of that. I can't say why lineNumber got left out - I think we just focused on exposing only what people claimed to need. But, realistically, once toString was exposed we were no longer really hiding anything anyway.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed.