Bug 94171 – Allow blocking of IndexedDB in third-party contexts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Vicki Pfau]

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.

[Joshua Bell]

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]

I think jpfau filed another bug on this topic, which should probably be duped one way or another.

[Adam Barth]

Oh, maybe this is same one.  :)

[Vicki Pfau]

(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]

Created attachment 185056 [details]
Patch

[Mike West]

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]

(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]

(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]

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

[Mike West]

Created attachment 185076 [details]
Patch

[Mike West]

(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 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]

(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 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]

(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]

Created attachment 185204 [details]
Patch

[Mike West]

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]

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

[Mike West]

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

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

[Mike West]

Created attachment 185723 [details]
Patch for landing.

[WebKit Review Bot]

Comment on attachment 185723 [details]
Patch for landing.

Clearing flags on attachment: 185723

Committed r141418: <http://trac.webkit.org/changeset/141418>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.