34364 - Principals not correct following function clone

Status: VERIFIED FIXED

(Core :: Security, defect, P3)

Description

[Norris Boyd]

When JavaScript functions are cloned as part of brutal sharing, the principals
may be incorrect--they should be retrieved from the scope chain of the function
object rather than from the principals copied during the function clone.

Comment 1

[Norris Boyd]

Accept bug; needed for skins (right?), set TFV.

Comment 2

[Norris Boyd]

Attachment: proposed patch

Comment 3

[Norris Boyd]

Comments about the changes: All pretty straightforward except for some
refactoring that was required in nsScriptSecurityManager to handle the alternate
means of retrieving principals.

Also, I noticed that fp->argv[-2] can be nonnull even when fp->fun is null and
made a change to account for that in JS_GetFrameFunctionObject. Is this right?

Comment 4

[Brendan Eich [:brendan]]

Hyatt, how soon do you need this?

/be

Comment 5

[Brendan Eich [:brendan]]

Comments, mostly nits:

- use NS_STATIC_CAST to go from jsp to nsJSPrin in GetScriptPrincipal.
- for loop in IsCapabilityEnabled should be do {...} while ((fp = ...) != NULL)
since fp must be non-null the first time through.
- for loop in GetPrincipalAndFrame could be while ((fp = ...) != NULL) {...}.
- I forget why fp->argv would be non-null, but fp->fun would be null -- did you
figure out what was being called or executed in that case?  I.e., what was
fp->argv[-2], if not the callee object?  What were fp->script->filename and
lineno?  Thanks.

/be

Comment 6

[David Hyatt]

So in the skins meeting today, we decided that XBL installed through the skin 
installer should not be allowed to have scripts on it at all.  I'm going to be 
adding an API to the chrome registry that will tell you, for a given chrome URL, 
whether or not it should be allowed to access scripts.

I'll put checks at the XBL level that will disallow the kinds of nodes that can 
contain scripts, thus barring XBL from being unsafe inside skins.

In all other cases, I think the principal should be cloned just as we're doing 
here.

Comment 7

[David Hyatt]

Brendan, take your time.  This blocks included scripts via a <script> tag from 
working and blocks me from doing XBL brutal sharing.  I have other things I can 
work on in the meantime, and I could probably do this work as "performance work" 
following beta2.

Comment 8

[Brendan Eich [:brendan]]

I was really asking about timing just to see whether norris's patch needed to go
in M15, but it sounds like it'll go in next week when the trunk opens for M16.

/be

Comment 9

[Peter Trudelle]

Mass-adding beta2 keyword to all skins bugs.

Comment 10

[Norris Boyd]

- use NS_STATIC_CAST to go from jsp to nsJSPrin in GetScriptPrincipal.
* done.

- for loop in IsCapabilityEnabled should be do {...} while ((fp = ...) != NULL)
since fp must be non-null the first time through.
* I changed it to a for loop so that I could avoid restating "fp =
JS_FrameIterator(cx, &fp)" for the end of the loop statements and for the
continue. I was willing to pay the redundant conditional test for clarity.

- for loop in GetPrincipalAndFrame could be while ((fp = ...) != NULL) {...}.
I also liked the clarity of the explicit "next case" in the for construct.

- I forget why fp->argv would be non-null, but fp->fun would be null -- did you
figure out what was being called or executed in that case? I.e., what was
fp->argv[-2], if not the callee object? What were fp->script->filename and
lineno? Thanks.
* It's a native function, so fp->script is null. The native function is
WrappedNative_Call, so fp->argv[-2] is a XPCWrappedNativeWithCall.

* I also discovered a security hole when looking through the code again. In
CanExecuteFunction, if there is no script currently executing we cannot just
blindly grant access. In this case the point is not who's asking, but what code
will be executed. I talked with jband about what to do for a JSContext in this
case and he pointed me to the "safe" JSContext used by XPConnect.

Comment 11

[Norris Boyd]

Attachment: new proposed patch for nsScriptSecurityManager.cpp

Comment 12

[Brendan Eich [:brendan]]

[my paragraphs below start with tab.  /be]

- for loop in IsCapabilityEnabled should be do {...} while ((fp = ...) != NULL)
since fp must be non-null the first time through.
* I changed it to a for loop so that I could avoid restating "fp =
JS_FrameIterator(cx, &fp)" for the end of the loop statements and for the
continue. I was willing to pay the redundant conditional test for clarity.

	This is not a big deal, but I don't see why the call needs to be restated if
it's nested in the do-while condition.  What am I missing?

- for loop in GetPrincipalAndFrame could be while ((fp = ...) != NULL) {...}.
I also liked the clarity of the explicit "next case" in the for construct.

	Again not a big deal, but both readers and (non-optimizing) compilers will have
to deal with the lengthier and more redundant form.  The 'while ((c = getchar())
!= EOF) ...' loop goes all the way back to the K&R C book, first edition.  It
wouldn't have done for K&R to write 'for (c = getchar(); c != EOF; c =
getchar()) ...', IMHO ;-)

* It's a native function, so fp->script is null. The native function is
WrappedNative_Call, so fp->argv[-2] is a XPCWrappedNativeWithCall.

	Oh, of course -- thanks, duh.  I was worried about this case requiring similar
checks in other code I was hacking, but now I see that it's separate.  See the
uses of fp->argv[-2] in jsfun.c.

* I also discovered a security hole when looking through the code again. In
CanExecuteFunction, if there is no script currently executing we cannot just
blindly grant access. In this case the point is not who's asking, but what code
will be executed.

	Hope I'm not confused: what about an event handler being called from C++? 
Won't it be called with no script currently executing, yet still not want the
safe context?

*I talked with jband about what to do for a JSContext in this
case and he pointed me to the "safe" JSContext used by XPConnect.

/be

Comment 13

[Norris Boyd]

Sorry, I read "do {...} while ((fp = ...) != NULL)" as "do {...} while (fp != 
NULL)". Yours is a fine change, I'll do it in both places.

Whether an event handler is called from C++ or JS makes no difference to this 
method: it's trying to determine whether to run the handler based on the origin 
of the code for the handler. So if the pref is set to disable JS in mail, an 
event handler shouldn't run if it was delivered as part of a mail message, even 
if the code invoking the event handler is C++.

Comment 14

[Norris Boyd]

Fix checked in.

Comment 15

[John Unruh]

Verified per norris' comments.

Comment 16

[John Unruh]

verified