837114 - Flash Shared objects leak state from Private Browsing mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Plugins, defect)

Description

[Mark Goodwin [:mgoodwin]]

Issue:
Flash shared objects written in Private Browsing mode are readable from non private tabs (and v.v.)

STR:
1) Create / switch to a Private Browsing tab
2) Visit a page that writes a flash LSO that you're able to read later; e.g. http://www.permadi.com/tutorial/flashSharedObject/
3) close PB tab
4) revisit page and read LSO value

Alternatively, visit any site which writes LSO in PB mode then get an ADB shell and poke around in /data/data/org.mozilla.fennec/app_plugins/com.adobe.flashplayer/.macromedia/Flash_Player/#SharedObjects to verify data has been written

Comment 1

[Mark Goodwin [:mgoodwin]]

Mark, who would be a good person to look at this?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Not sure really. How does desktop fix this issue?

CC'ing Brian and Snorp

Comment 3

[Kevin Brosnan [Ex-Mozilla]]

It was a problem on desktop pre Flash 10.3 http://helpx.adobe.com/flash-player/release-note/release-notes-flash-player-10-3.html#main_lso

Maybe BSmedberg knows what was done.

Comment 4

[Benjamin Smedberg]

Are we reporting the correct private-browsing state to the plugin? i.e. is the plugin querying for NPNVprivateModeBool and are we responding with the correct value? We certainly have code in the plugin host to provide the correct answer: 

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#667
and
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#3442

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

My guess is that the plugin just doesn't query for it.

Comment 6

[Mark Goodwin [:mgoodwin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> My guess is that the plugin just doesn't query for it.

That's possible, I guess. I'll test to see how Chrome and the stock browser handle this case on Monday. If they behave the same way, that's maybe an indication the android flash plugin is not private mode aware.

Comment 7

[Mark Goodwin [:mgoodwin]]

The stock browser works as expected here; private browsing mode can't see stored objects from a regular session (and v.v.).

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Mark Goodwin [:mgoodwin] from comment #7)
> The stock browser works as expected here; private browsing mode can't see
> stored objects from a regular session (and v.v.).

We probably need to return a different directory to the plugin when we are in private mode. e.g., /data/data/org.mozilla.fennec/app_plugins_private/com.adobe.flashplayer or something. Then delete the directory at some point?

Comment 9

[Benjamin Smedberg]

Has anyone *tested* to see whether http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#2077 or http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPluginInstance.cpp#1390 are being called? Let's not try working around the issue by passing other directories until we're sure that the normal protocol isn't working. Flash has a shared codebase, so I really suspect that it would work if we were giving it the proper values.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Has anyone *tested* to see whether
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.
> cpp#2077 or
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/
> nsNPAPIPluginInstance.cpp#1390 are being called? Let's not try working
> around the issue by passing other directories until we're sure that the
> normal protocol isn't working. Flash has a shared codebase, so I really
> suspect that it would work if we were giving it the proper values.

I tested just now and neither of those are called.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

The Android plugin code does this:

    WebCore::Settings* settings = pluginWidget->webViewCore()->mainFrame()->settings();
    if (settings && settings->privateBrowsingEnabled()) {
        // if this is an incognito view then check the path to see if it exists
        // and if it is a directory, otherwise if it does not exist create it.
        struct stat st;
        if (stat(gApplicationDataDirIncognito, &st) == 0) {
            if (!S_ISDIR(st.st_mode)) {
                return NULL;
            }
        } else {
            if (mkdir(gApplicationDataDirIncognito, S_IRWXU) != 0) {
                return NULL;
            }
        }

        return gApplicationDataDirIncognito;
    }

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 837114 - Don't leak Flash shared objects while private browsing on Android

Comment 13

[Benjamin Smedberg]

Comment on attachment 717918 [details] [diff] [review]
Bug 837114 - Don't leak Flash shared objects while private browsing on Android

It looks like you just duplicated the logic GetPrivacyFromNPP. Those really need to be one function, and unify the existing callers.

Why is the mPrivateMode member necessary? Are you worried about the private mode state changing while the instance is loaded? Or are you expecting IsPrivateBrowsing() to be called a lot? It feels weird to cache the result.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> Comment on attachment 717918 [details] [diff] [review]
> Bug 837114 - Don't leak Flash shared objects while private browsing on
> Android
> 
> It looks like you just duplicated the logic GetPrivacyFromNPP. Those really
> need to be one function, and unify the existing callers.

Hmm, alright. How does just making it public in nsNPAPIPlugin sound?

> 
> Why is the mPrivateMode member necessary? Are you worried about the private
> mode state changing while the instance is loaded? Or are you expecting
> IsPrivateBrowsing() to be called a lot? It feels weird to cache the result.

Yeah, it gets called a lot. Not like hundreds of times a second or anything, but pretty frequently. On the order of a dozen times per instance or so.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ugh, not sure what I am thinking. I'm going to remove the caching and and just add a GetPrivateBrowsing() or something to nsNPAPIPluginInstance. GetPrivacyFromNPP will then just use that.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 837114 - Don't leak Flash shared objects while private browsing on Android

Comment 17

[Benjamin Smedberg]

Comment on attachment 719009 [details] [diff] [review]
Bug 837114 - Don't leak Flash shared objects while private browsing on Android

Can you just remove GetPrivacyFromNPP and have the two callers call the method directly? r=me with that

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 837114 - Don't leak Flash shared objects while private browsing on Android r=bsmedberg

Final patch with GetPrivacyFromNPP removed

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb5ad877aee

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cb5ad877aee