307867 - [FIX]Setting capability.policy.default.*.focus to noAccess no longer works

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Setting capability.policy.default.*.focus to noAccess doesn't prevent
http://www.google.com/ from focusing its textbox onload.  Setting
capability.policy.default.HTMLInputElement.focus instead does work.

Why doesn't * work in this case?

MailNews default prefs rely on *.  If * is completely broken, this is a security
hole for MailNews.

Comment 1

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050908
Firefox/1.6a1

Comment 2

[Jesse Ruderman]

mcsmurf says this is indeed broken in mail.  Marking as security-sensitive,
which I should have done in the first place.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

So there are two bugs in our handling of *:

1)  capability.policy.default.*.propName never has any effect
2)  If capability.policy.policyName.className.prop1 is set, then
    capability.policy.policyName.*.prop2 has no effect for instances of
    className.

#1 is not an issue for mailnews, but #2 is a problem for some of our mailnews
policies.

This patch fixes both bugs.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Given that this somewhat a problem for mailnews I do think we want it on the 1.7
branch, but most of the * rules there _do_ work, so maybe it's not urgently
needed for 1.7.12?  dveditz, mscott, bienvenu, what do you think?

Comment 5

[Mike Schroepfer]

1.0.7 is closed - moving the nomination to 1.0.8

Comment 6

[Asa Dotzler [:asa]]

dveditz, time is running out for us on 1.8b5. can you help us with a review here?

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 195962 [details] [diff] [review]
Proposed fix

sr=dveditz
looks good

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195962 [details] [diff] [review]
Proposed fix

Requesting 1.8b5 approval.  This should be pretty safe and make some of our
mailnews security prefs actually work.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195962 [details] [diff] [review]
Proposed fix

Probably worth taking this on the 1.7 branch too...

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 195962 [details] [diff] [review]
Proposed fix

a=dveditz for drivers, please add fixed-aviary1.0.8 and fixed1.7.13 keywords when checked in.

Comment 13

[Boris Zbarsky [:bzbarsky]]

*** Committing to MOZILLA_1_7_BRANCH... 
new revision: 1.229.2.17; previous revision: 1.229.2.16

*** Committing caps/src/nsScriptSecurityManager.cpp on AVIARY_1_0_1_20050124_BRANCH
new revision: 1.229.6.6.2.13; previous revision: 1.229.6.6.2.12

Comment 14

[Bob Clary [:bc] (inactive)]

In a nightly ff 1.0.8/winxp/20060221 build and mozilla 1.7.13/winxp/20060221, using about:config to set capability.policy.default.*.focus to noAccess completed, however attempting to load google crashed. 

Attempting the same in a debug 1.0.8/winxp/20060220 build resulted in the following stack as soon as the pref was set.

