Bug 220665 – Safari says "Blocked Plug-in" instead showing a PDF

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

WebKit's PDFPlugin is a browser implementation detail and should probably bypass this CSP check.

[Kate Cheney]

<rdar://problem/64372944>

[Kate Cheney]

Created attachment 417725 [details]
Patch

[Tim Horton]

Comment on attachment 417725 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:278
> +        for (auto& pluginInfo : document().page()->pluginData().plugins()) {
> +            if (pluginInfo.bundleIdentifier == "com.apple.webkit.builtinpdfplugin"_s)
> +                return true;
> +        }

Kinda confused. is pluginData().plugins() not the set of installed plugins? Wouldn't this always evaluate to true? (even if, say, Adobe Reader plugin were being used to load the PDF)? Or is it actually the set of plugins being used for this page?

[Darin Adler]

Comment on attachment 417725 [details]
Patch

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

Seems really important to fix this, very good that we will have a regression test now!

I’m going to say review+ even though I have concerns about the content of the patch.

> Source/WebCore/ChangeLog:3
> +        Safari says "Blocked Plug-in" instead showing a PDF

instead *of* showing a PDF

Shouldn’t the bug title pinpoint where/when we regressed? I assume this is a regression.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:281
> +bool HTMLPlugInImageElement::shouldBypassCSPForPDFPlugin() const
> +{
> +    if (document().frame()->isMainFrame() && document().loader()->writer().mimeType() == "application/pdf") {
> +        for (auto& pluginInfo : document().page()->pluginData().plugins()) {
> +            if (pluginInfo.bundleIdentifier == "com.apple.webkit.builtinpdfplugin"_s)
> +                return true;
> +        }
> +    }
> +    return false;
> +}

This seems loosely coupled. We don’t want another plug-in to be able to claim application/pdf and get run because the built-in PDF plug-in is present. But that’s what this code says, and so it relies on code elsewhere.

Is there a way to target this more precisely, so it only targets the one plug-in rather than all plug-ins. It’s not great that this counts on the fact that this plug-in if present always "wins".

I suppose given this, I’d like to at least eventually see a test that proves that trick doesn’t work, with a test plug-in that claims application/pdf and prove that it doesn't run.

It’s *critical* to quickly fix this regression; I don’t have good insight how it was introduced and how important it is to use this fix we have here even if it’s not perfectly target.

[Tim Horton]

> It’s not great that this counts on the fact that this plug-in if present always "wins".

I don't even think this is true?

[Kate Cheney]

(In reply to Tim Horton from comment #3)
> Comment on attachment 417725 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417725&action=review
> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:278
> > +        for (auto& pluginInfo : document().page()->pluginData().plugins()) {
> > +            if (pluginInfo.bundleIdentifier == "com.apple.webkit.builtinpdfplugin"_s)
> > +                return true;
> > +        }
> 
> Kinda confused. is pluginData().plugins() not the set of installed plugins?
> Wouldn't this always evaluate to true? (even if, say, Adobe Reader plugin
> were being used to load the PDF)? Or is it actually the set of plugins being
> used for this page?

I think it is only added to the list of plugins if an additional PDF plugin is not being used (see the comment in WebProcessProxy::getPlugins()).

[Tim Horton]

