QtWebKit's default local load policy is WebCore::SecurityOrigin::AllowLocalLoadsForLocalAndSubstituteData. This policy automagically allows local content to load more local content, including local files. The problem now lies with WebCore::Settings::allowFileAccessFromFileURLs(), which gets ignored when local load policy allows substitute data to do so. What it means is that even if the developer sets this setting to false, local files are still accessible, which raises a security issue. The issue was found while doing tests on QtWebKit 2.2. On QtWebKit 2.1 this behavior is not present, thus it started with patch d287567e486ad3902fe6d79bcbad42f64f536bc5 . Trunk was tested as well and the issue is still present. PMO Bug 292822.
Created attachment 119597 [details] Proposed patch + Qt testcase. This patch adds a check on WebCore::SubframeLoader::loadSubframe for wheter WebCore::Settings::allowFileAccessFromFileURLs returns true or false. If true is returned, it is assumed that a user or global setting has higher priority than local load policy. This patch missed layout tests which I am finishing creating, but please consider reviewing it already.
Created attachment 119600 [details] Proposed patch + Qt testcase (updated for trunk). Refreshed patch for trunk. A layout test might not be required since the issue affects WebCore::Settings and thus needs to be tested by platform (in this case, a Qt testcase is provided).
Adding Adam Barth to CC list, as it envolves security issues.
Comment on attachment 119600 [details] Proposed patch + Qt testcase (updated for trunk). Attachment 119600 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10914703 New failing tests: fast/xmlhttprequest/xmlhttprequest-no-file-access.html
Comment on attachment 119600 [details] Proposed patch + Qt testcase (updated for trunk). View in context: https://bugs.webkit.org/attachment.cgi?id=119600&action=review > Source/WebCore/loader/SubframeLoader.cpp:265 > + // Document is allowed to load local resources, but frame settings > + // explicitly disables local file access. > + if (ownerElement->document()->securityOrigin()->canLoadLocalResources()) { > + Settings* settings = m_frame->settings(); > + if (settings && !settings->allowFileAccessFromFileURLs()) { > + FrameLoader::reportLocalLoadFailed(m_frame, url.string()); > + return 0; > + } > + } This is not a Qt change, so can't you test with this a layout test instead of a Qt autotest? Also the settings are per page group and not per frame. why not just write "but the settings"
Created attachment 129637 [details] Proposed patch + Layout tests + Qt auto test (updated) Updated patch with Kenneth's suggestions as well as a refactory on the code. The previous approach was causing another layout test to fail (fast/xmlhttprequest/xmlhttprequest-no-file-access.html). This new approach is less aggressive and allows unique resources to bypass the setting (ie. about:blank).
Comment on attachment 129637 [details] Proposed patch + Layout tests + Qt auto test (updated) View in context: https://bugs.webkit.org/attachment.cgi?id=129637&action=review > LayoutTests/fast/loader/resources/local-file-access-real.html:2 > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" > + "http://www.w3.org/TR/html4/loose.dtd"> Any reason this is not html5? > LayoutTests/fast/loader/resources/local-file-access-real.html:12 > +<script> > +function log(message) > +{ > + var console = document.getElementById('console'); > + console.appendChild(document.createTextNode(message)); > + console.appendChild(document.createElement('br')); > +} > + It should be possible to make javascript tests only. There is even a script for generating those, but im no expert on this > LayoutTests/fast/loader/resources/non-accessible-file.txt:1 > +FAIL: You should not see this message in the browser. hmm...
Created attachment 129708 [details] Proposed patch + layout test + Qt auto test (v2) Updated patch simplifying layout test code (using HTML5 DOCTYPE and removing unnecessary JavaScript functions). I've also added a bit of identation for better layout test code readability. As for the non-accessible-file.txt message, I needed a text file that could present a FAIL message if file could still be accessible even after the LocalContentCanAccessFileUrls is set to false. There is a similar text file ( LayoutTests/fast/loader/resources/plain-text-document.txt ) that does exactly the opposite (contains a PASS message if content is readable) used by another test, so I guess it is fine to use it as it is.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Attachment 129708 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1 WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h" Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:71: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 199 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 129708 [details] Proposed patch + layout test + Qt auto test (v2) View in context: https://bugs.webkit.org/attachment.cgi?id=129708&action=review Just an informal an quick review. > Source/WebCore/loader/SubframeLoader.cpp:269 > + if (!targetOrigin.get()->isUnique() && !document()->securityOrigin()->canAccess(targetOrigin.get())) { Why you can't use SecurityOrigin::passesFileCheck only? It already takes into account the flag allowFileAccessFromFileURLs. > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2542 > + while (loadFinishedSpy->count() == 1) Couldn't you use waitForSignal here instead?
Hi Rafael, Thanks for the feedback :) Here goes my answer to your questions: (In reply to comment #11) > (From update of attachment 129708 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129708&action=review > > Just an informal an quick review. > > > Source/WebCore/loader/SubframeLoader.cpp:269 > > + if (!targetOrigin.get()->isUnique() && !document()->securityOrigin()->canAccess(targetOrigin.get())) { > > Why you can't use SecurityOrigin::passesFileCheck only? It already takes into account the flag allowFileAccessFromFileURLs. I don't use because it is a private function. And even if it would be public, there are some issues to consider: The function initContextSecurity() (from Document()) that calls for enforceFilePathSeparation() if allowFileAccessFromFileURLs() is true, is not called on the test runs. I'm actually curious if copying the settings code used on the initContextSecurity() on SubframeLoader would make sense. > > > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2542 > > + while (loadFinishedSpy->count() == 1) > > Couldn't you use waitForSignal here instead? Good point :) I see this is a QtWebKit tests-only function, I'll update the patch replacing these while's with waitForSignal calls.
Created attachment 129785 [details] Proposed patch + layout test + Qt auto test (v3) Updated patch with refactory on Qt test code (using Rafael's suggestion for ::waitForSignal). His suggestion actually helped me find out a hidden bug on my testcase: The parent frame is always created after loading the HTML file, but the child frames created (or not) after frame loads the local file are what matters.
-cc a bunch of people the watchlist system mistakenly added
Comment on attachment 129785 [details] Proposed patch + layout test + Qt auto test (v3) View in context: https://bugs.webkit.org/attachment.cgi?id=129785&action=review > LayoutTests/fast/loader/local-file-access.html:10 > + <script> > +if (window.layoutTestController) { > + layoutTestController.dumpAsText(); > + layoutTestController.waitUntilDone(); > + layoutTestController.setAllowUniversalAccessFromFileURLs(false); > + layoutTestController.setAllowFileAccessFromFileURLs(false); > +} we normally indent this > LayoutTests/fast/loader/resources/local-file-access-real.html:14 > +function ReadFileContent() { > + var input1 = new FormData(); > + try { > + input1.append('data', local_file.document.firstChild.innerHTML); > + } catch(e) { > + if (window.layoutTestController) { > + layoutTestController.notifyDone(); > + } > + } > +} Same here > Source/WebCore/loader/SubframeLoader.cpp:271 > + if (ownerElement->document()->securityOrigin()->isLocal() && !settings->allowFileAccessFromFileURLs()) { > + RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); > + if (!targetOrigin.get()->isUnique() && !document()->securityOrigin()->canAccess(targetOrigin.get())) { > + FrameLoader::reportLocalLoadFailed(m_frame, url.string()); > + return 0; Shouldn't the security origin handle all this? ie. adhere to the setting?
Created attachment 136925 [details] Proposed patch + layout test + Qt auto test (v4) Please check the updated patch with LayoutTests JavaScript identation fixes. Details on security origin handling as follows: I may not be fully aware of how security origin works together with user-defined settings, but there's not a single query for WebCore::Settings properties inside WebCore::SecurityOrigin. AFAIU these are used together by WebCore::Document when initializing a new security context to enforce file path separation only. Also from WebCore::Document code, a child frame is allowed to access its ancestor even if WebCore::Settings' allowFileAccessFromFileURLs is set to false, but not the opposite. So what this patch fixes is the relationship from parent frames trying to load subframes. The WebCore::Settings' allowFileAccessFromFileUrls was completely ignored in that sense and thus might raise a security issue. As you can see on WebCore::SubframeLoader::loadSubframe code, this patch queries if the subframe's security origin is not unique neither can document security origin can access that origin before reporting local load failure. So in the end, security origin is taking into consideration, indeed.
Comment on attachment 136925 [details] Proposed patch + layout test + Qt auto test (v4) View in context: https://bugs.webkit.org/attachment.cgi?id=136925&action=review > Source/WebCore/loader/SubframeLoader.cpp:282 > + if (Settings* settings = m_frame->settings()) { > + // Document is allowed to load local resources, but the settings > + // explicitly disables local file access. > + if (ownerElement->document()->securityOrigin()->isLocal() && !settings->allowFileAccessFromFileURLs()) { > + RefPtr<SecurityOrigin> targetOrigin = SecurityOrigin::create(url); > + if (!targetOrigin.get()->isUnique() && !document()->securityOrigin()->canAccess(targetOrigin.get())) { > + FrameLoader::reportLocalLoadFailed(m_frame, url.string()); > + return 0; > + } > + } > + } This isn't the right place for this logic. The fix for this bug should be inside the SecurityOrigin::canDisplay call on the previous line. I don't fully understand what the problem is, but I suspect this isn't the right way to fix this issue. Hopefully the next iteration of this patch will make things clearer.
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.