Boris Zbarsky [:bzbarsky] Assignee
	Description • 11 years ago

This is fallout from bug 918345.

I guess I'll add the annotations I want for bug 945411 in a minimal patch here, then do more with them later.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 11 years ago

Specifically, what fails is:

  window.focus.call(crossOriginWindow);

Direct calls work fine, because they end up entering the cross-origin compartment.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 4 • 11 years ago

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

Maybe I should name those extended attributes CrossOriginAccessible and Getter/SetterCrossOriginAccessible?  Note that we'll need to spec these, so we want sane names....  I'll start a public-script-coord thread on the naming.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 11 years ago

Though I guess WebIDL actually punts this to HTML, so we won't need a standard annotation here.

	Bobby Holley (:bholley)
	Comment 6 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #4)
> Maybe I should name those extended attributes CrossOriginAccessible and
> Getter/SetterCrossOriginAccessible?  Note that we'll need to spec these, so
> we want sane names....  I'll start a public-script-coord thread on the
> naming.

I think we should do [CrossOriginAccessible] for methods, and [CrossOriginReadable]/[CrossOriginWritable] for attributes.

	Bobby Holley (:bholley)
	Comment 7 • 11 years ago

Comment on attachment 8342160 [details] [diff] [review]
part 1.  Allow this-unwrapping to do an unchecked unwrap for properties/methods that are flagged appropriately.   peterv

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

I don't know enough Codegen.py to give this a real review, but f=bholley with the below answered/addressed.

::: dom/bindings/Codegen.py
@@ +1520,5 @@
>                         "methodInfo": not m.isStatic(),
>                         "length": methodLength(m),
>                         "flags": "JSPROP_ENUMERATE",
> +                       "condition": PropertyDefiner.getControllingCondition(m),
> +                       "uncheckedThis": m.getExtendedAttribute("SkipsThisSecurityCheck")}

I'd probably prefer a more explicit name for this, something like "allowCrossOriginThis". But I also understand the symmetry with CheckedUnwrap/UncheckedUnwrap. Your call.

@@ +1651,5 @@
>              if self.static:
>                  accessor = 'get_' + attr.identifier.name
>                  jitinfo = "nullptr"
>              else:
> +                if attr.hasLenientThis():

What is a lenient this?

::: dom/bindings/parser/WebIDL.py
@@ +2707,5 @@
> +                                  [attr.location, self.location])
> +            if self.getExtendedAttribute("SetterSkipsThisSecurityCheck"):
> +                raise WebIDLError("[LenientThis] is not allowed in combination "
> +                                  "with [SetterSkipsThisSecurityCheck]",
> +                                  [attr.location, self.location])

See my suggestion above for the naming.

@@ +2764,5 @@
> +        elif (identifier == "GetterSkipsThisSecurityCheck" or
> +              identifier == "SetterSkipsThisSecurityCheck"):
> +            if not attr.noArguments():
> +                raise WebIDLError("[%s] must take no arguments" % identifier,
> +                                  [attr.location])

In what situation does an attribute take arguments?

	Bobby Holley (:bholley)
	Comment 8 • 11 years ago

Comment on attachment 8342161 [details] [diff] [review]
part 2.  Allow doing certain operations on Window cross-origin, like we should.

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

I feel pretty strongly that we should introduce annotations in IDL in the same patch as where we CodeGen IsPermitted and remove the hard-coded version. It probably even makes sense to do that in a patch before the one where we handle those attributes in Codegen.py.

If this is burdensome somehow, let me know and we can talk about it.

	Bobby Holley (:bholley)
	Comment 9 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #1)
> Specifically, what fails is:
> 
>   window.focus.call(crossOriginWindow);
> 
> Direct calls work fine, because they end up entering the cross-origin
> compartment.

Is this actually true? If so, this bug should be hidden.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 11 years ago

