79796 - Post-XPCDOM-landing fixes for ScriptSecurityManager

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

The attached patch contains Brendan's "cloned native" fix and some other fixes.
Jband already gave his r=, Brendan, can I get your OK?

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch with various fixes for nsScriptSecurityManager

Comment 2

[Brendan Eich [:brendan]]

Nice indentation fix here:

         if (NS_FAILED(GetPrefName(principal, aClassName, aPropertyName,
-                      classPolicy, prefName)))
+                                  classPolicy, prefName)))

But why not do the same here:

+            if (NS_FAILED(GetPrefName(principal, aClassName, aPropertyName,
+                          nsnull, prefName)))
+                return SCRIPT_SECURITY_NO_ACCESS;

(i.e., indent the underhanging arguments to start underneath 'principal, ...')?

Ok, now I get to pick on myself: don't JS_GetFunctionScript twice, do move the
script local and its initializer up.  Instead of


+    if (JS_GetFunctionScript(cx, fun) && JS_GetFunctionObject(fun) != obj) {
+        // Scripted function has been cloned; get principals from obj's
parent-linked scope chain.
+        // We do not get object principals for a cloned *native* function,
because the subject in
+        // that case is a script or function further down the stack who is
calling us.
         return GetObjectPrincipal(cx, obj, result);
+    }
     JSScript *script = JS_GetFunctionScript(cx, fun);

Do this:

    JSScript *script = JS_GetFunctionScript(cx, fun);
    if (script && JS_GetFunctionObject(fun) != obj) {
      // Scripted function has been cloned; get principals from obj's
      // parent-linked scope chain.  We do not get object principals for a
      // cloned *native* function, because the subject in that case is a
      // script or function further down the stack who is calling us.
      return GetObjectPrincipal(cx, obj, result);
    }

I also fixed the overindented, overlong lines in the then part.

I did not optimize further by testing if (!script) and bailing with *result set
to nsnull, preferring to let the code flow into return GetScriptPrincipal(...).

Is nsIPrefBranchInternal an "internal" interface, or is it just confusingly
named.  Cc'ing alecf.  I can rs= that if it works, just want an answer recorded
here, maybe a followup bug if there is one.

Fix these nits and I'll sr.

/be

Comment 3

[Mitchell Stoltz (not reading bugmail)]

I was told that someone didn't want nsIPrefBranch::AddObserver to be part of the
embedding API, which is why it was moved to nsIPrefBranchInternal. That's the
only way I could get at that function. I corrected the things you mentioned
above; do you think I should, as you mention, return a null result from
GetFunctionObjectPrincipal if (!script) ?

Comment 4

[Alec Flett]

mitch is correct, Internal means "Not for embedders" - as approved (maybe even
suggested) by valeski... but is just fine for use by developers

Comment 5

[Brendan Eich [:brendan]]

mstoltz: I don't mind letting a null script be handled by GetScriptPrincipal,
but I wouldn't have written things that way.  I usually make GetFoopyWhatever
that takes a Foopy* require that the caller not pass null.  Entirely your call,
it's your code.  How about a new attachment?

/be

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Checked in.

Comment 7

[ckritzer (gone)]

Mitch, anyway to test this?

Comment 8

[Mitchell Stoltz (not reading bugmail)]

Not really, apart from making sure DOM same-origin works as expected.

Comment 9

[ckritzer (gone)]

Okay, this is weird...
I ran all my tests earlier this week on same origin (as well as last week) and 
the ran fine.  Just to make double-secret-probation sure that they still work I 
rant them nn the 2001-05-23-xx-trunk bits, and now they all throw an "uncaught 
exception: Permission denied to access property" in the JS console (which my 
testcases are checking for)...

This looks like it's related to bug#78783.

Comment 10

[ckritzer (gone)]

Additionally, these tests were working on yesterday's bits, so something that 
went in last night probably is the culprit.

Comment 11

[ckritzer (gone)]

Okay, I think this might be a prefs.js problem on my machines.  I'm going to 
clear out my prefs and install clean versions.

Comment 12

[ckritzer (gone)]

Yep, user head-gap.  Ignore my previous three posts...

Comment 13

[ckritzer (gone)]

Marking VERIFIED FIXED on:
-MacOS91 2001-05-30-09-trunk
-Win98SE 2001-05-30-09-trunk
-LinRH62 2001-05-30-08-trunk