(In reply to katherine_cheney from comment #6)
> (In reply to Tim Horton from comment #3)
> > Comment on attachment 417725 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=417725&action=review
> > 
> > > Source/WebCore/html/HTMLPlugInImageElement.cpp:278
> > > +        for (auto& pluginInfo : document().page()->pluginData().plugins()) {
> > > +            if (pluginInfo.bundleIdentifier == "com.apple.webkit.builtinpdfplugin"_s)
> > > +                return true;
> > > +        }
> > 
> > Kinda confused. is pluginData().plugins() not the set of installed plugins?
> > Wouldn't this always evaluate to true? (even if, say, Adobe Reader plugin
> > were being used to load the PDF)? Or is it actually the set of plugins being
> > used for this page?
> 
> I think it is only added to the list of plugins if an additional PDF plugin
> is not being used (see the comment in WebProcessProxy::getPlugins()).

I don't think that's what the comment says? omitPDFSupport isn't about if there are other PDF plugins, it's about a secret default.

[Kate Cheney]

Comment on attachment 417725 [details]
Patch

Seems like there is probably a better way to do this, maybe by checking to make sure the client is Safari or is explicitly using the WebKit PDFPlugin (not just has it installed).

[Kate Cheney]

Created attachment 417747 [details]
Patch

[Kate Cheney]

Ok, round 2. I figured out that the PDFPlugin object isn’t created until after the CSP check, so we can’t use that. This patch checks for disabled plugins and then performs the same check as WebPage::createPlugin() to see if we will create a PDFPlugin. This prevents the case where another plugin tries to claim application/pdf.

[Kate Cheney]

Created attachment 417748 [details]
Patch

[Darin Adler]

Comment on attachment 417747 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:272
> +bool HTMLPlugInImageElement::shouldBypassCSPForPDFPlugin(const String& mimeType) const

Sometimes we use the more elegant term "content type" instead of "MIME type"; I prefer that term almost always. But this file has a lot of mimeType.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:278
> +    // We only consider bypassing this CSP check if plugins are disabled. In that case we know that
> +    // any plugin used is a browser implementation detail.
> +    if (document().frame()->arePluginsEnabled())
> +        return false;

This explains why it’s safe to bypass CSP when plug-ins are disabled, but does not explain why it’s unnecessary to bypass CSP when plug-ins are enabled.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:281
> +    if (document().frame()->loader().client().shouldUsePDFPlugin() && (MIMETypeRegistry::isPDFOrPostScriptMIMEType(mimeType) || (mimeType.isEmpty() && (path.endsWithIgnoringASCIICase(".pdf") || path.endsWithIgnoringASCIICase(".ps")))))

Is there a way to share some of this logic with the PDF plug-in code? Seems like this must be echoing a rule implemented elsewhere.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:283
> +#endif

#else
    UNUSED_PARAM(mimeType);
#endif

[Darin Adler]

Comment on attachment 417747 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:281
>> +    if (document().frame()->loader().client().shouldUsePDFPlugin() && (MIMETypeRegistry::isPDFOrPostScriptMIMEType(mimeType) || (mimeType.isEmpty() && (path.endsWithIgnoringASCIICase(".pdf") || path.endsWithIgnoringASCIICase(".ps")))))
> 
> Is there a way to share some of this logic with the PDF plug-in code? Seems like this must be echoing a rule implemented elsewhere.

Reading the comments in more detail, wondering specifically if can we refactor so we share the code to check the rule with WebPage::createPlugin.

[Darin Adler]

Comment on attachment 417747 [details]
Patch

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

>>> Source/WebCore/html/HTMLPlugInImageElement.cpp:281
>>> +    if (document().frame()->loader().client().shouldUsePDFPlugin() && (MIMETypeRegistry::isPDFOrPostScriptMIMEType(mimeType) || (mimeType.isEmpty() && (path.endsWithIgnoringASCIICase(".pdf") || path.endsWithIgnoringASCIICase(".ps")))))
>> 
>> Is there a way to share some of this logic with the PDF plug-in code? Seems like this must be echoing a rule implemented elsewhere.
> 
> Reading the comments in more detail, wondering specifically if can we refactor so we share the code to check the rule with WebPage::createPlugin.

Maybe the way to achieve this is to give shouldUsePDFPlugin a different name and pass in the content type, then share the code.

[Kate Cheney]

Created attachment 417765 [details]
Patch

[Kate Cheney]

New patch uploaded. Let me know if this is what you meant by making the code shared. Otherwise this patch addresses your comments, including noting in the comment that we should consider actually looking for alternative installed PDF plugins instead of only checking the disabled plugins case. This is hard right now because this info lives in PluginInfoStore.cpp in the UIProcess. This is fine for Safari which will always have plugins disabled.

[Brent Fulgham]

Comment on attachment 417765 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1070
> +        || path.endsWithIgnoringASCIICase(".ps"))));

This indentation is a little misleading, since the 'contentType.isEmpty()' applies to both the PDF and PS cases. I'd almost prefer having the pdf and ps cases on the same line, but maybe indenting would also make it clear.

