Bug 149014

Summary: Hide all plugin names except Flash, Java, and QuickTime
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebCore Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, bfulgham, buildbot, dbates, oliver, rmondello, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=151782
Bug Depends on:    
Bug Blocks: 153657    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Patch darin: review+

Description Brent Fulgham 2015-09-09 15:45:19 PDT
Browser plugin details accounts for around 18-20 bits of identifying fingerprint information. We should take a page from Mozilla and hide all plugins that are not Shockwave (Flash), QuickTime, or Java.

This one change drops the plugin-based fingerprint bits on my test system from 19.89 bits (only 1 in 970,980 machines match) to 3.31 bits (1 in 10 machines match).
Comment 1 Brent Fulgham 2015-09-09 15:48:53 PDT
This bug attempts to implement the recommendation from the EFF regarding decreasing the attack surface for Fingerprinting (see <https://panopticlick.eff.org/browser-uniqueness.pdf>).

"There are probably stronger ease-of-development arguments for making plu- gins enumeratable, but the example of IE shows that it is not strictly necessary. We recommend that browsers switch to confirm-only testing for fonts and plu- gins, with an exponential backoff to prevent exhaustive searches by malicious javascript."
Comment 2 Brent Fulgham 2015-09-09 15:49:24 PDT
See <https://panopticlick.eff.org> for some simple fingerprinting measurements.
Comment 3 Radar WebKit Bug Importer 2015-09-09 15:58:24 PDT
<rdar://problem/22635015>
Comment 4 Brent Fulgham 2015-09-09 16:01:11 PDT
Created attachment 260885 [details]
Patch
Comment 5 Brent Fulgham 2015-09-09 16:29:24 PDT
<rdar://problem/20042582>
Comment 6 Build Bot 2015-09-09 16:30:44 PDT
Comment on attachment 260885 [details]
Patch

Attachment 260885 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/155532

New failing tests:
plugins/plugin-javascript-access.html
plugins/navigator-mimeTypes-length.html
Comment 7 Build Bot 2015-09-09 16:30:48 PDT
Created attachment 260887 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 8 Build Bot 2015-09-09 16:43:27 PDT
Comment on attachment 260885 [details]
Patch

Attachment 260885 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/155551

New failing tests:
plugins/plugin-javascript-access.html
plugins/navigator-mimeTypes-length.html
Comment 9 Build Bot 2015-09-09 16:43:30 PDT
Created attachment 260888 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 10 Brent Fulgham 2015-09-09 19:39:20 PDT
These errors are happening because the tests iterate over the installed plugins until they find the desired entry. We should just query for existence, rather than do this iteration.
Comment 11 Brent Fulgham 2015-09-10 09:52:14 PDT
Created attachment 260932 [details]
Patch
Comment 12 Darin Adler 2015-09-11 09:48:40 PDT
Comment on attachment 260932 [details]
Patch

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

> Source/WebCore/plugins/PluginData.cpp:61
> +        if ((plugin.name.findIgnoringCase("Shockwave") == notFound)
> +            && (plugin.name.findIgnoringCase("QuickTime") == notFound)
> +            && (plugin.name.findIgnoringCase("Java") == notFound)) {
> +            continue;

Need a comment explaining the rationale behind this list. Otherwise it’s too mysterious in the future.

I suggest putting this predicate into a separate named function by itself.

The correct function to use is containsIgnoringASCIICase rather than findIgnoringCase != notFound.

    static bool shouldBePubliclyVisible(const PluginInfo& plugin)
    {
        // For practical website compatibility there are a few plug-ins that need to be visible ... <good explanation>
        return plugin.name.containsIgnoringASCIICase("Shockwave") || ...
    }

    ...

        if (shouldBePubliclyVisible(plugin))
            plugins.append(WTF::move(plugin));

> Source/WebCore/plugins/PluginData.cpp:68
> +        return codePointCompare(a.name, b.name) < 0;

Consider using codePointCompareLessThan?

Is there a reason we are sorting in a case sensitive way rather than a case folding way?

> Source/WebCore/plugins/PluginData.h:93
>      Vector<PluginInfo> webVisiblePlugins() const;
> +    Vector<PluginInfo> publiclyVisiblePlugins() const;

Very unclear naming here. How is "web visible" different from "publicly visible". We should use more precise terminology.

> Source/WebCore/testing/Internals.cpp:3095
> +    Document* document = contextDocument();
> +    if (!document)
> +        return;
> +    
> +    if (Page* page = document->page())
> +        page->setShowAllPlugins(show);

Slightly strange combination of idioms. Early return for one null check, but nested if for the other.
Comment 13 Brent Fulgham 2015-09-11 10:24:19 PDT
Comment on attachment 260932 [details]
Patch

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

>> Source/WebCore/plugins/PluginData.cpp:61
>> +            continue;
> 
> Need a comment explaining the rationale behind this list. Otherwise it’s too mysterious in the future.
> 
> I suggest putting this predicate into a separate named function by itself.
> 
> The correct function to use is containsIgnoringASCIICase rather than findIgnoringCase != notFound.
> 
>     static bool shouldBePubliclyVisible(const PluginInfo& plugin)
>     {
>         // For practical website compatibility there are a few plug-ins that need to be visible ... <good explanation>
>         return plugin.name.containsIgnoringASCIICase("Shockwave") || ...
>     }
> 
>     ...
> 
>         if (shouldBePubliclyVisible(plugin))
>             plugins.append(WTF::move(plugin));

Great ideas! I'll incorporate them.

>> Source/WebCore/plugins/PluginData.cpp:68
>> +        return codePointCompare(a.name, b.name) < 0;
> 
> Consider using codePointCompareLessThan?
> 
> Is there a reason we are sorting in a case sensitive way rather than a case folding way?

Neat -- I didn't know about codePointCompareLessThan! I'll switch.

No. I just want a consistent ordering returned to the client to minimize differences from systems to system. Is there a preferred sorting criteria besides codePointCompareLessThan?

>> Source/WebCore/plugins/PluginData.h:93
>> +    Vector<PluginInfo> publiclyVisiblePlugins() const;
> 
> Very unclear naming here. How is "web visible" different from "publicly visible". We should use more precise terminology.

The "web visible" plugins are the set of things that are allowed to be accessed from the web based on the "WebPlatformStrategies" settings. The "publiclyVisiblePlugins" are the subset of the "webVisiblePlugins" that we want to publicly advertise. All others should only be findable using a specific existence test.

Maybe "advertisedWebVisiblePlugins" or "publiclyVisibleWebVisiblePlugins"?

>> Source/WebCore/testing/Internals.cpp:3095
>> +        page->setShowAllPlugins(show);
> 
> Slightly strange combination of idioms. Early return for one null check, but nested if for the other.

I'll revise this to be consistent.
Comment 14 Brent Fulgham 2015-10-16 14:26:25 PDT
Committed r191211: <http://trac.webkit.org/changeset/191211>