Closed
Bug 119646
Opened 23 years ago
Closed 15 years ago
Speedups of nsScriptSecurityManager::CanAccess
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
RESOLVED
FIXED
People
(Reporter: bratell, Assigned: security-bugs)
References
Details
(Keywords: perf)
Attachments
(1 file, 7 obsolete files)
99.31 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•23 years ago
|
Updated•23 years ago
|
OS: Windows 2000 → All
Hardware: PC → All
Assignee | ||
Comment 1•23 years ago
|
||
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
Assignee | ||
Comment 2•23 years ago
|
||
Reporter | ||
Comment 3•23 years ago
|
||
Just for us curious who doesn't know very much, how is the new code faster? More caching?
Assignee | ||
Comment 4•23 years ago
|
||
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
Comment 5•23 years ago
|
||
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 ;-)
Assignee | ||
Comment 6•23 years ago
|
||
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
Assignee | ||
Comment 7•23 years ago
|
||
Some more improvements - fixed crash in viewer - using unions for security levels instead of casting
Attachment #65715 -
Attachment is obsolete: true
Comment 8•23 years ago
|
||
Will this land after the branch?
Reporter | ||
Comment 9•23 years ago
|
||
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.
Comment 10•23 years ago
|
||
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
Comment 11•23 years ago
|
||
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*.
Reporter | ||
Comment 12•23 years ago
|
||
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.
Comment 13•23 years ago
|
||
The world would fall appart if sizeof(long) != sizeof(void*), this is a safe assumption and it's made all over the mozilla code.
Reporter | ||
Comment 14•23 years ago
|
||
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.
Comment 15•23 years ago
|
||
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...
Comment 16•23 years ago
|
||
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?
Comment 17•23 years ago
|
||
I'd wait for the problems to show up, it'll be obvious when they do...
Comment 18•23 years ago
|
||
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
Comment 19•23 years ago
|
||
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
Reporter | ||
Comment 20•23 years ago
|
||
Or just go back to the casts. That way this code will work forever without any assumptions about specific compiler implementations.
Comment 21•23 years ago
|
||
+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.
Comment 22•23 years ago
|
||
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?
Assignee | ||
Comment 23•23 years ago
|
||
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.
Comment 24•23 years ago
|
||
- 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.
Comment 25•23 years ago
|
||
Um, GetClassName() should initialize the out params even if there's no aClassInfo.
Assignee | ||
Comment 26•23 years ago
|
||
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
Comment 27•23 years ago
|
||
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.
Updated•23 years ago
|
Attachment #68244 -
Flags: review+
Assignee | ||
Comment 28•23 years ago
|
||
Attachment #68244 -
Attachment is obsolete: true
Comment 29•22 years ago
|
||
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 30•22 years ago
|
||
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+
Assignee | ||
Comment 31•22 years ago
|
||
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
Assignee | ||
Comment 32•22 years ago
|
||
Attachment #69166 -
Attachment is obsolete: true
Assignee | ||
Comment 33•22 years ago
|
||
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.
Comment 34•22 years ago
|
||
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
Assignee | ||
Comment 35•21 years ago
|
||
Clearing milestone for now.
Target Milestone: mozilla1.2alpha → ---
Comment 36•20 years ago
|
||
So what does GetSecurityInfoAddr() do exactly, and how could we use it here?
Depends on: 213946
Comment 37•20 years ago
|
||
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.
Comment 38•20 years ago
|
||
Hmm.. could it hold an object principal or something? We have a problem with doGetObjectPrincipal being pretty slow...
Comment 39•20 years ago
|
||
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.
Comment 40•20 years ago
|
||
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...
Comment 41•20 years ago
|
||
Is it possible for an object's principal to change (eg for it to move across documents) without it getting a new XPConnect wrapper?
Comment 42•20 years ago
|
||
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.
Comment 43•20 years ago
|
||
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....
Comment 44•20 years ago
|
||
(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.
Comment 45•20 years ago
|
||
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.
Comment 46•20 years ago
|
||
(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...
Comment 47•20 years ago
|
||
> 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?
Comment 48•20 years ago
|
||
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?
Comment 49•20 years ago
|
||
(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.
Comment 50•20 years ago
|
||
(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).
Comment 51•20 years ago
|
||
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)
Updated•18 years ago
|
Assignee: security-bugs → dveditz
Status: ASSIGNED → NEW
QA Contact: bsharma → toolkit
Comment 52•16 years ago
|
||
Can someone attach a minimal testcase for this bug, or set testcase-needed in Whiteboard?
Comment 53•16 years ago
|
||
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".
Comment 54•15 years ago
|
||
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.
Description
•