Closed Bug 119646 Opened 23 years ago Closed 15 years ago

Speedups of nsScriptSecurityManager::CanAccess

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bratell, Assigned: security-bugs)

References

Details

(Keywords: perf)

Attachments

(1 file, 7 obsolete files)

I couldn't find a bug for this but unless there already is one this should track 
what jst said in bug 118933. It's about nsScriptSecurityManager::CanAccess 
taking more than half the time of simple DOM calls being a big reason MSIE seems 
to be 3-4 times as fast as Mozilla. 

From bug 118933:

------- Additional Comment #3 From Johnny Stenback 2002-01-12 02:31 -------

mstoltz has some *really* nice goodies in his pocket that should make a world of
difference here. Don't know if there's a bug for those speed improvements 
though.
Blocks: 118933
Keywords: perf
OS: Windows 2000 → All
Hardware: PC → All
I hadn't filed a bug on it yet - thanks for doing so. The patch is nearly
complete - I'll post what I've got so far, for the heck of it.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Just for us curious who doesn't know very much, how is the new code faster? More
caching?
Attached patch Patch take 2 (obsolete) — Splinter Review
Things still to do:
  - Clean up string usage in a few places
  - Re-implement wildcard support (maybe)
  - Maybe switch ClassPolicy over to a PLDHashTable
  - Lots of Testing!
Attachment #65062 - Attachment is obsolete: true
I did some initial testing with Quantify with this patch, my numbers are a bit
iffy since my pre-patch numbers are from viewer running a simple DOM perf test
(the getElementById() test from bug 118933), and my post-patch tests are from
running the same test in mozilla (since the patched build causes viewer to crash
on startup). The numbers show that the average time spent in
nsScriptSecurityManager::CanAccess() dropped from 33us to 2.5us per call, that's
a 10+ time speedup of that function! Woohoo, totally awesome, great, neat, sweet
:-) Again, I don't know if those numbers mean much, but it sure looks promising ;-)
This patch includes:
  - Changed DomainPolicies to PLDHashTables. We may want to reconsider this;
it's less space-efficient.
  - Re-implemented wildcard policies.
  - Some other improvements and bug fixes
Attachment #65129 - Attachment is obsolete: true
Attached patch Patch 4 (obsolete) — Splinter Review
Some more improvements
   - fixed crash in viewer
   - using unions for security levels instead of casting
Attachment #65715 - Attachment is obsolete: true
Will this land after the branch?
Comment on attachment 66231 [details] [diff] [review]
Patch 4

I'm more than a little afraid of that code. Sharing a char* and a PRUint32
without knowing what you have there seems awfully dangerous to me. 

A pointer could be 64 bit where the first 32 bit shares memory with the
integer. If the high 32 bit of the pointer is 0,1, 3 or 5 you will think that
the union represents the constants and I don't know what that could cause
(security holes? crashes?).

