Fixing iframe jank in the local omnibox popup.

chrome/browser/search/suggestion_source.cc

Index: chrome/browser/search/suggestion_source.cc
diff --git a/chrome/browser/search/suggestion_source.cc b/chrome/browser/search/suggestion_source.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f6f64b6ea981cd1c9a540e94579b4bcc7eb1e5ed
--- /dev/null
+++ b/chrome/browser/search/suggestion_source.cc
@@ -0,0 +1,176 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/search/suggestion_source.h"
+
+#include "base/json/string_escape.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted_memory.h"
+#include "base/string_util.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/instant_io_context.h"
+#include "chrome/browser/search/search.h"
+#include "chrome/common/url_constants.h"
+#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/browser/web_contents.h"
+#include "googleurl/src/gurl.h"
+#include "grit/browser_resources.h"
+#include "net/base/url_util.h"
+#include "net/url_request/url_request.h"
+#include "ui/base/layout.h"
+#include "ui/base/resource/resource_bundle.h"
+
+namespace {
+
+const char kLoaderHtmlPath[] = "/loader.html";
+const char kLoaderJSPath[] = "/loader.js";
+const char kResultHtmlPath[] = "/result.html";
+const char kResultJSPath[] = "/result.js";
+const char kProcessIDParam[] = "p";
+const char kViewIDParam[] = "v";
+
+}  // namespace
+
+SuggestionSource::SuggestionSource() {
+}
+
+SuggestionSource::~SuggestionSource() {
+}
+
+std::string SuggestionSource::GetSource() {
+  return chrome::kChromeSearchSuggestionHost;
+}
+
+void SuggestionSource::StartDataRequest(
+    const std::string& path_and_query,
+    bool is_incognito,
+    const content::URLDataSource::GotDataCallback& callback) {
+  std::string path(GURL(chrome::kChromeSearchSuggestionUrl +
+                        path_and_query).path());
+  if (path == kLoaderHtmlPath)
+    SendResource(IDR_OMNIBOX_RESULT_LOADER_HTML, callback);
+  else if (path == kLoaderJSPath)
+    SendJSWithOrigin(IDR_OMNIBOX_RESULT_LOADER_JS, path_and_query, callback);
+  else if (path == kResultHtmlPath)
+    SendResource(IDR_OMNIBOX_RESULT_HTML, callback);
+  else if (path == kResultJSPath)
+    SendJSWithOrigin(IDR_OMNIBOX_RESULT_JS, path_and_query, callback);
+  else
+    callback.Run(NULL);
+}
+
+void SuggestionSource::SendResource(
+    int resource_id,
+    const content::URLDataSource::GotDataCallback& callback) {
+  scoped_refptr<base::RefCountedStaticMemory> response(
+      ResourceBundle::GetSharedInstance().LoadDataResourceBytes(resource_id));
+  callback.Run(response);
+}
+
+void SuggestionSource::SendJSWithOrigin(
+    int resource_id,
+    const std::string& path_and_query,
+    const content::URLDataSource::GotDataCallback& callback) {
+  // Look up the origin for this request using the process id and render view
+  // id previously added in WillServiceRequest() on the IO thread. We must do
+  // this here because GetOrigin() needs to run on the UI thread.

[sreeram, 2013/04/16 21:23:31]

Does it make sense to add DCHECK(IsOnThread(...)) to all the methods here?

[Jered, 2013/04/18 18:11:48]

I'd really prefer not to.

I have seen this idiom a couple of places in chrome and am completely baffled by
it. It makes code needlessly hard to unit test and buys almost nothing for a
private method in a class with ~7 methods.

+  std::string process_id_str;
+  std::string render_view_id_str;
+  int process_id = -1;
+  int render_view_id = -1;
+  std::string origin;
+  GURL url(chrome::kChromeSearchSuggestionUrl + path_and_query);
+  if (!net::GetValueForKeyInQuery(url, kProcessIDParam, &process_id_str) ||
+      !net::GetValueForKeyInQuery(url, kViewIDParam, &render_view_id_str) ||
+      !base::StringToInt(process_id_str, &process_id) ||
+      !base::StringToInt(render_view_id_str, &render_view_id) ||
+      !GetOrigin(process_id, render_view_id, &origin)) {
+    callback.Run(NULL);
+    return;
+  }
+
+  std::string js_escaped_origin;
+  base::JsonDoubleQuote(origin, false, &js_escaped_origin);
+  base::StringPiece template_js =
+      ResourceBundle::GetSharedInstance().GetRawDataResource(resource_id);
+  std::string response(template_js.as_string());
+  ReplaceFirstSubstringAfterOffset(&response, 0, "{{ORIGIN}}", origin);
+  callback.Run(base::RefCountedString::TakeString(&response));
+}
+
+std::string SuggestionSource::GetMimeType(
+    const std::string& path_and_query) const {
+  std::string path(GURL(chrome::kChromeSearchSuggestionUrl +
+                        path_and_query).path());
+  if (path == kLoaderHtmlPath || path == kResultHtmlPath)
+    return "text/html";
+  if (path == kLoaderJSPath || path == kResultJSPath)
+    return "application/javascript";
+  return "";
+}
+
+bool SuggestionSource::ShouldServiceRequest(
+    const net::URLRequest* request) const {
+  const std::string& path = request->url().path();
+  return InstantIOContext::ShouldServiceRequest(request) &&
+      request->url().SchemeIs(chrome::kChromeSearchScheme) &&
+      request->url().host() == chrome::kChromeSearchSuggestionHost &&
+      (path == kLoaderHtmlPath || path == kLoaderJSPath ||
+       path == kResultHtmlPath || path == kResultJSPath);
+}
+
+void SuggestionSource::WillServiceRequest(
+    const net::URLRequest* request, std::string* path) const {
+  // Overwrite the query string for this request with parameters &p and &v
+  // giving the process id and render view id which generated this URL request.
+  // These will be used to lookup the requestor's origin later.
+  std::string query_str;
+  const content::ResourceRequestInfo* info =
+      content::ResourceRequestInfo::ForRequest(request);
+  if (info) {
+    int process_id = -1;
+    int render_view_id = -1;
+    if (info->GetAssociatedRenderView(&process_id, &render_view_id)) {
+      query_str.append(kProcessIDParam);
+      query_str.append("=");
+      query_str.append(base::IntToString(process_id));
+      query_str.append("&");
+      query_str.append(kViewIDParam);
+      query_str.append("=");
+      query_str.append(base::IntToString(render_view_id));
+    }
+  }
+  // Note that this replaces any existing query string parameters, including
+  // any existing &p and &v parameters.
+  GURL url(chrome::kChromeSearchSuggestionUrl + (*path));
+  GURL::Replacements set_query;
+  set_query.SetQueryStr(query_str);
+  GURL url_with_query(url.ReplaceComponents(set_query));
+  *path = url_with_query.path().substr(1) + "?" + url_with_query.query();
+}
+
+bool SuggestionSource::ShouldDenyXFrameOptions() const {
+  return false;
+}
+
+bool SuggestionSource::GetOrigin(
+    int process_id,
+    int render_view_id,
+    std::string* origin) const {
+  content::RenderViewHost* rvh =
+      content::RenderViewHost::FromID(process_id, render_view_id);
+  if (rvh == NULL)
+    return false;
+  content::WebContents* contents =
+      content::WebContents::FromRenderViewHost(rvh);
+  if (contents == NULL)
+    return false;
+  *origin = contents->GetURL().GetOrigin().spec();

[sreeram, 2013/04/16 21:23:31]

Is it possible for this to be spoofed? Say the search page navigates to
"chrome-search://suggestion/..." (that's correct, it sets location.href to
navigate). But before doing that, it installs an unload handler that takes a
while to execute. Now for some reason (maybe the unload handler adds the loader
iframe), we get here. I assume GetURL() would return the "chrome-search://" URL.
No? (Since the active entry is the pending entry.) I've now lost track of what I
was trying to do, but perhaps it's now possible for the search page to spoof
something?

[sreeram, 2013/04/16 21:28:19]

Don't waste too much time on this. For the page to be able to do anything at
all, the unload handler must yield, at which point, the page will probably have
been considered to be unloaded. So, I don't think there's any path that leads
from an unload handler to a successful attack. I mentioned it in case it sparks
any interesting lines of attack/consideration in your mind.

[Jered, 2013/04/16 23:16:38]

I think WebContents is getting its URL from the navigation controller. I'll play
around with unload and some of the html5 history hooks, but my intuition is that
this will happen before the url is updated and won't get an attacker anything.

[Jered, 2013/04/18 18:11:48]

tl;dr There doesn't seem to be a viable attack here.

The biggest problem would be if you could get your code to run with
SecurityOrigin chrome-search://suggestion, since then you could call the
privileged api or read result iframes. That doesn't seem possible; I checked
that onunload() still has the correct SecurityOrigin.

It would also be mildly problematic if you could get any page to be able to show
suggestions by getting a loader iframe to accept a non-Instant origin. I tried
setting location.href="http://www.example.com" and then adding a loader iframe
in onunload(), but it still has the expected origin of the Instant page, not the
pending page, and I can't do anything with the newly created iframe, because
postMessage is async and I can't cancel the navigation at that point.

I also tried hooking on to onbeforeunload. Surprisingly, Instant doesn't block
the modal dialog, and I can see the iframe element I created. But it is still
set up with the origin of the Instant page.

+  // Origin should not include a trailing slash. That is part of the path.
+  TrimString(*origin, "/", origin);
+  return true;
+}