83131 - JS security errors painfully uninformative

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect, P1)

Description

[John Bandhauer]

For instance in bug 83107 we see:

  Error: uncaught exception: Permission denied to create wrapper for object

The only way I could get a clue about the real issue here was to run under the 
debugger, set a breakpoint where the the security error happens (lucky me, I 
know that), and then inspect the objects associated with the code on the stack.

I'm thinking that most DOM programmers are not going to be able to do this when 
they run into a problem.

I can think of three strategies to improve this...

a) Have the caps code format the info available to it when if builds its 
exception - at least the caller location.

b) Have xpconnect add extra info to the exception. Currently, the caps code sets 
a JSString as an expection. xpconnect could get that string exception and format 
a new exception that incorporates that string and adds the information about the 
call attempted. xpconnect would need to do something else if the pending 
exception turned out to not be a string.

c) We could give up on the idea that the caps code will give informative 
exception output and just change the interface contract such that the caps code 
is not even responsible for setting a JS expection and instead just returns some 
result code to the caller (xpconnect usually) that identifies the type of error 
and lets the caller set the exception of its choice. This will require changing 
all uses of nsIXPCSecurityManager.

We *do* have the potential security issue of exposing *too* much info. Maybe 
this does not matter. But it should be considered.

I say 'we' above, but I mean the plural 'you' :)

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Very colorful bug summary...

This is something I'd been meaning to do but ran out of time for 0.9.1. I think
we can do (a), which is what I was planning to do anyway, unless you think one
of the other options is simpler or makes for better organization. In the case of
canAccess and canCreateWrapper, I have a classInfo to get the class name from.
For the others I have only a CID. How do I look up a class name from a CID?
Performance isn't as much of an issue in this case, since this is an error
condition, so it's OK if the lookup is clunky.

Do we really need to be checking on canCreateWrapper? We discussed this before,
but I forget the conclusion. What can a script do with a wrapper on which they
can't call any properties or methods? 

Comment 2

[info]

*** Bug 87059 has been marked as a duplicate of this bug. ***

Comment 3

[Mitchell Stoltz (not reading bugmail)]

*** Bug 70162 has been marked as a duplicate of this bug. ***

Comment 4

[ckritzer (gone)]

Markintg INVALID

Comment 5

[ckritzer (gone)]

Okay, I realllllllly need to drink my coffee *before* looking at bugs.  As Jesse 
has very politely pointed out to me, *this* is the real bug and 70162 is the 
dup.


Ahem.

Comment 6

[Mitchell Stoltz (not reading bugmail)]

*** Bug 91282 has been marked as a duplicate of this bug. ***

Comment 7

[Mitchell Stoltz (not reading bugmail)]

Target is now 0.9.4, Priority P1.

Comment 8

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - more descriptive error messages and refactor the call to SetExceptionWasThrown

Comment 9

[Johnny Stenback (:jst)]

- No need for the PRUnichar* cast at:

+  errorMsg.AppendWithConversion((PRUnichar*)JSValIDToString(aJSContext, aName));

- No need for rv2 here:

+            nsresult rv2;
+            nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID(),
&rv2);
+            if (NS_SUCCEEDED(rv2))

simply:

+            nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID());
+            if (xpc)

would do the same thing (since you don't return the error code anyway).

The code:

+        nsCAutoString errorMsg("Permission denied to create wrapper for object ");
...
+        if (aClassInfo)
+        {
...
+                errorMsg += "of class ";
+                errorMsg += className;
+            }
+        }

could maybe be changed to include "of class " in the initial value of errorMsg
here in stead of appending it separately?

This will leak the string from ToString() (in a few places):

+        errorMsg += aCID.ToString();

Other than that, sr=jst

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch with jst's changes (there are some unrelated changes in here too)

Comment 11

[Johnny Stenback (:jst)]

A few nits...

- I still see the extra PRUnichar* cast line ~200

- Be consistent with whitespace-after-if:

+            if (aClassInfo)
+                aClassInfo->GetClassDescription(getter_Copies(className));
+            if(className)
+                errorMsg += className;

- 3-space indentation at:
@@ -661,6 +712,9 @@
 nsresult
 nsScriptSecurityManager::GetBaseURIScheme(nsIURI* aURI, char** aScheme)
 {
+    if (!aURI)
+       return NS_ERROR_FAILURE;

sr=jst

Comment 12

[Mitchell Stoltz (not reading bugmail)]

Fix checked in.

Comment 13

[Henrik Gemal]

any reason why when I try to access "history.length" we write:
"Error: uncaught exception: Permission denied to get property History.length"

"history" -> "History"?

Comment 14

[Johnny Stenback (:jst)]

"history" == object name

"History" == class of object

Comment 15

[bsharma]

Verified on 2001-09-19-03 build on WinNT

Ran the following code on the browser window:
javascript:alert(x.document.images[0]);

And got the following suitable error:
Error: uncaught exception: Permission denied to get property HTMLDocument.images