If you have to share the memory and don't want to use a tag of some kind, maybe
you can use the low bit in the pointer in some way. I think we can safely
assume that the low bit (maybe even the low 3-4 bits) in heap allocated memory
is 0. I haven't thought very much about it, but the way you do now will cause
trouble.
JS tags low bits in pointers, but it takes care to align memory on 0 mod 2^n
boundaries for n tag bits.  IIRC, malloc must return a pointer that satisfies
double's alignment requirements, but there is no standard for double alignment
-- architectures vary.  It's probably safe to assume 0 mod 4 for double, but if
you can control the alignment, by overallocating and rounding up (see for
example http://lxr.mozilla.org/mozilla/source/js/src/jsarena.h#104 and nearby).

/be
To avoid confusion and possible alignment errors in the union, we should make
the |PRInt32| a |long| in stead which would guarantee that the low bit of the
|long| lines up with the low bit of the char*.
Well, no. I haven't seen any guarantee anywhere that long is the same size as a
pointer. We even have the obscure long long type that is 64 bits on platforms
where long continues to be 32 bits.

Since this is unsafe anyway, why not skip the union and add a macro/inline
method that accesses the data and does the cast?

An inline method would be good because then we could easily add assertions to
check the low bit but I don't trust platforms to inline so a macro is probably
best after all.
The world would fall appart if sizeof(long) != sizeof(void*), this is a safe
assumption and it's made all over the mozilla code.
I asked around and got Win32/IA64 as an example. There long is 32 bits och void
* 64 bits. I hope we doesn't depend on your assumption too much because Mozilla
will probably have to run on that platform very soon if it doesn't already.

To get an integer that's the size of a pointer, use the types ptrdiff_t and size_t.
Yeah, looks like you're right, Microsoft has done it again, why do what everyone
else does when they can ignore history and screw things up for everyone in
stead? I know we rely on sizeof(long) == sizeof(void*) in numerous places in
Mozilla, someone will need to find all those once we start building Mozilla on
Win32/IA64.

So I guess there's no 128-bit long long type on Win32/IA64 then like there is on
all other 64-bit architectures (AFAIK), how messed up is that?

So, we'll use ptrdiff_t here then, or just use long and deal with this when we
start porting to Win32/IA64...
Johnny, related to your last comment, should we open a bug on "don't rely on
rely on sizeof(long) == sizeof(void*)" because of Win32/IA64 where long is 32
bits and void 64 bits? Or should we just wait for the problem to appear?



I'd wait for the problems to show up, it'll be obvious when they do...
Actually, on Win16, size_t and ptrdiff_t were not the same size as a pointer. 
That may have been a standard C violation.

We should not, in any event, assume that sizeof(long) == sizeof(void *).  The
deprecated (but never to be removed!) types PRWord and PRUword are the types you
want to use.  Let's not wait for problems we can foresee, let's use these types
that are existing solutions now.

/be
Argh.  My memory was either of NSPR 1, or was just wrong.  I thought PRWord was
#ifdef'd to be an integral typedef whose size was sizeof(void*), but Pav pointed
out that it's always just a long.  This seems like a problem, even if it is a
deprecated type (JS uses its own jsword, currently also a long).  Oh well, we
can cross the sizeof(long) < sizeof(void*) bridge when we come to it.  An
assertion that sizeof(long) == sizeof(void*) would be good, in the mean time.

/be
Or just go back to the casts. That way this code will work forever without any
assumptions about specific compiler implementations.
+struct PropertyPolicy : public PLDHashEntryHdr
+{
+    jsval          key;  // property name as jsval
+    SecurityLevel  get;
+    SecurityLevel  set;
+};
+
+// Class Policy
+#define NO_POLICY_FOR_CLASS (ClassPolicy*)1
+
+struct ClassPolicy : public PLDHashEntryHdr
+{
+    char*  key;
+    PLDHashTable mPolicy;
+    ClassPolicy* mDefault;
+    ClassPolicy* mWildcard;
+};

Inconsistent naming convention of struct members, I'd sugest consistently using
the 'm' prefix in member names.

- The pldhash stub op PL_DHashGetKeyStub() is identical to GetClassPolicyKey(),
so no need for that method here.

- In ClearClassPolicyEntry():

+    PL_DHashTableDestroy(&cp->mPolicy);

This will crash if ever executed, this will cause &cp->mPolicy that was never
directly allocated to be freed, you want PL_DHashTableFinish(&cp->mPolicy) since
you don't own the memory where the pldhash lives.

- domainPolicyOps should be static and should be moved from the global scope
into the DomainPolicy() ctor which is the only place where it's used.

- DomainPolicy should be a class, not a struct, and mRefCount should be private
to make it clearer who's supposed to access it...

- Seems like:

+DeleteDomainEntry(nsHashKey *aKey, void *aData, void* closure)
+{
+    DomainEntry *entry = (DomainEntry*) aData;
+    do
+    {
+        DomainEntry *next = entry->mNext;
+        entry->mDomainPolicy->Drop();
+        delete entry;
+        entry = next;
+    } while (entry);

would be a bit cleaner if the DomainEntry class had a destructor that took care
if releasing its mDomainPolicy when the entry is deleted. The DomainEntry
constructor should also "hold" the reference to mDomainPolicy.

- All internd JSString's should be held in class static's so that they can be
cleard on shutdown, and not as local static's. Follow the code in nsDOMClassInfo
for how to do this.

I stopped reviewing at nsScriptSecurityManager::CheckPropertyAccessImpl(), I'll
continue from there later.
Mitchell, the Observe() method seems to no longer handler messages for a single
capability.policy.something pref changing... Is this not still needed?  Or am I
missing something?
Boris,
  Correct, that's no longer needed. Under the new scheme, policy prefs must be
parsed all at once, not incrementally. I need to expose a hook that can be
called from the UI when the options in prefs:Scripts and Windows are changed.

jst,
   I'll make those changes.
- All calls to InitPolicies() should check for returned error codes and take
appropriate action.

- I don't see any code that checks if mustFreeClassName is true after this call
and frees the allocated className after this code:

+    if (cpolicy->key[0] == '*' && cpolicy->key[1] == '\0' &&
+        securityLevel.level == SCRIPT_SECURITY_UNDEFINED_ACCESS)
+    {
+        GetClassName(aClassInfo, &className, &mustFreeClassName);

- nsScriptSecurityManager::GetClassName() could cause leaks, it doesn't always
set *mustFree. Also, it doesn't look like the method needs to be part of the
class at all, or if it needs to be, then you should make it static (no need to
pass |this| around. I'd suggest making it a inline file static function, and
write it like this:

static inline void
GetClassName(nsIClassInfo* aClassInfo, char** aClassName, PRBool* mustFree)
{
    if(*aClassName)
    {
         *mustFree = PR_FALSE;
         return;
    }

    if (aClassInfo)
        aClassInfo->GetClassDescription(aClassName);

        if (*aClassName) {
            *mustFree = PR_TRUE;
        } else {
            *aClassName = "UnnamedClass";
            *mustFree = PR_FALSE;
        }
    }
}

- In nsScriptSecurityManager::GetClassPolicy():

+        //-- and the wildcard policy (class "*" for this domain)
+        static nsCStringKey wildcardKey("*");
...

wildcardKey is unused, remove it.

- In nsScriptSecurityManager::Observe(), no need to allocate |message|.
Something like this would do the right thing w/o always allocating, and this
would be unicode safe too:

  NS_ConvertUCS2toUTF8 utf8str(aMessage);
  const char *message = utf8str.get();

- In nsScriptSecurityManager::InitPolicies():

+        while(*policyCurrent != '\0' && (*policyCurrent == ' ' ||
*policyCurrent == ','))
+            policyCurrent++;

The *policyCurrent != '\0' check in the loop is redundant.

Also, inconsistent handling of OOM errors at:

+                DomainEntry *newEntry = new DomainEntry(domainStart, domainPolicy);
+                if (!newEntry)
                     break;

Why not return NS_ERROR_OUT_OF_MEMORY here like the other similar cases in this
same method do?

And right after that:

+                // Update reference count
+                newEntry->mDomainPolicy->Hold();

Seems like this would be cleanre and less error-prone if the
constructor/destructor of DomainEntry would deal with reference counting
mDomainPolicy.

- In nsScriptSecurityManager::InitDomainPolicies():

+        ClassPolicy* cpolicy = 
+          NS_REINTERPRET_CAST(ClassPolicy*,
+                              PL_DHashTableOperate(aDomainPolicy,
+                                                   start,
+                                                   PL_DHASH_ADD));
+
+        if (!cpolicy->key) // New class policy
+        {
+            cpolicy->key = PL_strndup(start, end-start);
+            PL_DHashTableInit(&cpolicy->mPolicy, PL_DHashGetStubOps(), nsnull,
+                sizeof(PropertyPolicy), 16);
+        }

The PL_DHASH_ADD operation can return null in low memory situations so you
should check for that and return NS_ERROR_OUT_OF_MEMORY. Also, the
initialization of the new entry should be done with a pldhash InitEntry() op,
that way the init code will be isolated into one place even if entries were
added in more that one place in the code.

Same thing for PropertyPolicy, add an InitEntry() op.

Further down:

            if (*start == 's' || *start == 'S')
                ppolicy->set = secLevel;
            else
                ppolicy->get = secLevel;

Is checking for 's' or 'S' enough, don't we want to be more strict here and
check for "set" and "get" (case sensitively)?

All JS strings that are internd should be cleaned up on shutdown, check how
nsDOMClassInfo does that and do something similar.

Fix the above and I'll have one more look before stamping this.
Um, GetClassName() should initialize the out params even if there's no aClassInfo.
Attached patch Patch take 5 (obsolete) — Splinter Review
This patch incorporates jst's comments, plus the ResetSecurityPolicy hook to be
called when prefs change (it's called from pref-scripts.js, included in this
patch).
Attachment #66231 - Attachment is obsolete: true
A few minor issues that I saw when looking through the new patch:

+// XXX What type to use for these?
+#define SCRIPT_SECURITY_UNDEFINED_ACCESS      0
+#define SCRIPT_SECURITY_NO_ACCESS             1
+#define SCRIPT_SECURITY_SAME_ORIGIN_ACCESS    3
+#define SCRIPT_SECURITY_ALL_ACCESS            5

Remove the XXX comment. And for the values of these #defines I'd suggest
something like this

#define SCRIPT_SECURITY_UNDEFINED_ACCESS 0
#define SCRIPT_SECURITY_ACCESS_IS_SET_BIT 1
#define SCRIPT_SECURITY_NO_ACCESS \
  (0 << 1 | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)
#define SCRIPT_SECURITY_SAME_ORIGIN_ACCESS \
  (1 << 1 | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)
#define SCRIPT_SECURITY_ALL_ACCESS \
  (2 << 1 | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)

And document this in the header, especially mention the fact that these numbers
all have the low bit set (except the UNDEFINED one) and that there can never be
a pointer to allocated memory that would conflict with these numbers.

- In nsScriptSecurityManager::~nsScriptSecurityManager():

+    sCallerID = JSVAL_VOID;
+    sEnabledID = JSVAL_VOID;

These static jsval's are initialized in the constructor of
nsScriptSecurityManager, and to do this right and to make it fool proof you
should initialize them only if the variables haven't already been initialized,
and you should only set them back to JSVAL_VOID on shutdown, not in the
destructor. Resetting these static strings in the destructor means that if
someone creates a security manager after the first one has been created and then
delets it, your strings in the still existing security manager will be set to
JSVAL_VOID, which is not what you want. Not that anyone should ever do this, but
it's doable, and it's easy enough to make that not cause problems so you should
fix it. You should create a static shutdown method that you call only from the
caps module's destructor code, like the DOM code does (for some things, look for
DOMModuleDestructor()).


Also a few minor optimizations:

- In nsScriptSecurityManager::GetCurrentJSContext():

+    nsresult rv;
+    if (!mJSContextStack) {
+        mJSContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1", &rv);
+    }
+    if (!mJSContextStack)
+        return nsnull;

Remove |rv| since you don't use it for anything other that to assign into it.
And move the second if (!mJSContextStack) check inside the first one to avoid a
double check for null in the normal path.

- In nsScriptSecurityManager::GetSafeJSContext() you should also loose |rv| for
the same reason. There are also similar optimization possibilities there, maybe
something like this:

    nsCOMPtr<nsIThreadJSContextStack> threadContextStack =
        do_QueryInterface(mJSContextStack);

    if (!threadContextStack) {
        if (!GetCurrentJSContext()) {
            NS_ASSERTION("...");
            return nsnull;
        }

        threadContextStack = do_QueryInterface(mJSContextStack);

        if (!threadContextStack) {
            NS_ASSERTION("...");
            return nsnull;
        }
    }

    ...

This would eliminate one null check in the normal path...

- Similar stuff in GetClassName():

+    if(*aClassName)
+        return PR_FALSE;
+
+    if (aClassInfo)
+        aClassInfo->GetClassDescription(aClassName);
+
+    if (*aClassName)
+        return PR_TRUE;
+    else
+    {
+        *aClassName = "UnnamedClass";
+        return PR_FALSE;
+    }

The if checks for *aClassName is only needed inside the if (aClassInfo) check.
Also loose the else-after-return, which will happen automatically if you move
the if (*aClassName) check inside the other if check...

- In DeleteCapability():

+    char* secLevel = NS_REINTERPRET_CAST(char*, aData); 
+    nsMemory::Free(secLevel);

No need for the secLevel variable nor the cast, nsMemory::Free() doesn't care
about types, so you can pass aData directly to nsMemoru::Free().

r=jst with the above issues addressed.
Attachment #68244 - Flags: review+
Attachment #68244 - Attachment is obsolete: true
jst suggested :
#define SCRIPT_SECURITY_UNDEFINED_ACCESS 0
#define SCRIPT_SECURITY_ACCESS_IS_SET_BIT 1
#define SCRIPT_SECURITY_NO_ACCESS \
  (0 << 1 | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)
#define SCRIPT_SECURITY_SAME_ORIGIN_ACCESS \
  (1 << 1 | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)
#define SCRIPT_SECURITY_ALL_ACCESS \
  (2 << 1 | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)

This yields the right numbers but is hardly informative. For one thing parens
would clarify for the precedence impaired what values these represent. For
another thing the pattern 'n << 1' breaks after 2; e.g. 3 << 1 == 6 not 8. So,
if you are going to use this sort of pattern then please go with:

#define SCRIPT_SECURITY_UNDEFINED_ACCESS 0
#define SCRIPT_SECURITY_ACCESS_IS_SET_BIT 1
#define SCRIPT_SECURITY_NO_ACCESS \
  ((1 << 0) | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)
#define SCRIPT_SECURITY_SAME_ORIGIN_ACCESS \
  ((1 << 1) | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)
#define SCRIPT_SECURITY_ALL_ACCESS \
  ((1 << 2) | SCRIPT_SECURITY_ACCESS_IS_SET_BIT)

+MatchClassPolicyKey(PLDHashTable *table,
+                    const PLDHashEntryHdr *entry,
+                    const void *key)
+{
+    ClassPolicy* cp = (ClassPolicy *)entry;
+    return (PL_strcmp(cp->key, (char*)key) == 0);
+}

*If* there is much chance that these two string *pointers* are identical, then
you can save some cycles with:

 return cp->key == (char*)key || (PL_strcmp(cp->key, (char*)key) == 0);

+PR_STATIC_CALLBACK(void)
+ClearClassPolicyEntry(PLDHashTable *table, PLDHashEntryHdr *entry)
+{
+    ClassPolicy* cp = (ClassPolicy *)entry;
+    if (cp->key)
+        PL_strfree(cp->key);
+    PL_DHashTableFinish(&cp->mPolicy);
+}

No "cp->key = nsnull;" ? You're sure no code tries to use cp->key after this point?

> nsScriptSecurityManager::GetSafeJSContext

You should have your mJSContextStack be of type nsIThreadJSContextStack and init
it to that type when you first create it. That interface inherits from
nsIJSContextStack, so you can use it for everything you already do with it. But
doing this will save the local QI you do everytime you need that interface in
nsScriptSecurityManager::GetSafeJSContext.

+static inline PRBool
+GetClassName(nsIClassInfo* aClassInfo, char** aClassName)
+{
+    // The return value indicates whether the class name was allocated here
+    // and must be freed later
+    if(*aClassName)
+        return PR_FALSE;
+
+    if (aClassInfo)
+    {
+        aClassInfo->GetClassDescription(aClassName);
+        if (*aClassName)
+            return PR_TRUE;
+    }
+
+    *aClassName = "UnnamedClass";
+    return PR_FALSE;
+}


If aClassInfo == nsnull then this crashes. You want the following, right?

+    if(*aClassName)
+        return PR_FALSE;
+
+    aClassInfo->GetClassDescription(aClassName);
+    if (*aClassName)
+        return PR_TRUE;
+
+    *aClassName = "UnnamedClass";
+    return PR_FALSE;
+}

Does that match the expectations of the callers?

+#ifdef DEBUG
+    nsCString         mPolicyName;
+#endif

<trivial> I'm a big fan of including the chars DEBUG into the name of debug only
members (i.e. m_DEBUG_PolicyName) to keep me from accidentally
 using them in non-debug code.

-NS_IMPL_THREADSAFE_ISUPPORTS3(nsScriptSecurityManager,
+NS_IMPL_ISUPPORTS3(nsScriptSecurityManager,

I was going to ask about the lack of threadsafety in the DomainEntry
refcounting. I take it you are generally removing semblence of support of
threadsafe calls into the script security manager. I'm certainly OK with that. I
*am* wondering if it might be worth adding DEBUG only explicit checks to one or
two of the main entry points to ensure that you are not called on another
thread. Though, I suppose this is moot since, for DOM objects, xpconnect will do
that debug check before calling you.

+    if (mustFreeClassName)
...
     {
+        PR_Free(className);
+        className = nsnull;

These should be nsMemory::Free considering where the string came from.

Since you have to check that flag and conditionally free in a few places I'd be
considering a simple C++ auto class that includes a method to init the string
using the logic in GetClassName, tracks the needsDelete flag internally, and
conditionally frees the string for you in its dtor.

+    jsval propertyName = STRING_TO_JSVAL(::JS_InternString(cx, aPropertyName));

JS_InternString can fail and return null. You might want special handling for
that condition?

I'm amused that much of the trivial renaming of the patch is aJSContext -> cx
while some is principal -> aPrincipal. But it's all the same to me :)

Do you know how often the code does a getService to get the xpconnect service ?
would it be worth holding a longer term reference? Making sure to release it at
the right time would matter, of course. Just a thought.

Anyway... I have to admit that it is not easy to see all the security
implications of this change. I listed above some little changes you might make.
But, I don't see any show stopper problems. sr=jband.







Comment on attachment 68464 [details] [diff] [review]
Patch take 6 - above issues addressed.

setting flags for my sr= and jst's r=
Attachment #68464 - Flags: superreview+
Attachment #68464 - Flags: review+
Final patch, I hope. Only minor changes from the last one, I'm going to get a
verbal OK on those.
Attachment #68464 - Attachment is obsolete: true
Patch checked in, but I'm holding this open as a reminder to integrate
dbradley's GetSecurityInfoAddr() hook into DOMClassInfo, which will provide some
additional optimization.
Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to adt@netscape.com.  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.
Target Milestone: mozilla0.9.9 → mozilla1.2
Clearing milestone for now.
Target Milestone: mozilla1.2alpha → ---
So what does GetSecurityInfoAddr() do exactly, and how could we use it here?
Depends on: 213946
This goes back to bug 121526, XPConnect wrappers have an opaque pointer to
security information. GetSecurityInfoAddr basically returns an address to that
pointer. I'm not sure what optimization Mitch was looking into at the time.
Hmm.. could it hold an object principal or something?  We have a problem with
doGetObjectPrincipal being pretty slow...
I believe it had to do with giving the security component a way to store and
retrieve it's data for a wrapper, which is probably the principal.
I assume this pointer is initialized to null?  And that the security manager can
get notified when the wrapper goes away?  See the discussion in bug 213946...
Is it possible for an object's principal to change (eg for it to move across
documents) without it getting a new XPConnect wrapper?
Yes, that'll happen when document.domain is changed. The actual nsIPrincipal
object remains the same, but the security previleges for objects bound to that
principal may change.
What I mean is, is it possible for the nsIPrincipal associated to an object to
need to change to a new pointer without the object getting a new wrapper?  Say I
have a node, I cache a principal on the wrapper, and then I move the node to a
different document... what happens?

The perf issues in ScriptSecurityManager are mostly related to going from an
object to the nsIPrincipal.  So I'm considering caching the nsIPrincipal on the
wrapper, but I'm trying to make sure it won't open up privilege-escalation
scenarios where a cached principal is used even though it's no longer the
effective principal for that object....
(In reply to comment #43)
> What I mean is, is it possible for the nsIPrincipal associated to an object to
> need to change to a new pointer without the object getting a new wrapper?  Say I
> have a node, I cache a principal on the wrapper, and then I move the node to a
> different document... what happens?

Ah, I see. Well, in a case when a node is moved from one document to another, we
do attempt to reparent its existing wrapper, and in that case the object's
principal would indeed change. The code that does that is in
nsContentUtils::ReparentContentWrapper(), which ultimately calls
nsIXPConnect::ReparentWrappedNativeIfFound().

> The perf issues in ScriptSecurityManager are mostly related to going from an
> object to the nsIPrincipal.  So I'm considering caching the nsIPrincipal on the
> wrapper, but I'm trying to make sure it won't open up privilege-escalation
> scenarios where a cached principal is used even though it's no longer the
> effective principal for that object....

Ah, seems like that could be made safe, if you make
nsIXPConnect::ReparentWrappedNativeIfFound() invalidate the principal that's
cached in the wrapper.
OK.  So then it sounds like the steps to take are:

1) Make the security info settable somehow (perhaps have XPConnect call
   getObjectPrincipal itself?  Or have the script security manager able to set
   the info on the wrapper somehow?)
2) Make sure to release the principal properly when the wrapper goes away
3) Make sure to release the principal in ReparentWrappedNativeIfFound()
4) Change nsScriptSecurityManager::doGetObjectPrincipal to look for the
   principal on the wrapper.

