InteractiveAdvertisingBureau/safeframe

src/js/ext/ext.js _handle_msg prevents third-party HTML5 creative from itself using postMessage with a child IFRAME

Opened this issue · 18 comments

https://github.com/InteractiveAdvertisingBureau/safeframe/blob/master/src/js/ext/ext.js line 492 in _handle_msg() in the SafeFrames implementation currently cancels the postMessage event, always, as per this comment above _handle_msg():

    /**
     * Handle onmessage HTML5 x-domain events. We always cancel the event
     * never allowing it to go to other listeners besides our own, as we don't allow HTML5 messaging
     * beyond us and the publisher / host.
     * [...]
    */

This has the side effect of preventing any ad server's third-party script loaded into the safe-framed hosted environment (e.g., into http://sourceforge.net/p/safeframes/wiki/Home/) from installing its own IFRAME and postMessage-based protocol to shield code developed by a third-party developer from the parent page environment it is loaded into (irrespective of whether it is a safeFrame or non-safeFrame environment).

Is there any reason this is being prevented by cancelling the postMessage event? Would it be acceptable to move the dom.evtCncl() into the innermost "if" block, thereby only cancelling the bubbling up if the event is one generated by our own parent and matching our own "guid", and let other users of postMessage continue to work?

How about introducing a new ext API method: $sf.ext.regMessageListener(source_window, message_handler) -- this allows ext code to 'safely' register a message listener in the SafeFrame on a desired window object (e.g., iframe.contentWindow for an iframe the ext code itself might install), as long as the specified source_window object is not window, window.parent, or window.top ?

I submitted #10 for consideration.

+1

+1+1+1+1+1+1+1+1+1

+1

+1

+1

If intention of cancelling all events is to prevent non-safeframe js from intercepting safeframe messages then cancel should happen inside if block as @bmilekic suggested. Cancellation of all events just makes developers life harder. It's still possible for safe-framed js to communicate with cross domain iframe it creates: safe-framed js can create friendly iframe and register message listener inside that iframe and cross-domain iframe js must locate that friendly iframe and send messages to it. But it's messy, involves extra iframes and we (ads tech) should avoid unnecessary complexity which only makes browser slower.

Also safeframe spec doesn't say anything about intentionally breaking postMesage communication inside safeframe. postMessage is one of essential APIs so it would be great if this was fixed.

@seansd could you take a look?

That is partially they intention, and it's also b/c in older browsers where postMessage is supported (namely IE8) the ordering of event handlers called is not guaranteed to be the order in which listeners are attached, and so cancellation alone is not good enough. There is no easy fix for this without changing how the communication is setup in the first place (which I do intend to do, and likely also drop IE8 support at that time).

Could you elaborate more on intention of the cancellation? Does moving cancellation into "if" block make it less secure? I probably don't understand all consequences of cancelling only safeframe-messages instead of cancelling all events.

i'm just cancelling message events b/c you don't want those to leak down to another (nefarious) caller

But why do you cancel non-safeframe messages? Spec doesn't say anything about banning postMessage-based communication inside SafeFrame. I get the point of cancelling messages that were sent by SafeFrame host (or from top window) but other messages were not sent by SafeFrame so cancelling just breaks js running inside SafeFrame. Messages sent from nested iframes should be allowed.

As I mentioned, today it is possible to setup postMessage communication using dedicated friendly iframe. So if the point is to forbid all postMessage-communication inside SafeFrame for security reasons then just breaking it inside SafeFrame window is not sufficient. And I really hope that it's not the intention to ban all postMessages inside SafeFrame.

Messages sent from nested iframes should be allowed.
Why? What are the use cases where a nested iframe below would need to communicate to the SafeFrame itself. . . a nested frame should not be able to talk to the SafeFrame API. . .

But why do you cancel non-safeframe messages?
Because it's not clear at that point that it's a "safe frame message"

As I mentioned, today it is possible to setup postMessage communication using dedicated friendly iframe. So if the point is to forbid all postMessage-communication inside SafeFrame for security reasons then just breaking it inside SafeFrame window is not sufficient

It is b/c if you setup your own friendly iframe to communicate with there is no way that's a safeframe message since you can only do that after everything else is setup. That said it is possible theorhetically depending on browser to get a hook in before the SafeFrame itself is setup, but that's very hard to do. . .

That said I want to change this so it's a non-issue but I have to rework the message pipeline and thereby the rendering architecture to do so. . .

I still don't fully understand why reference implementation restricts postMessagin'g in a way that not mentioned in the specification.

Anyway, is there timeline for the rework you mentioned? This bug affects some of our logic and we need to figure out which way to go, either add not-great workarounds to our code or maybe wait for the rework.

+1

Timeline?

+1, this safeframe code is forcing us to code inefficient non-optimal (possibly costly) work-arounds, when will this be fixed?

it's not going to be fixed in this version, it's not up for further discussion really. there's a security gap that could be exposed much more easily without having this in place and ad content / frameworks really should not be sending unwarranted messages anyway since the publisher page / host is forced to either listen to all or none. . . i can't really elaborate more than that. . . Messaging mechanics in newer browsers have some things that could mitgate this but I have to change the baseline architecture to do that and that will take time and this library still conforms to being able to deal with IE 8, as such it's going to be this way. .