Bug 172038 – REGRESSION (r209608): Cross-origin plugin document opened in child window blocked by parent window CSP when object-src 'none' is set

• Component: New Bugs
• Status: RESOLVED FIXED

[Markus Backman]

Created attachment 309921 [details]
zip with screenshots

Our site, https://online.swedbank.se is using CSP to ensure that a customers online banking session is as secured as possible. The CSP contains among other object-src 'none'; to ensure that no plugins are allowed to be executed under our domain as we currently don't have that need. 

But with the latest Safari (Technology Preview have the same problem) we have gotten reports from our customer that the internal PDFPlugin in Safari gets blocked in the cases we transport a customer to a separate domain. That domain don't have any CSP policy defined by itself. Also when we open a PDF, in this case a electronic invoice that is hosted by a 3de party provider, we ensure to open a new tab so the customer can view the PDF in that tab and continue the online banking presence in the original tab.

Lastest Chrome, FF, Edge and IE (well not much of CSP there) opens the tab and allow the display of the PDF without the CSP policy tries to stop it. But of course this might be a different in how to interpret the CSP spec. If a domain opens a new tab and that points to another domain that serves a content the requires a plugin, should that domains CSP come into play here? Of course our site is driving traffic to the other domain but the PDF isn't served from our domain. The solution is of course to specify all domains that we load PDF from. This would most likely solve the problem. But would be good if we had one common view on how this should be handled over the different browsers.

I have tried to add a couple of screenshots as well as some proxy logs on the actual traffic that is going on. 

1.png -> the view of the customer before clicking to view the e-invoice
2.png -> the view of the customer after clicked the view link and a new tab has been opened
3.png -> network traffic one the POST to the external domain e-faktura1.stralfors.net which response with a 302 and redirect the traffic
4.png -> Request to the redirect to /invoiceViewer/showInvoiceSE.htm. Gives 200
5.png -> Response from the request, contains the PDF document

But the end results is that the CSP served from online.swedbank.se kicks in and stops the initiation of the PDFPlugin in Safari.

[John Wilander]

Thanks for reporting, Markus!

Dan, what's your take?

[Daniel Bates]

Created attachment 309951 [details]
Test case

[Daniel Bates]