Seem reasonable?  I can do step 4 and probably step 3.  I'm not sure where to
hook into step 2 and what the best approach is for step 1...

Note that this still involves QIing the JSObject's native to
nsIXPConnectWrappedNative, but hopefully that's not too bad.  I don't think
there's a reasonable way to store this info on the JSObject itself.
(In reply to comment #45)
> OK.  So then it sounds like the steps to take are:
> 
> 1) Make the security info settable somehow (perhaps have XPConnect call
>    getObjectPrincipal itself?  Or have the script security manager able to set
>    the info on the wrapper somehow?)

It'd be cool if we could use the existing
nsIXPCWrappedNative::GetSecurityInfoAddress() for this. It's a void pointer (or
a pointer to a void pointer, really, thus you can trivially store any pointer in
a wrapper), so managing ownership will be tricky, but maybe we could figure that
out somehow...

> 2) Make sure to release the principal properly when the wrapper goes away

Right.

> 3) Make sure to release the principal in ReparentWrappedNativeIfFound()

Yup.

> 4) Change nsScriptSecurityManager::doGetObjectPrincipal to look for the
>    principal on the wrapper.

Yeah.

> 
> Seem reasonable?  I can do step 4 and probably step 3.  I'm not sure where to
> hook into step 2 and what the best approach is for step 1...

