Closed Bug 837114 Opened 7 years ago Closed 7 years ago

Flash Shared objects leak state from Private Browsing mode

Categories

(Firefox for Android Graveyard :: Plugins, defect)

19 Branch
ARM
Android
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 22

People

(Reporter: mgoodwin, Assigned: snorp)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Mark, who would be a good person to look at this?
Flags: needinfo?(mark.finkle)
Not sure really. How does desktop fix this issue?

CC'ing Brian and Snorp
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(benjamin)
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
Flags: needinfo?(benjamin)
My guess is that the plugin just doesn't query for it.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
The stock browser works as expected here; private browsing mode can't see stored objects from a regular session (and v.v.).
(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?
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.
(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.
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;
    }
Attachment #717918 - Flags: review?(benjamin)
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.
Attachment #717918 - Flags: review?(benjamin) → review-
(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.
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.
Attachment #717918 - Attachment is obsolete: true
Attachment #719009 - Flags: review?(benjamin)
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
Attachment #719009 - Flags: review?(benjamin) → review+
Final patch with GetPrivacyFromNPP removed
Attachment #719009 - Attachment is obsolete: true
Attachment #720790 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8cb5ad877aee
Assignee: nobody → snorp
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Keywords: verifyme
OS: Mac OS X → Android
Hardware: x86 → ARM
Keywords: verifyme
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.