[Kate Cheney]

Created attachment 417911 [details]
Patch

[Kate Cheney]

Comment on attachment 417765 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1070
>> +        || path.endsWithIgnoringASCIICase(".ps"))));
> 
> This indentation is a little misleading, since the 'contentType.isEmpty()' applies to both the PDF and PS cases. I'd almost prefer having the pdf and ps cases on the same line, but maybe indenting would also make it clear.

You're right, this is a strange indent situation. I uploaded a new patch with everything in the second part (starting with MIMETypeRegistry::) on the same line. It is a long line but I think more clear.

[Darin Adler]

Comment on attachment 417765 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInImageElement.cpp:272
> +bool HTMLPlugInImageElement::shouldBypassCSPForPDFPlugin(const String& contentType) const

It’s cool to have call the shouldUsePDFPlugin in the client, but I still think there’s a bit of a logic gap given these checks that are in WebPage::createPlugin:

    if (isUnsupported || isBlockedPlugin || !pluginProcessToken)

Not sure I totally understand the significance of not having these checks or their equivalent. Seems like we would get it wrong with PDF plug-ins.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:278
> +    // FIXME: Check for alternative PDF plugins here so we know whether we will use PDFPlugin.

Is it OK to land without resolving this? I suppose it is for Safari, but what about non-Safari WebKit clients? Are we doing damage to CSP by adding this bypass?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:282
> +    if (document().frame()->loader().client().shouldUsePDFPlugin(contentType,  document().url().path()))

Stray space here after the comma.

I’d also suggest just doing a return here instead of an if statement and moving the "return false" inside the #else, but not critical.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1958
> +    if (!webPage)
> +        return false;
> +
> +    return webPage->shouldUsePDFPlugin(contentType, path);

I like to use the "&&" style for these:

    auto page = m_frame->page();
    return page && page->shouldUsePDFPlugin(contentType, path);

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1070
>> +        || path.endsWithIgnoringASCIICase(".ps"))));
> 
> This indentation is a little misleading, since the 'contentType.isEmpty()' applies to both the PDF and PS cases. I'd almost prefer having the pdf and ps cases on the same line, but maybe indenting would also make it clear.

I agree that we could improve the indenting. Generally we try to show the structure with indentation with the "||" be further in.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1086
> +    bool canUsePDFPlugin() const;

Should also make this private since it’s now only called by shouldUsePDFPlugin. Or even possibly just move the two checks into the new function (and put the whole thing into the .mm file instead of the .cpp).

> Source/WebKit/WebProcess/WebPage/WebPage.h:1383
> +    bool shouldUsePDFPlugin(const String& contentType, const StringView& path) const;

Should just be StringView, not "const StringView&".

[Kate Cheney]

Comment on attachment 417765 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:272
>> +bool HTMLPlugInImageElement::shouldBypassCSPForPDFPlugin(const String& contentType) const
> 
> It’s cool to have call the shouldUsePDFPlugin in the client, but I still think there’s a bit of a logic gap given these checks that are in WebPage::createPlugin:
> 
>     if (isUnsupported || isBlockedPlugin || !pluginProcessToken)
> 
> Not sure I totally understand the significance of not having these checks or their equivalent. Seems like we would get it wrong with PDF plug-ins.

You're right that this part of the check is missing from the new check in CSP, but checking for disabled plugins in the CSP check is stronger than this because in that case pluginProcessToken would always be 0, so it is not loosening any existing requirements. 

In order to get these exact results here we would need a sync IPC call to the UIProcess to check for a compatible plugin, and I wasn't sure about perf repercussions.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:278
>> +    // FIXME: Check for alternative PDF plugins here so we know whether we will use PDFPlugin.
> 
> Is it OK to land without resolving this? I suppose it is for Safari, but what about non-Safari WebKit clients? Are we doing damage to CSP by adding this bypass?

My thought process is this: currently this doesn't work for any WebKit clients. This patch adds support for Safari and many other clients who completely disable plugins. It does not damage CSP because any plugin that is not a browser implementation detail will still be blocked, as expected. So this patch won't hurt anything, and will help in many cases. Sending sync IPC to the UIProcess and delaying the load did not seem like a great thing to do here, maybe I am wrong about that.