(In reply to John Wilander from comment #1)
> Thanks for reporting, Markus!
> 
> Dan, what's your take?

This bug was caused by <http://trac.webkit.org/changeset/209608> (bug #15531). We should only have a plugin document loaded in a subframe inherit the CSP policy of its parent frame regardless of whether it inherits the security origin of its parent. This will allow a cross-origin plugin document loaded in a child window to be allowed to load regardless of the CSP policy of its opener.   

Additional remarks:

We treat a plugin documents (direct navigation to a document that requires a plugin) as special case. In particular, a plugin document loaded in a subframe (e.g. <object>) will inherit the CSP policy of its parent page regardless of whether the document inherits the security origin of its parent page. (Bug #153160 is about changing this behavior). Following the patch for bug #15531 we apply the same inheritance policy to a plugin document loaded in a child window.

[Daniel Bates]

(In reply to Daniel Bates from comment #3)
> (In reply to John Wilander from comment #1)
> > Thanks for reporting, Markus!
> > 
> > Dan, what's your take?
> 
> This bug was caused by <http://trac.webkit.org/changeset/209608> (bug
> #15531).

err, bug #165531

[Daniel Bates]

Created attachment 310097 [details]
Patch and layout tests

[Build Bot]

Comment on attachment 310097 [details]
Patch and layout tests

Attachment 310097 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3740468

New failing tests:
http/tests/security/contentSecurityPolicy/same-origin-plugin-document-blocked-in-child-window.html
http/tests/security/contentSecurityPolicy/plugin-blocked-in-about-blank-window.html
http/tests/security/contentSecurityPolicy/cross-origin-plugin-document-allowed-in-child-window.html

[Build Bot]

Created attachment 310099 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Daniel Bates]

(In reply to Build Bot from comment #6)
> Comment on attachment 310097 [details]
> Patch and layout tests
> 
> Attachment 310097 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/3740468
> 
> New failing tests:
> http/tests/security/contentSecurityPolicy/same-origin-plugin-document-
> blocked-in-child-window.html
> http/tests/security/contentSecurityPolicy/plugin-blocked-in-about-blank-
> window.html
> http/tests/security/contentSecurityPolicy/cross-origin-plugin-document-
> allowed-in-child-window.html

Will skip these tests on iOS as iOS does not support plugins.

[Daniel Bates]

Created attachment 310101 [details]
Patch and layout tests

[Andy Estes]

Comment on attachment 310101 [details]
Patch and layout tests

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

> Source/WebCore/dom/Document.cpp:5221
> +    if (!shouldInheritSecurityOriginFromOwner(m_url) && (!isPluginDocument() || (!parentFrame && !openerFrame->document()->securityOrigin().canAccess(securityOrigin()))))

This if statement is hard to parse. I think it'd read better with the plug-in document parts broken out into a function or a local variable with a descriptive name (e.g. shouldInheritSecurityOriginForPlugInDocument()).

[Radar WebKit Bug Importer]

<rdar://problem/32258262>

[Daniel Bates]

Comment on attachment 310101 [details]
Patch and layout tests

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

>> Source/WebCore/dom/Document.cpp:5221
>> +    if (!shouldInheritSecurityOriginFromOwner(m_url) && (!isPluginDocument() || (!parentFrame && !openerFrame->document()->securityOrigin().canAccess(securityOrigin()))))
> 
> This if statement is hard to parse. I think it'd read better with the plug-in document parts broken out into a function or a local variable with a descriptive name (e.g. shouldInheritSecurityOriginForPlugInDocument()).

Will extract logic into a member function called shouldInheritContentSecurityPolicyFromOwner():

bool Document::shouldInheritContentSecurityPolicyFromOwner() const
{
    ASSERT(m_frame);
    if (shouldInheritSecurityOriginFromOwner(m_url))
        return true;
    if (!isPluginDocument())
        return true;
    Frame* parentFrame = m_frame->tree().parent();
    Frame* openerFrame = m_frame->loader().opener();
    if (!parentFrame && !openerFrame)
        return false;
    if (parentFrame)
        return true;
    return !openerFrame->document()->securityOrigin().canAccess(securityOrigin());
}

Then will update the code in Document::initContentSecurityPolicy() to read:

void Document::initContentSecurityPolicy()
{
    // 1. Inherit Upgrade Insecure Requests
    Frame* parentFrame = m_frame->tree().parent();
    if (parentFrame)
        contentSecurityPolicy()->copyUpgradeInsecureRequestStateFrom(*parentFrame->document()->contentSecurityPolicy());

    // 2. Inherit Content Security Policy
    if (!shouldInheritContentSecurityPolicyFromOwner())
        return;
    Frame* ownerFrame = parentFrame;
    if (!ownerFrame)
        ownerFrame = m_frame->loader().opener();
    contentSecurityPolicy()->copyStateFrom(ownerFrame->document()->contentSecurityPolicy()); // Does not copy Upgrade Insecure Requests state.
}

[Daniel Bates]

(In reply to Daniel Bates from comment #12)
> Will extract logic into a member function called
> shouldInheritContentSecurityPolicyFromOwner():
> 
> bool Document::shouldInheritContentSecurityPolicyFromOwner() const
> {
>     ASSERT(m_frame);
>     if (shouldInheritSecurityOriginFromOwner(m_url))
>         return true;
>     if (!isPluginDocument())
>         return true;

err, this should return false in this case.

[Daniel Bates]

(In reply to Daniel Bates from comment #12)
> Will extract logic into a member function called
> shouldInheritContentSecurityPolicyFromOwner():
> 
> bool Document::shouldInheritContentSecurityPolicyFromOwner() const
> {
>     ASSERT(m_frame);
>     if (shouldInheritSecurityOriginFromOwner(m_url))
>         return true;
>     if (!isPluginDocument())
>         return true;
>     Frame* parentFrame = m_frame->tree().parent();
>     Frame* openerFrame = m_frame->loader().opener();
>     if (!parentFrame && !openerFrame)
>         return false;
>     if (parentFrame)
>         return true;
>     return
> !openerFrame->document()->securityOrigin().canAccess(securityOrigin());

err, this should read:

openerFrame->document()->securityOrigin().canAccess(securityOrigin())

[Daniel Bates]

Created attachment 310456 [details]
Patch and layout tests

Updated patch based on feedback from Andy Estes.

[Build Bot]

Comment on attachment 310456 [details]
Patch and layout tests

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 310464 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310456 [details]
Patch and layout tests

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 310465 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310456 [details]
Patch and layout tests

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 310467 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310456 [details]
Patch and layout tests

Attachment 310456 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3765563

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 310469 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Daniel Bates]

Created attachment 310481 [details]
Patch and layout tests

[Andy Estes]

Comment on attachment 310481 [details]
Patch and layout tests

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

> Source/WebCore/dom/Document.cpp:5216
> +    Frame* parentFrame = m_frame->tree().parent();
> +    if (parentFrame)

You don't need the parentFrame local variable since you don't use if after the null check.

[Andy Estes]

(In reply to Andy Estes from comment #25)
> Comment on attachment 310481 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310481&action=review
> 
> > Source/WebCore/dom/Document.cpp:5216
> > +    Frame* parentFrame = m_frame->tree().parent();
> > +    if (parentFrame)
> 
> You don't need the parentFrame local variable since you don't use if after
> the null check.

... since you don't use *it* ...

[Daniel Bates]

(In reply to Andy Estes from comment #25)
> Comment on attachment 310481 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310481&action=review
> 
> > Source/WebCore/dom/Document.cpp:5216
> > +    Frame* parentFrame = m_frame->tree().parent();
> > +    if (parentFrame)
> 
> You don't need the parentFrame local variable since you don't use if after
> the null check.

Will inline the value of parentFrame into the if condition before landing.

[Daniel Bates]

Committed r217054: <http://trac.webkit.org/changeset/217054>