PR_IMPLEMENT(PLDHashEntryHdr *)
PL_DHashTableOperate(PLDHashTable *table, const void *key, PLDHashOperator op)
{
    PLDHashNumber keyHash;
    PLDHashEntryHdr *entry;
    PRUint32 size;
    int deltaLog2;

=>    keyHash = table->ops->hashKey(table, key);
    keyHash *= PL_DHASH_GOLDEN_RATIO;

	deltaLog2	0x0012de50
+	entry	0x03b33e4c
	key	0x0339d824
	keyHash	0x8b684024
	op	0x00000000
	size	0x0012df14
+	table	0xdddddddd

PL_DHashTableOperate(PLDHashTable * 0xdddddddd, const void * 0x0339d824, int 0x00000000) line 490 + 11 bytes
nsScriptSecurityManager::LookupPolicy(nsIPrincipal * 0x0320f8a8, const char * 0x100b2074 _js_Object_str, long 0x0339d824, unsigned int 0x00000001, ClassPolicy * * 0x00000000, SecurityLevel * 0x0012e1a0) line 1063 + 21 bytes
nsScriptSecurityManager::CheckPropertyAccessImpl(unsigned int 0x00000001, nsIXPCNativeCallContext * 0x00000000, JSContext * 0x02e592a0, JSObject * 0x03bab380, nsISupports * 0x00000000, nsIURI * 0x00000000, nsIClassInfo * 0x00000000, const char * 0x100b2074 _js_Object_str, long 0x0339d824, void * * 0x00000000) line 635 + 45 bytes
nsScriptSecurityManager::CheckPropertyAccess(nsScriptSecurityManager * const 0x00ec8350, JSContext * 0x02e592a0, JSObject * 0x03bab380, const char * 0x100b2074 _js_Object_str, long 0x0339d824, unsigned int 0x00000001) line 493
nsScriptSecurityManager::CheckObjectAccess(JSContext * 0x02e592a0, JSObject * 0x03bab378, long 0x0339d824, JSAccessMode JSACC_READ, long * 0x0012e264) line 475 + 46 bytes
js_InternalGetOrSet(JSContext * 0x02e592a0, JSObject * 0x03bab378, long 0x0339fef8, long 0x03bab380, int 0x00000004, unsigned int 0x00000000, long * 0x00000000, long * 0x0012e5d8) line 1090 + 421 bytes
js_GetProperty(JSContext * 0x02e592a0, JSObject * 0x03bab378, long 0x0339fef8, long * 0x0012e5d8) line 2832 + 51 bytes
JS_GetProperty(JSContext * 0x02e592a0, JSObject * 0x03bab378, const char * 0x02e963e0, long * 0x0012e5d8) line 2536 + 27 bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x03e550a0, nsXPCWrappedJS * 0x03e550f8, unsigned short 0x0003, const nsXPTMethodInfo * 0x02e961f0, nsXPTCMiniVariant * 0x0012e654) line 1342 + 28 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x03e550f8, unsigned short 0x0003, const nsXPTMethodInfo * 0x02e961f0, nsXPTCMiniVariant * 0x0012e654) line 450
PrepareAndDispatch(nsXPTCStubBase * 0x03e550f8, unsigned int 0x00000003, unsigned int * 0x0012e704, unsigned int * 0x0012e6f4) line 117 + 31 bytes
SharedStub() line 147
nsTreeBodyFrame::Paint(nsTreeBodyFrame * const 0x03d40fa0, nsIPresContext * 0x03a16240, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer eFramePaintLayer_Overlay, unsigned int 0x00000000) line 2171
PresShell::Paint(PresShell * const 0x03cdd59c, nsIView * 0x03d42a68, nsIRenderingContext & {...}, const nsRect & {...}) line 5561 + 31 bytes
nsView::Paint(nsView * const 0x03d42a68, nsIRenderingContext & {...}, const nsRect & {...}, unsigned int 0x00000000, int & 0x00000001) line 262
nsViewManager::RenderDisplayListElement(DisplayListElement2 * 0x03e1d548, nsIRenderingContext * 0x03b2f768) line 1452
nsViewManager::RenderViews(nsView * 0x03a16560, nsIRenderingContext & {...}, const nsRegion & {...}, void * 0x03a12b08, const nsVoidArray & {...}) line 1370
nsViewManager::Refresh(nsView * 0x03a16560, nsIRenderingContext * 0x03b2f768, nsIRegion * 0x03b4ec10, unsigned int 0x00000001) line 929
nsViewManager::DispatchEvent(nsViewManager * const 0x03a16488, nsGUIEvent * 0x0012ecb4, nsEventStatus * 0x0012ebb0) line 1914
HandleEvent(nsGUIEvent * 0x0012ecb4) line 77
nsWindow::DispatchEvent(nsWindow * const 0x03a41f04, nsGUIEvent * 0x0012ecb4, nsEventStatus & nsEventStatus_eIgnore) line 1067 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012ecb4, nsEventStatus & nsEventStatus_eIgnore) line 1093
nsWindow::OnPaint(HDC__ * 0x00000000) line 5052 + 28 bytes
nsWindow::ProcessMessage(unsigned int 0x0000000f, unsigned int 0x00000000, long 0x00000000, long * 0x0012f194) line 3824 + 19 bytes
nsWindow::WindowProc(HWND__ * 0x0048029e, unsigned int 0x0000000f, unsigned int 0x00000000, long 0x00000000) line 1349 + 27 bytes
USER32! 77d48734()
USER32! 77d48816()
USER32! 77d4b4c0()
USER32! 77d4b50c()
NTDLL! 7c90eae3()
nsWindow::DispatchStarvedPaints(HWND__ * 0x0018027a, long 0x00000000) line 3646 + 10 bytes
USER32! 77d4ccd1()
USER32! 77d4da57()
nsWindow::DispatchPendingEvents() line 3692
nsWindow::ProcessMessage(unsigned int 0x00000202, unsigned int 0x00000000, long 0x00010013, long * 0x0012f784) line 4047
nsWindow::WindowProc(HWND__ * 0x003403f8, unsigned int 0x00000202, unsigned int 0x00000000, long 0x00010013) line 1349 + 27 bytes
USER32! 77d48734()
USER32! 77d48816()
USER32! 77d489cd()
USER32! 77d48a10()
nsAppShell::Run(nsAppShell * const 0x02db6198) line 135
nsAppShellService::Run(nsAppShellService * const 0x02db60d8) line 495
xre_main(int 0x00000003, char * * 0x003e6e10, const nsXREAppData * 0x0041e01c kAppData) line 1907 + 35 bytes
main(int 0x00000003, char * * 0x003e6e10) line 58 + 18 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c816d4f()

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  I can't reproduce that -- my 1.7 branch debug build works just fine...

