-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interop: pdf might or might not render in a sandboxed iframe (depending on a browser) #3958
Comments
/cc @bzbarsky and @travisleithead as people who I remember being interested in issues around PDFs as a very special type of "plugin" (sometimes not a plugin). And @rniwa for good measure. Vaguely related: #3462 |
https://html.spec.whatwg.org/#sandboxed-plugins-browsing-context-flag already allows for exceptions for secured plugins, which it seems like application/pdf might be in Chrome? |
I agree with @annevk that this isn't a spec issue; the spec clearly allows user agents to carve out plugins that act equivalently to web content with regard to sandboxing flags. I'm fairly certain that Chrome's PDF plugin does not respect things like If we can secure it in this way, great! If not, then it seems correct to continue blocking it. |
I think my preference at this point would be that we standardize upon not allowing sandboxed PDF and also start acknowledging PDF as a special type we'll have to account for going forward in page load. |
Note that we have a test for this as well apparently: |
What's the recommended way to deliver untrusted PDFs if sandboxing isn't one of them? |
I think the recommendation is: don't, since browsers are not capable of defending against malicious PDFs right now. ("Malicious" here = wants to do things disallowed by sandboxing, like navigate the parent page. Browsers can and do try to defend against PDFs that try to do system exploits like arbitrary code execution, as part of their same general code that defends against that for any content.) |
Given the protections against arbitrary code execution, I have more faith in browsers handling malicious PDFs than I have in users downloading and opening such files in their average PDF viewer app. While browsers might currently not support the entire sandboxing set for PDFs, even a subset of the protections would be great, no? |
I agree it would be great! Nobody has implemented it though. |
You can consider delivering untrustworthy PDFs from a separate origin or site (e.g. from https://untrustworthy-pdfs.example.com rather than from https://main-site.example.com). This should at least theoretically allow browsers to host such PDFs in a separate process (i.e. introducing a security barrier from the main site). I recognize that unlike sandboxed iframes this approach doesn't put each PDF in a separate origin. In particular it still allows mixing of PDFs from various sources (e.g. PDFs provided by two different site users). /CC @kmoon-chromium and @csreis as FYI |
Instead, sandboxed documents are just never allowed to display plugins. (Which, in the modern world, just means PDFs.) Closes whatwg#3958. Helps with whatwg#6003.
https://bugs.webkit.org/show_bug.cgi?id=284594 rdar://141166987 Reviewed by NOBODY (OOPS!). The PDF plugin is an internal WebKit implementation detail, and thus should not be subjected to the CSP sandbox. This patch makes sure we bypass the sandbox in SubframeLoader::pluginIsLoadable(). Note that we only do so for main frame PDFs. The embedded PDF case's behavior is directed by whatwg/html#3958, and the WPT `html/semantics/embedded-content/the-iframe-element/sandbox_004.htm`. Also, add two new API tests that assert correct loading behavior. The latter enables UnifiedPDFPlugin while the former tests legacy PDF plugin. - ContentSecurityPolicy.LoadPDFWithSandboxCSPDirective - UnifiedPDF.LoadPDFWithSandboxCSPDirective The rest of the change involves adding new test helpers to facilitate the API tests, TestWKWebView interface to sample colors (which, by the way, should be adopted by many tests), and the fallout unified source build fixes required for a clean build. * Source/WebCore/html/HTMLPlugInImageElement.h: * Source/WebCore/loader/SubframeLoader.cpp: (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable const): (WebCore::FrameLoader::SubframeLoader::requestPlugin): (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable): Deleted. * Source/WebCore/loader/SubframeLoader.h: * Tools/TestWebKitAPI/SourcesCocoa.txt: * Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: * Tools/TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicy.mm: (TEST(ContentSecurityPolicy, LoadPDFWithSandboxCSPDirective)): * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.h: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.mm: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. (TestWebKitAPI::runLoadPDFWithSandboxCSPDirectiveTest): * Tools/TestWebKitAPI/Tests/WebKitCocoa/CookieAcceptPolicy.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm: (applyStyle): Deleted. (applyAhemStyle): Deleted. * Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm: (): Deleted. (TestWebKitAPI::applyAhemStyle): Deleted. Fix ODR violation by pushing applyAhemStyle into specific namespaces. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ElementTargetingTests.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/NavigationAction.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/TextWidth.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/UnifiedPDFTests.mm: (TestWebKitAPI::UNIFIED_PDF_TEST): (TestWebKitAPI::sampleColorsInWebView): Deleted. * Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm: * Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.cpp: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h: * Tools/TestWebKitAPI/cocoa/HTTPServer.h: (TestWebKitAPI::HTTPResponse::HTTPResponse): * Tools/TestWebKitAPI/cocoa/TestWKWebView.h: * Tools/TestWebKitAPI/cocoa/TestWKWebView.mm: (-[TestWKWebView sampleColors]): (-[TestWKWebView sampleColorsWithInterval:]):
https://bugs.webkit.org/show_bug.cgi?id=284594 rdar://141166987 Reviewed by NOBODY (OOPS!). The PDF plugin is an internal WebKit implementation detail, and thus should not be subjected to the CSP sandbox. This patch makes sure we bypass the sandbox in SubframeLoader::pluginIsLoadable(). Note that we only do so for main frame PDFs. The embedded PDF case's behavior is directed by whatwg/html#3958, and the WPT `html/semantics/embedded-content/the-iframe-element/sandbox_004.htm`. Also, add two new API tests that assert correct loading behavior. The latter enables UnifiedPDFPlugin while the former tests legacy PDF plugin. - ContentSecurityPolicy.LoadPDFWithSandboxCSPDirective - UnifiedPDF.LoadPDFWithSandboxCSPDirective The rest of the change involves adding new test helpers to facilitate the API tests, TestWKWebView interface to sample colors (which, by the way, should be adopted by many tests), and the fallout unified source build fixes required for a clean build. * Source/WebCore/html/HTMLPlugInImageElement.h: * Source/WebCore/loader/SubframeLoader.cpp: (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable const): (WebCore::FrameLoader::SubframeLoader::requestPlugin): (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable): Deleted. * Source/WebCore/loader/SubframeLoader.h: * Tools/TestWebKitAPI/SourcesCocoa.txt: * Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: * Tools/TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicy.mm: (TEST(ContentSecurityPolicy, LoadPDFWithSandboxCSPDirective)): * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.h: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.mm: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. (TestWebKitAPI::runLoadPDFWithSandboxCSPDirectiveTest): * Tools/TestWebKitAPI/Tests/WebKitCocoa/CookieAcceptPolicy.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm: (applyStyle): Deleted. (applyAhemStyle): Deleted. * Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm: (): Deleted. (TestWebKitAPI::applyAhemStyle): Deleted. Fix ODR violation by pushing applyAhemStyle into specific namespaces. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ElementTargetingTests.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/NavigationAction.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/TextWidth.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/UnifiedPDFTests.mm: (TestWebKitAPI::UNIFIED_PDF_TEST): (TestWebKitAPI::sampleColorsInWebView): Deleted. * Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm: * Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.cpp: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h: * Tools/TestWebKitAPI/cocoa/HTTPServer.h: (TestWebKitAPI::HTTPResponse::HTTPResponse): * Tools/TestWebKitAPI/cocoa/TestWKWebView.h: * Tools/TestWebKitAPI/cocoa/TestWKWebView.mm: (-[TestWKWebView sampleColors]): (-[TestWKWebView sampleColorsWithInterval:]):
https://bugs.webkit.org/show_bug.cgi?id=284594 rdar://141166987 Reviewed by Wenson Hsieh. The PDF plugin is an internal WebKit implementation detail, and thus should not be subjected to the CSP sandbox. This patch makes sure we bypass the sandbox in SubframeLoader::pluginIsLoadable(). Note that we only do so for main frame PDFs. The embedded PDF case's behavior is directed by whatwg/html#3958, and the WPT `html/semantics/embedded-content/the-iframe-element/sandbox_004.htm`. Also, add two new API tests that assert correct loading behavior. The latter enables UnifiedPDFPlugin while the former tests legacy PDF plugin. - ContentSecurityPolicy.LoadPDFWithSandboxCSPDirective - UnifiedPDF.LoadPDFWithSandboxCSPDirective The rest of the change involves adding new test helpers to facilitate the API tests, TestWKWebView interface to sample colors (which, by the way, should be adopted by many tests), and the fallout unified source build fixes required for a clean build. * Source/WebCore/html/HTMLPlugInImageElement.h: * Source/WebCore/loader/SubframeLoader.cpp: (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable const): (WebCore::FrameLoader::SubframeLoader::requestPlugin): (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable): Deleted. * Source/WebCore/loader/SubframeLoader.h: * Tools/TestWebKitAPI/SourcesCocoa.txt: * Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: * Tools/TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicy.mm: (TEST(ContentSecurityPolicy, LoadPDFWithSandboxCSPDirective)): * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.h: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.mm: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. (TestWebKitAPI::runLoadPDFWithSandboxCSPDirectiveTest): * Tools/TestWebKitAPI/Tests/WebKitCocoa/CookieAcceptPolicy.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm: (applyStyle): Deleted. (applyAhemStyle): Deleted. * Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm: (): Deleted. (TestWebKitAPI::applyAhemStyle): Deleted. Fix ODR violation by pushing applyAhemStyle into specific namespaces. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ElementTargetingTests.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/NavigationAction.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/TextWidth.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/UnifiedPDFTests.mm: (TestWebKitAPI::UNIFIED_PDF_TEST): (TestWebKitAPI::sampleColorsInWebView): Deleted. * Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm: * Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.cpp: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h: * Tools/TestWebKitAPI/cocoa/HTTPServer.h: (TestWebKitAPI::HTTPResponse::HTTPResponse): * Tools/TestWebKitAPI/cocoa/TestWKWebView.h: * Tools/TestWebKitAPI/cocoa/TestWKWebView.mm: (-[TestWKWebView sampleColors]): (-[TestWKWebView sampleColorsWithInterval:]): Canonical link: https://commits.webkit.org/288059@main
https://bugs.webkit.org/show_bug.cgi?id=284594 rdar://141166987 Reviewed by Wenson Hsieh. The PDF plugin is an internal WebKit implementation detail, and thus should not be subjected to the CSP sandbox. This patch makes sure we bypass the sandbox in SubframeLoader::pluginIsLoadable(). Note that we only do so for main frame PDFs. The embedded PDF case's behavior is directed by whatwg/html#3958, and the WPT `html/semantics/embedded-content/the-iframe-element/sandbox_004.htm`. Also, add two new API tests that assert correct loading behavior. The latter enables UnifiedPDFPlugin while the former tests legacy PDF plugin. - ContentSecurityPolicy.LoadPDFWithSandboxCSPDirective - UnifiedPDF.LoadPDFWithSandboxCSPDirective The rest of the change involves adding new test helpers to facilitate the API tests, TestWKWebView interface to sample colors (which, by the way, should be adopted by many tests), and the fallout unified source build fixes required for a clean build. * Source/WebCore/html/HTMLPlugInImageElement.h: * Source/WebCore/loader/SubframeLoader.cpp: (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable const): (WebCore::FrameLoader::SubframeLoader::requestPlugin): (WebCore::FrameLoader::SubframeLoader::pluginIsLoadable): Deleted. * Source/WebCore/loader/SubframeLoader.h: * Tools/TestWebKitAPI/SourcesCocoa.txt: * Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: * Tools/TestWebKitAPI/Tests/WebKit/WKThumbnailView.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicy.mm: (TEST(ContentSecurityPolicy, LoadPDFWithSandboxCSPDirective)): * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.h: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ContentSecurityPolicyTestHelpers.mm: Copied from Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h. (TestWebKitAPI::runLoadPDFWithSandboxCSPDirectiveTest): * Tools/TestWebKitAPI/Tests/WebKitCocoa/CookieAcceptPolicy.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/DocumentEditingContext.mm: (applyStyle): Deleted. (applyAhemStyle): Deleted. * Tools/TestWebKitAPI/Tests/WebKitCocoa/EditorStateTests.mm: (): Deleted. (TestWebKitAPI::applyAhemStyle): Deleted. Fix ODR violation by pushing applyAhemStyle into specific namespaces. * Tools/TestWebKitAPI/Tests/WebKitCocoa/ElementTargetingTests.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/NavigationAction.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/SiteIsolation.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/TextWidth.mm: * Tools/TestWebKitAPI/Tests/WebKitCocoa/UnifiedPDFTests.mm: (TestWebKitAPI::UNIFIED_PDF_TEST): (TestWebKitAPI::sampleColorsInWebView): Deleted. * Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm: * Tools/TestWebKitAPI/Tests/mac/WKWebViewMacEditingTests.mm: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.cpp: * Tools/TestWebKitAPI/cocoa/CGImagePixelReader.h: * Tools/TestWebKitAPI/cocoa/HTTPServer.h: (TestWebKitAPI::HTTPResponse::HTTPResponse): * Tools/TestWebKitAPI/cocoa/TestWKWebView.h: * Tools/TestWebKitAPI/cocoa/TestWKWebView.mm: (-[TestWKWebView sampleColors]): (-[TestWKWebView sampleColorsWithInterval:]): Canonical link: https://commits.webkit.org/288060@main
Iframe sandbox disables plugins. This leads to the following interop problem: some browsers (like Chrome) implement PDF rendering via a plugin and therefore PDF might not render in a sandboxed iframe (or in pop-ups opened from a sandboxed iframe).
Brainstorming some ideas for how the spec can help interoperability here:
Maybe the spec can introduce a mechanism for more granular sandboxing of plugins (e.g. so that a parent frame may allow its subframe to embed plugins handling application/pdf but not application/x-shockwave-flash). This doesn't seem to help interop, because for backcompatibility, the default behavior would have to block all plugins (and so the default behavior would remain not interoperable)
Maybe the spec can give browsers some leeway in what plugins are blocked. For example - Chrome's PDF rendering restricts which APIs are exposed to PDFs (e.g. Net.* methods are not implemented) and maybe such restriction means that sandboxed frames should be allowed to embed PDF plugin.
In the long term high-profile plugins like Flash and PNaCl will be deprecated (Flash by 2020 although AFAIK PNaCl remains supported for Chrome Apps for foreseeable future). Maybe this means that browsers shouldn't render PDF via a plugin?
Other notes:
The text was updated successfully, but these errors were encountered: