Bug #94057 adds a parameter to SecurityOrigin::canAccessDatabase to enable the user to choose to block third-party contexts from accessing storage databases. This parameter needs to be passed in the IndexedDB code.
Looks like this could be done in DOMWindowIndexedDatabase::webkitIndexedDB and WorkerContextIndexedDatabase::webkitIndexedDB to forbid access to the IDBFactory itself. (Alternately, the checks could be added to IDBFactory.cpp's isContextValid(ScriptContext*) function which all IDBFactory methods call, but that doesn't seem to be the pattern used elsewhere.) jeffrey@ - were you planning to take this on?
I think jpfau filed another bug on this topic, which should probably be duped one way or another.
Oh, maybe this is same one. :)
(In reply to comment #1) > Looks like this could be done in DOMWindowIndexedDatabase::webkitIndexedDB and WorkerContextIndexedDatabase::webkitIndexedDB to forbid access to the IDBFactory itself. > > (Alternately, the checks could be added to IDBFactory.cpp's isContextValid(ScriptContext*) function which all IDBFactory methods call, but that doesn't seem to be the pattern used elsewhere.) > > jeffrey@ - were you planning to take this on? I haven't taken a deep look at what the spec says about how errors should be handled, but it seems like the best way to go about this is to return an error when attempting to open a database. It's probably also a good idea to check per call (as it looks like the spec might allow this)--this isn't done in other places because in many cases the spec does not give allowances that would let us do this. I was also not planning on implementing this soon. It looks like work on IndexedDB is currently very active, so it might be better for someone more involved in that work to do this.
Created attachment 185056 [details] Patch
I've taken a quick stab at implementing this for IndexedDB. It seems to work correctly, modulo the exception type that's thrown. Actually, now that I look at it again, this is likely wrong; the check probably needs to be in IDBFactory::open. Hrm. +jochen, since he likes blocking third-party content. Seriously. I think he'd block first-part content too if he could get away with it.
(In reply to comment #6) > I've taken a quick stab at implementing this for IndexedDB. It seems to work correctly, modulo the exception type that's thrown. Actually, now that I look at it again, this is likely wrong; the check probably needs to be in IDBFactory::open. Hrm. Yeah, this patch is wrong. It's blocking creation of `window.webkitIndexedDB` entirely. Ugh.
(In reply to comment #6) > I've taken a quick stab at implementing this for IndexedDB. It seems to work correctly, modulo the exception type that's thrown. Noticed that - looks like it's returning null from self.indexedDB and the TypeError arises from calling indexedDB.open(...) ? > Actually, now that I look at it again, this is likely wrong; the check probably needs to be in IDBFactory::open. Hrm. Yeah, that'd make returning a SecurityError easier. If implemented in ::open(), should be done in deleteDatabase() and getDatabaseNames() too. Another place this could conceivably live is in Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp's IDBFactoryBackendProxy::allowIndexedDB
Thanks for tackling this!
In chromium, we do the checks in IDBFactory.getDatabaseNames, IDBFactory.open, and IDBFactory.deleteDatabase For workers, we don't do third-party access checks (what's the first party URL of a shared worker anyway?)
Created attachment 185076 [details] Patch
(In reply to comment #10) > In chromium, we do the checks in IDBFactory.getDatabaseNames, IDBFactory.open, and IDBFactory.deleteDatabase The current patch moves the check to these methods, and seems to work correctly for .open(). I'll add moar tests for the other two methods sometime tomorrow. Depending on how much work it is to pipe an exception-throwing-mechanism down to getDatabaseNames, I might or might not want to split that out into a separate patch. WDYT? > For workers, we don't do third-party access checks Not even for regular ol' workers? > (what's the first party URL of a shared worker anyway?) According to this patch, it's the SecurityOrigin of the ScriptExecutionContext that's passed in when it's created. Shared workers don't have "top" origins at the moment, though, which would certainly cause problems.
Comment on attachment 185076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185076&action=review IDB changes LGTM with one nit > Source/WebCore/Modules/indexeddb/IDBFactory.cpp:104 > + return 0; Why no ec = SECURITY_ERR here? > LayoutTests/http/tests/security/cross-origin-worker-indexeddb-expected.txt:8 > +self.webkitIndexedDB.open() threw an exception: SecurityError tests for deleteDatabase() and webkitGetDatabaseNames() would be nice (as mentioned in a comment)
(In reply to comment #13) > (From update of attachment 185076 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185076&action=review > > IDB changes LGTM with one nit > > > Source/WebCore/Modules/indexeddb/IDBFactory.cpp:104 > > + return 0; > > Why no ec = SECURITY_ERR here Ah, no raises (DOMException) in the IDL. Want to go ahead an add that and update the method signature?
Comment on attachment 185076 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185076&action=review Thanks for taking a look! >>> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:104 >>> + return 0; >> >> Why no ec = SECURITY_ERR here? > > Ah, no raises (DOMException) in the IDL. Want to go ahead an add that and update the method signature? It's a bit tangential to this patch. I think I'd prefer to split it out into a separate patch and land that first. I'll spin something up tomorrow. >> LayoutTests/http/tests/security/cross-origin-worker-indexeddb-expected.txt:8 >> +self.webkitIndexedDB.open() threw an exception: SecurityError > > tests for deleteDatabase() and webkitGetDatabaseNames() would be nice (as mentioned in a comment) Yes. MOAR TESTS. Tomorrow. :)
(In reply to comment #15) > > Ah, no raises (DOMException) in the IDL. Want to go ahead an add that and update the method signature? > > It's a bit tangential to this patch. I think I'd prefer to split it out into a separate patch and land that first. I'll spin something up tomorrow. SGTM
Created attachment 185204 [details] Patch
The current patch adds tests on top of the patch from yesterday, and relies upon http://wkbug.com/108154 to add exception-throwing support to getDatabaseNames. I'm not going to throw it to the bots until that patch lands, but I'd appreciate you folks taking a look at your leisure. :)
(In reply to comment #18) > The current patch adds tests on top of the patch from yesterday, and relies upon http://wkbug.com/108154 to add exception-throwing support to getDatabaseNames. I'm not going to throw it to the bots until that patch lands, but I'd appreciate you folks taking a look at your leisure. :) Also: Jochen mentioned that this diverges from Chrome's current behavior, as it doesn't currently throw exceptions. I'll figure out how to change that: throwing exceptions seems like the right thing to do. Jochen: is it worth trying to stuff that into this patch as well? I haven't looked at the code yet, so I'm not sure what the complexity level is. :)
Comment on attachment 185204 [details] Patch Ugh. Thought I set this earlier. Would one of you fine folks mind reviewing this patch?
Comment on attachment 185204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185204&action=review > Source/WebCore/dom/ScriptExecutionContext.h:92 > + virtual const SecurityOrigin* topOrigin() const = 0; Adam, can you have a look at this? I also wonder whether the existing code e.g. DOMWindow::localStorage should use this method as well? > Source/WebCore/workers/WorkerContext.h:141 > + virtual const SecurityOrigin* topOrigin() const OVERRIDE { return m_topOrigin.get(); } What is topOrigin() I guess for workers that's the topOrigin of the frame that created the worker? What about shared workers? In the chromium port, we treat them as first parties (see ChromeContentBrowserClient::AllowWorkerIndexedDB)
Comment on attachment 185204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185204&action=review >> Source/WebCore/dom/ScriptExecutionContext.h:92 >> + virtual const SecurityOrigin* topOrigin() const = 0; > > Adam, can you have a look at this? > > I also wonder whether the existing code e.g. DOMWindow::localStorage should use this method as well? I do think we should change the existing code to use this method; I didn't want to do that in the same patch, but I'd certainly like to follow this up with changes for other places that are pulling 'frame()->document()->topDocument()->securityOrigin()'. >> Source/WebCore/workers/WorkerContext.h:141 >> + virtual const SecurityOrigin* topOrigin() const OVERRIDE { return m_topOrigin.get(); } > > What is topOrigin() I guess for workers that's the topOrigin of the frame that created the worker? > > What about shared workers? > > In the chromium port, we treat them as first parties (see ChromeContentBrowserClient::AllowWorkerIndexedDB) There's a FIXME in SecurityOrigin::canAccessDatabase that implies that there should always be a topOrigin. For the moment, however, that's not the case for shared workers, which pass in 0 as a topOrigin in their constructor. For normal workers, I think you're correct that the top origin passed in when creating the worker thread is the top-level origin of the document that created the worker: see WorkerMessagingProxy::startWorkerContext (Source/WebCore/workers/WorkerMessagingProxy.cpp:285).
Comment on attachment 185204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185204&action=review >>> Source/WebCore/dom/ScriptExecutionContext.h:92 >>> + virtual const SecurityOrigin* topOrigin() const = 0; >> >> Adam, can you have a look at this? >> >> I also wonder whether the existing code e.g. DOMWindow::localStorage should use this method as well? > > I do think we should change the existing code to use this method; I didn't want to do that in the same patch, but I'd certainly like to follow this up with changes for other places that are pulling 'frame()->document()->topDocument()->securityOrigin()'. +1
It would be good to align our notion of "top origin" and "first party" for workers. It's not clear to me what we should do for shared workers. For dedicated workers, I think it makes sense to take the top origin of the frame that created them because they're most similar to invisible iframes in that page.
Comment on attachment 185204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185204&action=review r=me > LayoutTests/http/tests/security/cross-origin-worker-indexeddb.html:1 > +<html> missing DOCTYPE
Created attachment 185723 [details] Patch for landing.
Comment on attachment 185723 [details] Patch for landing. Clearing flags on attachment: 185723 Committed r141418: <http://trac.webkit.org/changeset/141418>
All reviewed patches have been landed. Closing bug.