Jim Blandy :jimb Assignee
	Description • 11 years ago

To help Nick test his ongoing work in bug 961288, it would be nice if the JavaScript shell actually had principals that one could play with.

This patch implements a 'princpal' option to the shell's newGlobal function. The help string says:

      principal: if present, its value converted to a number must be an
         integer that fits in 32 bits; use that as the new compartment's
         principal. Shell principals are toys, meant only for testing; one
         shell principal subsumes another if its set bits are a superset of
         the other's. Thus, a principal of 0 subsumes nothing, while a
         principals of ~0 subsumes all other principals. The absence of a
         principal is treated as if its bits were 0xffff, for subsumption
         purposes. If this property is omitted, supply no principal.

With properly chosen values, this can implement any lattice of principals, assuming that the lattice has at most 32 points in it. That should be plenty for tests.

This patch needs unit tests; just posting in case people find it useful, or have comments.

	Jim Blandy :jimb Assignee
	Comment 1 • 11 years ago

A demo:

$ cat ~/moz/princ.js
var low = newGlobal({ principal: 0 });
var high = newGlobal({ principal: 0xfffff });

low .eval('function a() { print(Error().stack); b(); }');
     eval('function b() { print(Error().stack); c(); }');
high.eval('function c() { print(Error().stack); d(); }');
     eval('function d() { print(Error().stack); e(); }');
low .eval('function e() { print(Error().stack);      }');

low.b = b;
c = high.c;
high.d = d;
e = low.e;

low.a();
$ obj~/js/src/js -f ~/moz/princ.js
a@/home/jimb/moz/princ.js line 4 > eval:1

b@/home/jimb/moz/princ.js line 5 > eval:1
a@/home/jimb/moz/princ.js line 4 > eval:1
@/home/jimb/moz/princ.js:15

c@/home/jimb/moz/princ.js line 6 > eval:1
b@/home/jimb/moz/princ.js line 5 > eval:1
a@/home/jimb/moz/princ.js line 4 > eval:1
@/home/jimb/moz/princ.js:15

d@/home/jimb/moz/princ.js line 7 > eval:1
c@/home/jimb/moz/princ.js line 6 > eval:1
b@/home/jimb/moz/princ.js line 5 > eval:1
a@/home/jimb/moz/princ.js line 4 > eval:1
@/home/jimb/moz/princ.js:15

e@/home/jimb/moz/princ.js line 8 > eval:1
a@/home/jimb/moz/princ.js line 4 > eval:1

$ 

Note that the default is for compartments to be created with no principals at all, which causes checks to be omitted altogether, which is why 'd' can see the whole stack. And, note that 'a' and 'e' can only see each other, not the top-level (anonymous) frame, nor that for 'c'.

	Jim Blandy :jimb Assignee
	Comment 2 • 11 years ago

This seems like an obvious and trivial cleanup.

	Jim Blandy :jimb Assignee
	Comment 3 • 11 years ago

Now, with tests!

	Jeff Walden [:Waldo]
	Comment 4 • 11 years ago

Comment on attachment 8372106 [details] [diff] [review]
Move definition of JSSubsumesOp to where it belongs.

Review of attachment 8372106 [details] [diff] [review]:
-----------------------------------------------------------------

So, yeah, this location doesn't make sense now, but jsapi.h as dumping ground seems somewhat unideal.  A new js/public/ header for security-related concepts seems nice, then that can move there.  Maybe for security callbacks and principal stuff, initially, then we can expand or contract as needed with experience.  I'd just #include it in jsapi.h to start, to start dividing things without requiring adjusting all the callers right from the start, myself.  (Particularly as jsapi.h probably depends on principal definition stuff now, so a full split might be hard.)  If you want to go further, fine with me.

	Jim Blandy :jimb Assignee
	Comment 5 • 11 years ago

Revised per comments.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 6 • 11 years ago

I think one of these patches has bit rotted.

Would love to see some action in this bug since it is blocking me.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 8 • 11 years ago

Comment on attachment 8372679 [details] [diff] [review]
Implement a toy principal type for the JS shell, for testing.

Review of attachment 8372679 [details] [diff] [review]:
-----------------------------------------------------------------

I have one non-nit comment to address. r=me with it fixed.

::: js/src/shell/js.cpp
@@ +3903,2 @@
>      if (!global)
>          return false;

If this fails, then we'll leak principals, right? It looks like you should hold a strong ref to the nascent principals in this function.

	Jeff Walden [:Waldo]
	Comment 9 • 11 years ago

Comment on attachment 8372679 [details] [diff] [review]
Implement a toy principal type for the JS shell, for testing.

Review of attachment 8372679 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/basic/shell-principals.js
@@ +29,5 @@
> +     eval('function a() { check("a",        Error().stack); b(); }');
> +low .eval('function b() { check("b",        Error().stack); c(); }');
> +mid .eval('function c() { check("cba",      Error().stack); d(); }');
> +high.eval('function d() { check("dcba",     Error().stack); e(); }');
> +     eval('function e() { check("edcba",    Error().stack); f(); }'); // no principal, so checks skipped

