Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bmilekic opened this issue Dec 14, 2015 · 18 comments

Comments

@bmilekic
Copy link

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?

@bmilekic
Copy link
Author

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 ?

@bmilekic
Copy link
Author

I submitted #10 for consideration.

@TRwhaler
Copy link

+1

1 similar comment
@bedo-code
Copy link

+1

@TRwhaler
Copy link

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

@emartgu
Copy link

emartgu commented Aug 15, 2017

+1

1 similar comment
@kyleder
Copy link

kyleder commented Oct 27, 2017

+1

@nbeloglazov
Copy link

nbeloglazov commented Nov 3, 2017

+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?

@seansd-zz
Copy link
Collaborator

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).

@nbeloglazov
Copy link

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.

@seansd-zz
Copy link
Collaborator

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

@nbeloglazov
Copy link

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.

@seansd-zz
Copy link
Collaborator

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. . .

@nbeloglazov
Copy link

nbeloglazov commented Nov 7, 2017

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.

@tiberiu-sevio
Copy link

+1

@jayson-yu
Copy link

+1

Timeline?

@plo-adacado
Copy link

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

@seansd-zz
Copy link
Collaborator

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. .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants