Bug 94888 – Make plugins respect third-party storage blocking setting

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Vicki Pfau]

Plugins are a common vector for storing tracking data about a user. As we are implementing a setting for disallowing pages in a third-party context from accessing storage, plugins should be subject to the same restriction.

<rdar://problem/8830539>

[Vicki Pfau]

Created attachment 160307 [details]
Patch

[Vicki Pfau]

This patch works by forcing the plugin into private browsing mode when it's in a third-party context. I'm a bit concerned that it might not work exactly as desired because I'm not sure if it correctly handles private browsing mode changing.

The patch only applies for WK2 currently, as WebKit 1 doesn't have an API for this currently.

[Adam Barth]

I don't know how private browsing mode works for plugins, but does this create a leak between normal and private modes?  For example, if I wanted to learn what someone was up to in private browsing mode, could I put a plugin in a third-party context and then get access to that information?

[Vicki Pfau]

(In reply to comment #3)
> I don't know how private browsing mode works for plugins, but does this create a leak between normal and private modes?  For example, if I wanted to learn what someone was up to in private browsing mode, could I put a plugin in a third-party context and then get access to that information?

I talked to people who know more than me about how plugins work, and they say that this shouldn't be a concern. The private browsing context is only for the plugin and not the whole document, so document/page acts as if it's a normal (but third-party) context, and the plugin just can't read/write LSOs.

[Adam Barth]

Comment on attachment 160307 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160307&action=review

> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1265
> +    if (frame()->document() && !frame()->document()->securityOrigin()->canAccessPluginStorage(frame()->document()->topDocument()->securityOrigin()))

How can frame()->document() be 0?  That shouldn't be possible.

frame()->document()->topDocument() seems like needless indirection.  You're going to the Document, which just goes back to the Frame.  Why not just frame()->tree()->top()->document() ?

[Adam Barth]

This patch is fine from my PoV, but I don't really know how isPrivateBrowsingEnabled is it's probably better if someone who knows about plug-ins does the official review.

[Vicki Pfau]

(In reply to comment #5)
> (From update of attachment 160307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160307&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/PluginView.cpp:1265
> > +    if (frame()->document() && !frame()->document()->securityOrigin()->canAccessPluginStorage(frame()->document()->topDocument()->securityOrigin()))
> 
> How can frame()->document() be 0?  That shouldn't be possible.

I didn't know if it was possible, so I added that check to be safe (I thought it was, but I guess not).

> frame()->document()->topDocument() seems like needless indirection.  You're going to the Document, which just goes back to the Frame.  Why not just frame()->tree()->top()->document() ?

I also didn't realize that you could just do this. I've been using topDocument() in all other locations, so I just did that here too. That might work better.

[Adam Barth]

> I didn't know if it was possible, so I added that check to be safe (I thought it was, but I guess not).

It used to be possible, but it's not possible anymore.  Most code that null checks the document is doing so needlessly.

> > frame()->document()->topDocument() seems like needless indirection.  You're going to the Document, which just goes back to the Frame.  Why not just frame()->tree()->top()->document() ?
> 
> I also didn't realize that you could just do this. I've been using topDocument() in all other locations, so I just did that here too. That might work better.

topDocument() is fine when you're working with a Document as your context object.  If you've already got a Frame, there's no reason to traverse to the Document just to traverse back to the Frame.

[Vicki Pfau]

Created attachment 161382 [details]
Patch

[Brady Eidson]

Comment on attachment 161382 [details]
Patch

Please make sure to file a bug for the followup task of dynamically updating all interested parties when the state changes, as we discussed.

[Vicki Pfau]

Committed r127513: <http://trac.webkit.org/changeset/127513>