Can no longer focus() a cross-origin window

RESOLVED FIXED in Firefox 28

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({dev-doc-complete})

unspecified
mozilla29
x86
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28+ fixed, firefox29 fixed)

Details

(Whiteboard: [qa-])

Attachments

(4 attachments, 2 obsolete attachments)

4.52 KB, patch
peterv
: review+
Details | Diff | Splinter Review
13.42 KB, patch
peterv
: review+
bholley
: review+
Details | Diff | Splinter Review
22.53 KB, patch
peterv
: review+
Details | Diff | Splinter Review
1.44 KB, text/plain
Details
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.
Specifically, what fails is:

  window.focus.call(crossOriginWindow);

Direct calls work fine, because they end up entering the cross-origin compartment.
Created attachment 8342160 [details] [diff] [review]
part 1.  Allow this-unwrapping to do an unchecked unwrap for properties/methods that are flagged appropriately.   peterv
Attachment #8342160 - Flags: review?(peterv)
Attachment #8342160 - Flags: review?(bobbyholley+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Created attachment 8342161 [details] [diff] [review]
part 2.  Allow doing certain operations on Window cross-origin, like we should.
Attachment #8342161 - Flags: review?(bobbyholley+bmo)
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.
Though I guess WebIDL actually punts this to HTML, so we won't need a standard annotation here.
(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.
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?
Attachment #8342160 - Flags: review?(bobbyholley+bmo) → feedback+
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.
Attachment #8342161 - Flags: review?(bobbyholley+bmo) → review-
(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.
Flags: needinfo?(bzbarsky)
> 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).
Flags: needinfo?(bzbarsky)
> 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.
Comment 9 is private: false
Alright, talked to bholley.  He would really like us to codegen IsPermitted even for the branch backports, so let's do that.
Created attachment 8342739 [details] [diff] [review]
part 1.  Add support for WebIDL extended attributes to allow annotating allowed cross-origin access.
Attachment #8342739 - Flags: review?(peterv)
Attachment #8342160 - Attachment is obsolete: true
Attachment #8342160 - Flags: review?(peterv)
Created attachment 8342740 [details] [diff] [review]
part 2.  Generate Window's access checks in XPConnect based on WebIDL access annotations.
Attachment #8342740 - Flags: review?(peterv)
Attachment #8342740 - Flags: review?(bobbyholley+bmo)
Created attachment 8342741 [details] [diff] [review]
part 3.  Adjust codegen to allow cross-origin this values based on WebIDL annotations.
Attachment #8342741 - Flags: review?(peterv)
Created attachment 8342742 [details]
The generated WindowBinding::IsPermitted, for auditing
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 :-)
Attachment #8342740 - Flags: review?(bobbyholley+bmo) → review+
Whiteboard: [need review]
Attachment #8342739 - Flags: review?(peterv) → review+
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).
Attachment #8342740 - Flags: review?(peterv) → review+
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
Attachment #8342741 - Flags: review?(peterv) → review+
> 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.
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.
Attachment #8342739 - Flags: approval-mozilla-aurora?
Whiteboard: Needs to be uplifted together with bug 949940
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?
Attachment #8342739 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8342740 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8342741 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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
status-firefox28: --- → affected
status-firefox29: --- → fixed
tracking-firefox28: --- → +
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...
Whiteboard: [qa-]
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 ;-) )
Keywords: dev-doc-needed
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?
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.
I made a stub explanation there: https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#CrossOriginReadable
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.