RESOLVED FIXED 94171
Allow blocking of IndexedDB in third-party contexts
https://bugs.webkit.org/show_bug.cgi?id=94171
Summary Allow blocking of IndexedDB in third-party contexts
Vicki Pfau
Reported 2012-08-15 19:19:22 PDT
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.
Attachments
Patch (20.61 KB, patch)
2013-01-28 13:59 PST, Mike West
no flags
Patch (24.56 KB, patch)
2013-01-28 15:00 PST, Mike West
no flags
Patch (26.27 KB, patch)
2013-01-29 02:24 PST, Mike West
no flags
Patch for landing. (26.26 KB, patch)
2013-01-31 02:16 PST, Mike West
no flags
Joshua Bell
Comment 1 2012-08-29 15:47:38 PDT
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?
Adam Barth
Comment 2 2012-08-29 15:56:46 PDT
I think jpfau filed another bug on this topic, which should probably be duped one way or another.
Adam Barth
Comment 3 2012-08-29 15:57:06 PDT
Oh, maybe this is same one. :)
Vicki Pfau
Comment 4 2012-08-29 17:18:01 PDT
(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.
Mike West
Comment 5 2013-01-28 13:59:59 PST
Mike West
Comment 6 2013-01-28 14:11:45 PST
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.
Mike West
Comment 7 2013-01-28 14:15:40 PST
(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.
Joshua Bell
Comment 8 2013-01-28 14:17:17 PST
(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
Joshua Bell
Comment 9 2013-01-28 14:18:18 PST
Thanks for tackling this!
jochen
Comment 10 2013-01-28 14:35:20 PST
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?)
Mike West
Comment 11 2013-01-28 15:00:05 PST
Mike West
Comment 12 2013-01-28 15:08:18 PST
(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.
Joshua Bell
Comment 13 2013-01-28 15:10:46 PST
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)
Joshua Bell
Comment 14 2013-01-28 15:12:50 PST
(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?
Mike West
Comment 15 2013-01-28 15:16:46 PST
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. :)
Joshua Bell
Comment 16 2013-01-28 16:37:15 PST
(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
Mike West
Comment 17 2013-01-29 02:24:01 PST
Mike West
Comment 18 2013-01-29 02:25:30 PST
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. :)
Mike West
Comment 19 2013-01-29 02:29:51 PST
(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. :)
Mike West
Comment 20 2013-01-30 01:14:08 PST
Comment on attachment 185204 [details] Patch Ugh. Thought I set this earlier. Would one of you fine folks mind reviewing this patch?
jochen
Comment 21 2013-01-30 01:57:15 PST
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)
Mike West
Comment 22 2013-01-30 02:11:52 PST
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).
Adam Barth
Comment 23 2013-01-30 13:22:59 PST
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
Adam Barth
Comment 24 2013-01-30 13:24:19 PST
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.
jochen
Comment 25 2013-01-31 01:16:51 PST
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
Mike West
Comment 26 2013-01-31 02:16:58 PST
Created attachment 185723 [details] Patch for landing.
WebKit Review Bot
Comment 27 2013-01-31 06:59:29 PST
Comment on attachment 185723 [details] Patch for landing. Clearing flags on attachment: 185723 Committed r141418: <http://trac.webkit.org/changeset/141418>
WebKit Review Bot
Comment 28 2013-01-31 06:59:36 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.