Maybe we could not refcount the principals, just store the pointer value, and
whenever a principal goes away we just call
nsIXPConnect::ClearAllWrappedNativeSecurityPolicies(), which should null out all
the security info on all live wrappers. This *should* be safe.

> Note that this still involves QIing the JSObject's native to
> nsIXPConnectWrappedNative, but hopefully that's not too bad.  I don't think
> there's a reasonable way to store this info on the JSObject itself.

Correct. Maybe we could get rid of the QI if we'd make caps aware of the JSClass
es that are used by the JS wrappers that are indeed nsIXPConnectWrappedNatives,
there might be more than one, but it may be cheaper to store those in an array,
and check there, before QI'ing. A bit of a hack :-) but might be worth looking
into...
> It'd be cool if we could use the existing
> nsIXPCWrappedNative::GetSecurityInfoAddress() for this

That was exactly the plan.  I sorta missed the fact that it returns a void*.  So
yeah, no need for a setter.  I dunno about people other than the security
manager writing to this data, though....

> nsIXPConnect::ClearAllWrappedNativeSecurityPolicies()

This could be a little painful in edge cases... but I guess principals don't go
away much, do they?
I don't have my head around the entire picture. But would you really need to
clear out all wrappers? If a principal is going away, couldn't you compare the
addresses and only clear that wrappers with that princpal?
(In reply to comment #48)
> I don't have my head around the entire picture. But would you really need to
> clear out all wrappers? If a principal is going away, couldn't you compare the
> addresses and only clear that wrappers with that princpal?

Yes, we could, but we currently don't have an API to do that. The cost of
clearing all of them is relatively small anyways, since all that means is that
we'll end up having to look up the principal for all objects again, which is
what we do *all the time* right now. And given that principals go away rather
rarely, I think clearing all of them wouldn't be too bad.

(In reply to comment #47)
> > It'd be cool if we could use the existing
> > nsIXPCWrappedNative::GetSecurityInfoAddress() for this
> 
> That was exactly the plan.  I sorta missed the fact that it returns a void*.  So
> yeah, no need for a setter.  I dunno about people other than the security
> manager writing to this data, though....

This was meant for use by the security manager, noone else should write to it,
if someone does, that's their problem and the consequences are for them to deal
with.

> > nsIXPConnect::ClearAllWrappedNativeSecurityPolicies()
> 
> This could be a little painful in edge cases... but I guess principals don't go
> away much, do they?

Right. Principals should generally only die when a page is unloaded, i.e. when
you close a window or load a new page into an existing window/frame. And chrome
stuff shouldn't play any role in this since all chrome pages get the system
principal, which is a singleton that only dies on shutdown (IOW, UI code should
have no effect on this).
OK, I just tried to do this.  We have a bit of a problem -- the security info
stuff is:

1)  Stored only on prototypes (so per-class, but that may be OK for us)
2)  Already being used to store the CAPS policy for the class (this is bad, since
    it means we can't use it)
Blocks: 203448
Assignee: security-bugs → dveditz
Status: ASSIGNED → NEW
QA Contact: bsharma → toolkit
Can someone attach a minimal testcase for this bug, or set testcase-needed in Whiteboard?
Lucas, any DOM access is a testcase for this.  There's no need to create a testcase here; they're readily available.

Of course you need to have a profiler to see whether this bug is "fixed".
Depends on: 467900
Depends on: 468370
i'm sorry. we can't leave this bug open. there is established (and sadly broken) code in the tree because of this bug.

Comment 33
> Patch checked in

if people want to do other things (e.g. fix the code, or enhance it), they need to get their own bugs.
Assignee: dveditz → security-bugs
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: