Bug 20879

Created attachment 23473 [details] proposed patch This doesn't include non-Mac project file changes yet.

> return new (exec) JSMessageChannelConstructor(exec, impl()->frame()); // FIXME: is this the right browsing context for MessageChannel? That looks right to me. > JSMessageChannelConstructor(ExecState* exec, Frame* browsingContext) I'm not sure we want to hold on to the browsingContext as a Frame. I think we'd be better holding on to the DOMWindow. The difference is that Frame is reused after navigation but DOMWindow is not. If we hold on to the context as a Frame, the objects will magically change origins after navigation. It looks like you only ever user the browsingContext to get at the DOMWindow anyway, so this should be an easy change. We should also add a LayoutTest for this case. Looks like we don't test this case for regular postMessage, but we should. :) > Frame* browsingContext = asJSDOMWindow(exec->dynamicGlobalObject())->impl()->frame(); // FIXME: is this the right browsing context? We should use the lexicalGlobalObject. We're in the process of cleaning this up in TOT, so it's a bit confusing at the moment, but lexical is almost always the right global object. > String sourceOrigin = m_ownerContext->document()->securityOrigin()->toString(); Don't we need to null check document()? This might have changed, but I think you can have a Frame without a Document. Also, maybe I missed it, but how to you keep the MessagePorts, etc, from holding on to pointers to Frames that have been deallocated? Recall that a document can outlive the frame in which it was created.

(In reply to comment #2) > > JSMessageChannelConstructor(ExecState* exec, Frame* browsingContext) > > I'm not sure we want to hold on to the browsingContext as a Frame. I think > we'd be better holding on to the DOMWindow. The difference is that Frame is > reused after navigation but DOMWindow is not. If we hold on to the context as > a Frame, the objects will magically change origins after navigation. It looks > like you only ever user the browsingContext to get at the DOMWindow anyway, so > this should be an easy change. A browsing context as defined in HTML5 is a Frame, and there will be another variant for worker threads. A worker thread has no DOMWindow, so I tried to stick closer to the spec here (I think that I have the navigation case handled). > We should also add a LayoutTest for this case. Agreed, will do. > Don't we need to null check document()? This might have changed, but I think > you can have a Frame without a Document. I don't know how to tell for sure, but the word of mouth on IRC was that this can no longer happen. > Also, maybe I missed it, but how to you keep the MessagePorts, etc, from > holding on to pointers to Frames that have been deallocated? Recall that a > document can outlive the frame in which it was created. I didn't know this can happen. I think that making MessagePort::m_ownerContext a RefPtr should work - how can I test this condition?

(In reply to comment #3) > A browsing context as defined in HTML5 is a Frame, and there will be another > variant for worker threads. A worker thread has no DOMWindow, so I tried to > stick closer to the spec here (I think that I have the navigation case > handled). The HTML 5 spec is confused on this point. I had some correspondence with Ian, and I think his plan is to prevent a closure from running if it isn't associated with the active document in its frame. We don't have this behavior yet, so we'll need to make sure we handle the post-navigation cases correctly. Will worker threads have Frames? Maybe the long term solution is to create a BrowsingContext class and have DOMWindow inherit from that? > > We should also add a LayoutTest for this case. > > Agreed, will do. There are two cases here: 1) A parent frame grabs a closure from a child frame, navigates the child, waits for the navigation to finish, and then calls the closure (which uses/creates a channel). 2) A frame uses/creates a channel, synchronously navigates to a new security origin (see LayoutTests/http/tests/security/xss-DENIED-synchronous-form.html for an example of how to do this), and then uses/creates another channel. In both cases, we should also test the situation in which the channel is created before the navigation and used after the navigation. > > Also, maybe I missed it, but how to you keep the MessagePorts, etc, from > > holding on to pointers to Frames that have been deallocated? Recall that a > > document can outlive the frame in which it was created. > > I didn't know this can happen. I think that making MessagePort::m_ownerContext > a RefPtr should work - how can I test this condition? You can get the Document to outlive the Frame by grabbing a JavaScript pointer to the Document and then removing the Frame from the DOM. For example, something like this should work: <script> var doc; window.onload = function () { doc = frames[0].document; var holder = document.getElementById('frameHolder'); holder.removeChild(holder.firstChild); } </script> <div id="frameHolder"> <iframe src="..."></iframe> </div> If that doesn't work, you can try creating a popup with window.open(), grabbing a pointer to its document, and closing the window.

(In reply to comment #4) > The HTML 5 spec is confused on this point. I had some correspondence with Ian, > and I think his plan is to prevent a closure from running if it isn't > associated with the active document in its frame. We don't have this behavior > yet, so we'll need to make sure we handle the post-navigation cases correctly. Messages to MessagePorts that are not associated with documents that are not fully active shouldn't be dispatched with the current code, because MesagePortTimer only dispatches messages for the active document in the frame, and only if the frame has a page, see MessagePortTimer::fired(). > Will worker threads have Frames? Maybe the long term solution is to create a > BrowsingContext class and have DOMWindow inherit from that? Worker threads won't have frames, so my plan was to create a BrowsingContext class and make Frame inherit from that. Memeber variable names in the patch are already chosen with this in mind (Frame* m_ownerContext) and such.

> Messages to MessagePorts that are not associated with documents that are not > fully active shouldn't be dispatched with the current code, because > MesagePortTimer only dispatches messages for the active document in the frame, > and only if the frame has a page, see MessagePortTimer::fired(). Ok. The easiest thing is probably to leave it as a Frame for now. If we can find test cases where it does the wrong thing, we can change it later. > Worker threads won't have frames, so my plan was to create a BrowsingContext > class and make Frame inherit from that. Memeber variable names in the patch are > already chosen with this in mind (Frame* m_ownerContext) and such. That makes sense.

Created attachment 23573 [details] updated patch > We should use the lexicalGlobalObject. Fixed. > Also, maybe I missed it, but how to you keep the MessagePorts, etc, from > holding on to pointers to Frames that have been deallocated? Fixed this (it did crash), and several more instances of hanging references. Added a test case. (In reply to comment #4) > 1) A parent frame grabs a closure from a child frame, navigates the child, > waits for the navigation to finish, and then calls the closure (which > uses/creates a channel). Currently, messages sent through the created port have the new origin. More precisely, Subframe: window.createClosure = function() { var MessageChannelConstructor = window.MessageChannel; return function() { var channel = new MessageChannelConstructor; // can't use window.MessageChannel channel.port1.postMessage("ping"); return channel.port2; } } Main frame: var closure = window.frames[0].createClosure(); ... navigate subframe from 127.0.0.1 to localhost var mainPort = closure(); mainPort.onmessage = function(evt) { alert(evt.origin); // http://localhost:8000 } mainPort.start(); Is this correct behavior? > 2) A frame uses/creates a channel, synchronously navigates to a new security > origin (see LayoutTests/http/tests/security/xss-DENIED-synchronous-form.html > for an example of how to do this), and then uses/creates another channel. I'm not sure what this means - what kinds of issues I can expect if a new channel is created? > In both cases, we should also test the situation in which the channel is > created before the navigation and used after the navigation. Noted, but haven't tried this yet, as I'm also not sure about expected behavior.

(In reply to comment #7) > (In reply to comment #4) > > 1) A parent frame grabs a closure from a child frame, navigates the child, > > waits for the navigation to finish, and then calls the closure (which > > uses/creates a channel). > > Currently, messages sent through the created port have the new origin. > [snip] > Is this correct behavior? This isn't the behavior we want because it lets content from one origin impersonate another origin by spoofing the "origin" property. If we hold on to the DOMWindow instead of the Frame, the origin property shouldn't be spoofable. > > 2) A frame uses/creates a channel, synchronously navigates to a new security > > origin (see LayoutTests/http/tests/security/xss-DENIED-synchronous-form.html > > for an example of how to do this), and then uses/creates another channel. > > I'm not sure what this means - what kinds of issues I can expect if a new > channel is created? What this test is supposed to get at is whether the "origin" property is determined at the time the message/channel is created or at the time when the message/channel is used. The description probably isn't that clear. We'll probably have to use the channel (as well as create it) to see interesting behavior. > > In both cases, we should also test the situation in which the channel is > > created before the navigation and used after the navigation. > > Noted, but haven't tried this yet, as I'm also not sure about expected > behavior. The behavior we're trying to avoid is letting an malicious web site send a message that identifies itself as originating from an honest web site.

Comment on attachment 23573 [details] updated patch Thanks, I think I finally understood the scenario.

...not quite. What I did understand was that code loaded from one origin was dispatching an event with another origin. However, it only happens when the other side tries really hard to achieve that - and I don't see how to send a spoofed message to 3rd party. What am I missing? More importantly, Hixie has just pointed it out to me that MessagePort messages are supposed to have an empty origin: "Unless otherwise specified, when the user agent creates and dispatches a message event in the algorithms described in the following sections, ... the origin attribute must be the empty string ..."

(In reply to comment #10) > What I did understand was that code loaded from one origin was dispatching an > event with another origin. However, it only happens when the other side tries > really hard to achieve that - and I don't see how to send a spoofed message to > 3rd party. What am I missing? I'm confident that an attacker can make this happen reliably. The point is you're grabbing the security context from an object that the attacker can make have whatever security context he likes. > More importantly, Hixie has just pointed it out to me that MessagePort messages > are supposed to have an empty origin: Ah, ok. Then maybe this doesn't matter. What else is the Frame* used for? Do we get in trouble if the pointer points to a Frame with an unexpected Document? I guess I don't understand why we don't want to use DOMWindow (or a new superclass of DOMWindow) to hold onto the context. It seems like this avoids the complexity of having to reason about what happens during navigation.

For example, JSMessagePort::addEventListener looks exploitable. This function lets the attacker compile his script in toJSDOMWindow(impl()->ownerContext()), which can be the DOMWindow of any SecurityOrigin the attacker chooses.

(In reply to comment #11) > What else is the Frame* used for? Do > we get in trouble if the pointer points to a Frame with an unexpected Document? The current spec doesn't mention a lot of uses for it - it is only used in a very roundabout way to check that messages can be delivered (the owner should be "available"), and whether to unentangle the ports when a document is discarded. Its use in JSMessagePort::addEventListener is implicit (all the spec says is that the object should implement EventTarget, not how it should be implemented). > I guess I don't understand why we don't want to use DOMWindow (or a new > superclass of DOMWindow) to hold onto the context. As far as I'm concerned, I was trying to keep code close to what the spec says to make everyone's life easier. You've convinced me that this wasn't such a great idea in this case. Holding a pointer to DOMWindow is basically the same as holding one to Document, correct? I think most existing code uses a Document pointer.

> Holding a pointer to DOMWindow is basically the same as holding one to > Document, correct? I think most existing code uses a Document pointer Yeah, I think that works out to be the same.

Created attachment 23654 [details] updated patch

This patch looks a lot better w.r.t. the issues above. I don't have reviewer status, so someone else will have to give you the r+. :)

Comment on attachment 23654 [details] updated patch >+/* Hash table for constructor */ >+ >+static const HashTableValue JSMessageChannelConstructorTableValues[1] = >+{ >+ { 0, 0, 0, 0 } >+}; >+ >+static const HashTable JSMessageChannelConstructorTable = { 0, JSMessageChannelConstructorTableValues, 0 }; >+ >+const ClassInfo JSMessageChannelConstructor::s_info = { "MessageChannelConstructor", 0, &JSMessageChannelConstructorTable, 0 }; I don't think you need a table at all. >+ Document* m_document; This should be private. >+void Document::dispatchMessagePortEvents() >+{ >+ RefPtr<Document> protect(this); >+ HashSet<MessagePort*> ports = m_messagePorts; // Make a frozen copy. Can you make the frozen copy by copying to a Vector? >+void Document::createdMessagePort(MessagePort* port) >+{ This should assert that the MessagePort is not null. >+void Document::destroyedMessagePort(MessagePort* port) >+{ As should this. >+ MessagePort* port1() { return m_port1.get(); } >+ MessagePort* port2() { return m_port2.get(); } These should be const. >+class CloseMessagePortTimer : public TimerBase { >+public: >+ CloseMessagePortTimer(PassRefPtr<MessagePort> port) >+ : m_port(port) >+ { This should assert that the port is not null. >+ virtual void fired() >+ { >+ ASSERT(!m_port->active()); >+ >+ // Closing may destroy the port, dispatch any remaining messages now. >+ if (m_port->queueIsOpen()) >+ m_port->dispatchMessages(); >+ >+ RefPtr<Event> evt = Event::create(EventNames::closeEvent, false, true); >+ if (m_port->onCloseListener()) { >+ evt->setTarget(m_port.get()); >+ evt->setCurrentTarget(m_port.get()); >+ m_port->onCloseListener()->handleEvent(evt.get(), false); >+ } >+ >+ ExceptionCode ec = 0; >+ m_port->dispatchEvent(evt.release(), ec, false); >+ ASSERT(!ec); >+ >+ delete this; I think this would be cleaner if the all this work was done in a member function of MessagePort, which this fired method calls. >+ m_entangledPort->m_messageQueue.append(MessageEvent::create(message, "", "", m_createdWithDocument->domWindow(), newMessagePort.get())); Why is the origin the empty string? >+PassRefPtr<MessagePort> MessagePort::startConversation(Document* scriptContext, const String& message) Perhaps the variable should be named scriptContextDocument? >+ m_entangledPort->m_messageQueue.append(MessageEvent::create(message, "", "", m_createdWithDocument->domWindow(), port2.get())); Why is the origin the empty string? >+ MessagePort* otherPort = m_entangledPort; otherPort is not a great name for this. >+void MessagePort::dispatchMessages() >+{ >+ // Messages for documents that are not fully active get dispatched too, but JSAbstractEventListener::handleEvent() doesn't call handlers for these. >+ ASSERT(queueIsOpen()); Is it possible for an event dispatch to cause a MessagePort to go away? Should it protect itself against it here? >+ Document* createdWithDocument() { return m_createdWithDocument; } This sounds like it would return a boolean. Perhaps it should be called something like, documentCreatedWith, associatedDocument, maybe even just document(). r=me and Anders!

Committed <http://trac.webkit.org/changeset/36891>, making all suggested changes, except as specifically mentioned below. > I think this would be cleaner if the all this work was done in a member > function of MessagePort, which this fired method calls. Moved most (but not all) of it to a member function. > Why is the origin the empty string? That's what HTML5 says, see comment 10. My understanding is that you don't need one, as you explicitly create a channel to a known party. > otherPort is not a great name for this. I think it is the right name to use, given that it is used in HTML5 when describing the algorithm. > Is it possible for an event dispatch to cause a MessagePort to go away? I don't think it is - prior dispatching the event to the first listener, its target is set to the port, and Event retains its targets. > >+ Document* createdWithDocument() { return m_createdWithDocument; } > This sounds like it would return a boolean. Perhaps it should be called > something like, documentCreatedWith, associatedDocument, maybe even just > document(). Then document() it is. If this becomes an issue when refactoring to support non-Document owners, we can rename it again. Thank you for the review!