If no principal (nullptr) means checks get skipped, wouldn't it be more accurate to equate it with the system/trusted principal, with principal bits 0xffffffff and not 0xffff?  See also comments on the other file.

@@ +49,5 @@
> +// Kick the whole process off.
> +a();
> +
> +assertEq(count, 8);
> +

Trailing blank line?

::: js/src/shell/js.cpp
@@ +203,5 @@
> + * In the shell, a principal is simply a 32-bit mask: P subsumes Q if the
> + * set bits in P are a superset of those in Q. Thus, the principal 0 is
> + * subsumed by everything, and the principal ~0 subsumes everything.
> + *
> + * As a special case, a null pointer as a principal is treated like 0xffff.

0xffff and not 0xffffffff?  Not sure what you're trying to emulate by having nullptr, to know if having it emulate a not-total set of capabilities is specifically desired.

@@ +208,5 @@
> + *
> + * The 'newGlobal' function takes an option indicating which principal the
> + * new global should have; 'evaluate' does for the new code.
> + */
> +class ShellPrincipals: public JSPrincipals {

class ShellPrincipals : public JSPrincipals
{

@@ +212,5 @@
> +class ShellPrincipals: public JSPrincipals {
> +    uint32_t bits;
> +
> +    static uint32_t getBits(JSPrincipals *p) {
> +        if (!p) return 0xffff;

Two lines.  Or combine via ternary expression, either way.

@@ +218,5 @@
> +    }
> +
> +  public:
> +    ShellPrincipals(uint32_t bits, int32_t refCount = 0) : bits(bits) {
> +        refcount = refCount;

The capitalization mismatching here is not particularly readable.

@@ +245,5 @@
> +    subsumes
> +};
> +
> +// The fully-trusted principal subsumes all other principals.
> +ShellPrincipals ShellPrincipals::fullyTrusted(~0, 1);

You're not going to make people happy with the static initializer here, but hopefully this doesn't affect the tinderbox-visible number.

Also, ~uint32_t(0).

@@ +3864,5 @@
>  }
>  
>  static JSObject *
> +NewGlobalObject(JSContext *cx, JS::CompartmentOptions &options,
> +                JSPrincipals *principals = nullptr);

Kinda rather just see the principals argument be made non-optional here.

	Jim Blandy :jimb Assignee
	Comment 10 • 11 years ago

Thanks for the review!


(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> If no principal (nullptr) means checks get skipped, wouldn't it be more
> accurate to equate it with the system/trusted principal, with principal bits
> 0xffffffff and not 0xffff?  See also comments on the other file.

I don't *think* the "checks get skipped" behavior is exactly equivalent to a fully trusted principal. (Note that, both before and after the patch, the principal passed to JS_SetTrustedPrincipals was not the default principal.)

The choice of 0xffff makes it possible to create other principals that both subsume and are subsumed by that. If we actually want to allow people to test the interaction between 'no principals' and specific existing principals, then it seems like choosing a value that lets people go on either side of that line is the most general behavior.

But perhaps "skipping checks" is supposed to behave universally like a subsumes-all principal? That's not the impression I got from the code...

> Trailing blank line?

Fixed, thanks.

> ::: js/src/shell/js.cpp
> @@ +203,5 @@
> > + * In the shell, a principal is simply a 32-bit mask: P subsumes Q if the
> > + * set bits in P are a superset of those in Q. Thus, the principal 0 is
> > + * subsumed by everything, and the principal ~0 subsumes everything.
> > + *
> > + * As a special case, a null pointer as a principal is treated like 0xffff.
> 
> 0xffff and not 0xffffffff?  Not sure what you're trying to emulate by having
> nullptr, to know if having it emulate a not-total set of capabilities is
> specifically desired.

Hopefully explained above.


> Two lines.  Or combine via ternary expression, either way.

Done.


> The capitalization mismatching here is not particularly readable.

Good point; new code changed to match JSPrincipals member.


> You're not going to make people happy with the static initializer here, but
> hopefully this doesn't affect the tinderbox-visible number.

"It's only the shell," I thought.

> Also, ~uint32_t(0).

Changed to -1.


> Kinda rather just see the principals argument be made non-optional here.

Changed.

	Jim Blandy :jimb Assignee
	Comment 11 • 11 years ago

Try:
https://tbpl.mozilla.org/?tree=Try&rev=9a425137344d

	Jim Blandy :jimb Assignee
	Comment 12 • 11 years ago

That ran afoul of the SpiderMonkey style checker. Revised try:
https://tbpl.mozilla.org/?tree=Try&rev=dc66a1a10809

	Jim Blandy :jimb Assignee
	Comment 13 • 11 years ago

Try push looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/49fcef3c246f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e02bf51cb69

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/49fcef3c246f
https://hg.mozilla.org/mozilla-central/rev/8e02bf51cb69