>> Source/WebCore/html/HTMLPlugInImageElement.cpp:282
>> +    if (document().frame()->loader().client().shouldUsePDFPlugin(contentType,  document().url().path()))
> 
> Stray space here after the comma.
> 
> I’d also suggest just doing a return here instead of an if statement and moving the "return false" inside the #else, but not critical.

Will change.

>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1958
>> +    return webPage->shouldUsePDFPlugin(contentType, path);
> 
> I like to use the "&&" style for these:
> 
>     auto page = m_frame->page();
>     return page && page->shouldUsePDFPlugin(contentType, path);

Will change.

>> Source/WebKit/WebProcess/WebPage/WebPage.h:1086
>> +    bool canUsePDFPlugin() const;
> 
> Should also make this private since it’s now only called by shouldUsePDFPlugin. Or even possibly just move the two checks into the new function (and put the whole thing into the .mm file instead of the .cpp).

Yes, I can do that.

[Kate Cheney]

Comment on attachment 417765 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.h:1383
>> +    bool shouldUsePDFPlugin(const String& contentType, const StringView& path) const;
> 
> Should just be StringView, not "const StringView&".

Why copy here?

[Darin Adler]

Comment on attachment 417765 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/WebPage.h:1383
>>> +    bool shouldUsePDFPlugin(const String& contentType, const StringView& path) const;
>> 
>> Should just be StringView, not "const StringView&".
> 
> Why copy here?

The StringView object fits into machine registers, is really small, and has no cost to copy except in debug builds. It’s a pointer plus an integer/boolean. For more complex types, using "const X&" optimizes performance, but for StringView does that instead has a net effect that hurts performance.

That’s different from classes like String where copying does reference count manipulation and is to be avoided when possible.

That’s not obvious, and I’m sorry that’s so!

[Kate Cheney]

Created attachment 417929 [details]
Patch

[Kate Cheney]

(In reply to Darin Adler from comment #23)
> Comment on attachment 417765 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417765&action=review
> 
> >>> Source/WebKit/WebProcess/WebPage/WebPage.h:1383
> >>> +    bool shouldUsePDFPlugin(const String& contentType, const StringView& path) const;
> >> 
> >> Should just be StringView, not "const StringView&".
> > 
> > Why copy here?
> 
> The StringView object fits into machine registers, is really small, and has
> no cost to copy except in debug builds. It’s a pointer plus an
> integer/boolean. For more complex types, using "const X&" optimizes
> performance, but for StringView does that instead has a net effect that
> hurts performance.
> 
> That’s different from classes like String where copying does reference count
> manipulation and is to be avoided when possible.
> 
> That’s not obvious, and I’m sorry that’s so!

Thank you for explaining!

I updated the Changelog in the most recent patch to clarify some of what we talked about in the comments, and why I think it is a good idea to land this even if it doesn't cover 100% of cases.

[Darin Adler]

Comment on attachment 417929 [details]
Patch

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

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1955
> +    auto* webPage = m_frame->page();
> +    return webPage && webPage->shouldUsePDFPlugin(contentType, path);

I slightly prefer the name "page" for this local variable, but I won’t insist. I really like single word variable names when they work grammatically.

[Kate Cheney]

Created attachment 417973 [details]
Patch for landing

[Kate Cheney]

(In reply to Darin Adler from comment #26)
> Comment on attachment 417929 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417929&action=review
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1955
> > +    auto* webPage = m_frame->page();
> > +    return webPage && webPage->shouldUsePDFPlugin(contentType, path);
> 
> I slightly prefer the name "page" for this local variable, but I won’t
> insist. I really like single word variable names when they work
> grammatically.

I fixed this in the patch-for-landing. Thanks for the review!

[EWS]

Committed r271650: <https://trac.webkit.org/changeset/271650>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417973 [details].

[Radar WebKit Bug Importer]

<rdar://problem/73408146>

[alexandre robuchon]

*** Bug 223576 has been marked as a duplicate of this bug. ***