> I think we should do [CrossOriginAccessible] for methods, and
> [CrossOriginReadable]/[CrossOriginWritable] for attributes.

Will do.

> I don't know enough Codegen.py to give this a real review

I mostly wanted your review on the naming here.  Peter will cover the rest.

> I'd probably prefer a more explicit name for this, something like
> "allowCrossOriginThis". 

OK.  I can do that.

> What is a lenient this?

If "this" is the wrong type, just silently no-op.  Some things on the web need this behavior....

> In what situation does an attribute take arguments?

This is about extended attributes.  So [Foo] is an extended attribute without arguments and [Pref("something.other")] is one with arguments.  This is just asserting people didn't write "[CrossOriginReadable(foopy)]".

> I feel pretty strongly that we should introduce annotations in IDL in the same patch as
> where we CodeGen IsPermitted 

I plan to do that ASAP, but this patch needs to land on m-c soonish and on Aurora 27 (or likely Beta 27 by then) to fix the functionality regression we have...

The other option is to try to hack in calls to the current access check code in this-unwrapping here, which seems like wasted effort given that we plan to have the IDL annotations generating AccessCheck stuff soon (matter of days on m-c).

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 11 years ago

> Is this actually true?

Turns out, no.  What's actually happening is that the call is via an Xray, which calls the XPConnect methods, not the WebIDL ones.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 11 years ago

Alright, talked to bholley.  He would really like us to codegen IsPermitted even for the branch backports, so let's do that.

	Bobby Holley (:bholley)
	Comment 17 • 11 years ago

Comment on attachment 8342740 [details] [diff] [review]
part 2.  Generate Window's access checks in XPConnect based on WebIDL access annotations.

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

I didn't look much at the Codegen, but did a careful review of the generated IsPermitted. Looks good! Thanks for doing this :-)

	Peter Van der Beken [:peterv]
	Comment 19 • 11 years ago

Comment on attachment 8342740 [details] [diff] [review]
part 2.  Generate Window's access checks in XPConnect based on WebIDL access annotations.

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

::: dom/bindings/Codegen.py
@@ +2540,5 @@
> +            else:
> +                assert name in readwrite
> +            firstLetter = name[0]
> +            case = cases.get(firstLetter, CGList([], "\n"))
> +            case.append(CGGeneric("if (%s) return true;" % cond))

I think I'd like to follow the style of the rest of the generated code (return on its own line, braces around it).

	Peter Van der Beken [:peterv]
	Comment 20 • 11 years ago

Comment on attachment 8342741 [details] [diff] [review]
part 3.  Adjust codegen to allow cross-origin this values based on WebIDL annotations.

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

::: dom/base/test/test_window_cross_origin_props.html
@@ +19,5 @@
> +
> +  function doSet(prop, thisObj, value) {
> +    return Object.getOwnPropertyDescriptor(window, prop).set.call(thisObj, value);
> +  }
> +  

Trailing whitespace.

@@ +78,5 @@
> +    // "window" is not a getter property yet
> +    //is(doGet("window", frames[0]), frames[0], "window getter should still work");
> +    todo_isnot(Object.getOwnPropertyDescriptor(window, "window").get, undefined,
> +              "Should have a getter here");
> +  

Trailing whitespace.

::: dom/bindings/Codegen.py
@@ +5682,5 @@
>      called 'args'.  This can be "" if there is already such a variable
>      around.
> +
> +    If allowCrossOriginThis is true, then this-unwrapping will first do an
> +    UncheckedUnwrap and after that operated on the result.

s/operated/operate/

@@ +5780,5 @@
>      """
> +    A class for generating the C++ code for an IDL method.
> +
> +    If allowCrossOriginThis is true, then this-unwrapping will first do an
> +    UncheckedUnwrap and after that operated on the result.

s/operated/operate/

@@ +5988,5 @@
>                  "}\n"
>                  "args.rval().set(JS::UndefinedValue());\n"
>                  "return true;")
>          else:
> +            if allowCrossOriginThis:

elif

@@ +6100,5 @@
>                  "}\n"
>                  "args.rval().set(JS::UndefinedValue());\n"
>                  "return true;")
>          else:
> +            if allowCrossOriginThis:

elif

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 11 years ago

> elif

Not quite, because we want the same unwrapFailureCode no matter whether allowCrossOriginThis is true...  So I'd prefer to keep this part as-is.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 22 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/698fbdf07307
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa1529af448
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1ae27215d1b

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 11 years ago

Comment on attachment 8342739 [details] [diff] [review]
part 1.  Add support for WebIDL extended attributes to allow annotating allowed cross-origin access.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 918345
User impact if declined: Some web pages might break in some cases
Testing completed (on m-c, etc.): Passes regression tests
Risk to taking this patch (and alternatives if risky): Medium risk, I think.
    This is about as safe as security-related changes get, but this _is_ a
    security-related change.
String or IDL/UUID changes made by this patch: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/698fbdf07307
https://hg.mozilla.org/mozilla-central/rev/2aa1529af448
https://hg.mozilla.org/mozilla-central/rev/c1ae27215d1b
https://hg.mozilla.org/mozilla-central/rev/991f8be2963d

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 25 • 11 years ago

Comment on attachment 8342739 [details] [diff] [review]
part 1.  Add support for WebIDL extended attributes to allow annotating allowed cross-origin access.

Well it's early in the cycle so let's get as much bake time as possible. Any extra checking QA should be doing to try and kick the tires here?

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 26 • 11 years ago

Comment on attachment 8342740 [details] [diff] [review]
part 2.  Generate Window's access checks in XPConnect based on WebIDL access annotations.

tracked bug 949940 to be sure we're getting both into 27.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 27 • 11 years ago

(In reply to Lukas Blakk [:lsblakk] from comment #26)
> Comment on attachment 8342740 [details] [diff] [review]
> part 2.  Generate Window's access checks in XPConnect based on WebIDL access
> annotations.
> 
> tracked bug 949940 to be sure we're getting both into 27.

should read 28, not 27

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 28 • 11 years ago

As far as QA, not sure.  The fuzzing is already happening, as you noticed.  Past that, the main thing that would be worth testing is that we expose exactly the right sort of things cross-origin.  But we have decent automated tests for that too...

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/9901690374a7
https://hg.mozilla.org/releases/mozilla-aurora/rev/74e283087be2
https://hg.mozilla.org/releases/mozilla-aurora/rev/64770afb2313

	Jean-Yves Perrier [:teoli]
	Comment 30 • 9 years ago

Adding dev-doc-needed here because it adds CrossOriginReadable, CrossOriginWritable and CrossOriginCallable that need to be documented there: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Custom_extended_attributes

(Found this as I was dealing with Location ;-) )

	Jean-Yves Perrier [:teoli]
	Comment 31 • 9 years ago

So if I understand correctly:

1. Window and Location (only) can be accessed (that is attributes can be set or get, methods can be called) from a same-origin context only.

2. CrossOriginReadable, CrossOriginWritable and CrossOriginCallable lift this restriction in the specific case (attribute get, attribute set, method call)

Side question: is there a way to know if only Window, Location are subject to this restriction by reading the WebIDL?

Am I correct?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 32 • 9 years ago

You understand correctly for items 1 and 2.  I guess these extended attributes aren't in a spec (yet, at least) so we really should add them to the docs...

> is there a way to know if only Window, Location are subject to this restriction by reading the WebIDL?

All objects are subject to the same-origin access restriction.  Window and Location are the only ones which loosen it in some cases.

In practice, if you ignore document.domain, Window and Location are the only cases when you can have a reference to a cross-origin object at all.

	Jean-Yves Perrier [:teoli]
	Comment 33 • 9 years ago

I made a stub explanation there: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#CrossOriginReadable