Bob, are you restarting after changing the pref via about:config?  Or no?

Comment 16

[Bob Clary [:bc] (inactive)]

(In reply to comment #15)
> Hmm.  I can't reproduce that -- my 1.7 branch debug build works just fine...
> 

:-/ the story of my life. 

> Bob, are you restarting after changing the pref via about:config?  Or no?
>

no. 

Comment 17

[Boris Zbarsky [:bzbarsky]]

Do you get the same crash if you do restart?  I'd be willing to believe that the security manager somehow screws up dynamic changes to these prefs...

Comment 18

[Bob Clary [:bc] (inactive)]

I can't tell with the nightlies since I can't send in talkbacks. In my debug build it crashes immediately on setting the pref. Adding the pref to my user.js prevents the browser from starting. It just sort of sits there, until I Ctrl-C it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm....  I wish I could reproduce that.  I'll try again with an aviary build tomorrow, I guess.

Comment 20

[Bob Clary [:bc] (inactive)]

ditto crash with ff 1.0.8/mac os x/20060221

Comment 21

[Daniel Veditz [:dveditz]]

If I add this capability as a user_pref() to prefs.js I get what bob saw in comment 18. If I add it as a pref() to all.js then there's no startup issue, but it doesn't seem to prevent focus. (debug 1.0.8 build)

In a release 1.0.7 build the behavior is the same (nothing much) in either place.

Comment 22

[Daniel Veditz [:dveditz]]

Firefox 1.5 is definitely fixed (can even set the capability through about:config successfully), and I verified that the patch has been applied to the aviary101 branch. So why doesn't it work there?

Comment 23

[Daniel Veditz [:dveditz]]

I found that the old branches don't have the crash fix from bug 261339 which seemed promising ("Setting capability.policy.default.Window.top to noAccess seems to crash mozilla"), but the patch doesn't seem to help

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: This ought to fix the crash, I think

Comment 25

[timeless]

Comment on attachment 212925 [details] [diff] [review]
This ought to fix the crash, I think

can you add a comment to nsScriptSecurityManager.cpp?

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 212925 [details] [diff] [review]
This ought to fix the crash, I think

> struct ClassPolicy : public PLDHashEntryHdr
> {
>     char* key;
>     PLDHashTable* mPolicy;
>+    // Note: the DomainPolicy owns us, so if if dies we will too.  Hence no
>+    // need to refcount it here (and in fact, we'd probably leak if we tried).
>+    DomainPolicy* mDomainWeAreWildcardFor;
> };

This reads a little better with a blank line before the comment, IMHO.  Up to you to pick this nit.

>+MoveClassPolicyEntry(PLDHashTable *table,
>+                     const PLDHashEntryHdr *from,
>+                     PLDHashEntryHdr *to)
>+{
>+    memcpy(to, from, table->entrySize);
>+
>+    // Now update the mDefaultPolicy pointer that points to us, if any.
>+    ClassPolicy* cp = NS_STATIC_CAST(ClassPolicy*, to);
>+    if (cp->mDomainWeAreWildcardFor) {
>+        cp->mDomainWeAreWildcardFor->mWildcardPolicy = cp;
>+    }

Just for reader edification, and as a sanity check in case memory is trashed or pldhash breaks, assert that cp->mDomainWeAreWildcardFor->mWildcardPolicy == from before storing cp?

Thanks for fixing this -- sorry I didn't catch it during ancient review cycles.

/be

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 212934 [details] [diff] [review]
Updated to comments

>+        NS_ASSERTION(cp->mDomainWeAreWildcardFor->mWildcardPolicy ==
>+                     NS_STATIC_CAST(const ClassPolicy*, from),
>+                     "Unexpected wildcard policy on mDomainWeAreWildcardFor");

This was my concern too. Is an assertion strong enough or are we confident this isn't going to come up in real life?

>+        cp->mDomainWeAreWildcardFor->mWildcardPolicy = cp;

Would the domain policy ever get moved first (making cp->mDomainWeAreWildcardFor bogus)? Do we need an analogous

        if (cp->mWildcardPolicy)
          cp->mWildcardPolicy->mDomainWeAreWildcardFor = cp;

(and corresponding assertion) to catch that?

r=dveditz with those concerns. Tested in my crashing tree and it does stop the crash/hang

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #28)
> (From update of attachment 212934 [details] [diff] [review] [edit])
> >+        NS_ASSERTION(cp->mDomainWeAreWildcardFor->mWildcardPolicy ==
> >+                     NS_STATIC_CAST(const ClassPolicy*, from),
> >+                     "Unexpected wildcard policy on mDomainWeAreWildcardFor");
> 
> This was my concern too. Is an assertion strong enough or are we confident this
> isn't going to come up in real life?

The pldhash/jsdhash code is easy to analyze.  The moveEntry callback is used as you would expect.
> 
> >+        cp->mDomainWeAreWildcardFor->mWildcardPolicy = cp;
> 
> Would the domain policy ever get moved

The domain policy is a PLDHashTable subtype, not a PLDHashEntryHdr subtype.  Entries may move, tables do not.

/be

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 212934 [details] [diff] [review]
Updated to comments

I think we want this on the 1.8 branches, too.

a=dveditz for the aviary/moz17 branches

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 212934 [details] [diff] [review]
Updated to comments

I think we want this on the 1.8 branches, too.

a=dveditz for the aviary/moz17 branches

Comment 32

[Boris Zbarsky [:bzbarsky]]

What brendan said.  The assertion is not getting hit in our current code, ever.  And DomainEntrys can't change pointer (eg they're pointed to in other places, reference counted, etc).

I've checked in the fix on the aviary and 1.7 branches; I'll work on getting a patch that applies to 1.8 and trunk (or separate patches?) posted.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8/1.8.0 version of crash fix

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk version of crash fix

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 212999 [details] [diff] [review]
Trunk version of crash fix

Checked this in on trunk

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 212998 [details] [diff] [review]
1.8/1.8.0 version of crash fix

a=dveditz for 1.8.1
approved for 1.8.0 branch, a=dveditz

Comment 37

[Boris Zbarsky [:bzbarsky]]

Checked in crash fix on both 1.8 branches.

Comment 38

[Bob Clary [:bc] (inactive)]

v no crash on setting policy and that policy prevents google from setting focus to text input with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060224 Firefox/1.0.8

There are no linux builds for today.

Comment 39

[Bob Clary [:bc] (inactive)]

v moz1.7.13 winxp 2006022718

Comment 40

[Bob Clary [:bc] (inactive)]

v 1.8.0.2/winxp/20060308