Network service: Safe browsing check for sub-resources from renderer.

chrome/browser/safe_browsing/safe_browsing_impl.cc

+// Copyright 2017 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/safe_browsing/safe_browsing_impl.h"
+
+#include <vector>
+
+#include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/timer/timer.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/prerender/prerender_contents.h"
+#include "chrome/browser/safe_browsing/ui_manager.h"
+#include "components/safe_browsing_db/database_manager.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/resource_type.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "net/base/load_flags.h"
+
+namespace safe_browsing {
+namespace {
+
+// TODO(yzshen): Share such value with safe_browsing::BaseResourceThrottle.

[Nathan Parker, 2017/05/31 17:17:06]

nit: Can you add a class-level or file-level comment to this and other new
files?  There are now lots of files/classes named
(SafeBrowsing).*(Url).*(Check).* and it's important to make it clear what
they're used for.  Thanks.

[yzshen1, 2017/05/31 23:34:44]

Done.

+// Maximum time in milliseconds to wait for the safe browsing service to
+// verify a URL. After this amount of time the outstanding check will be

[vakh (use Gerrit instead), 2017/05/31 23:53:08]

SafeBrowsing service also checks things that are not URLs. Might be better
worded as:
Maximum time in milliseconds to wait for the SafeBrowsing service reputation
check.

[yzshen1, 2017/06/01 17:45:04]

Done.

+// aborted, and the URL will be treated as if it were safe.

[vakh (use Gerrit instead), 2017/05/31 23:53:08]

s/URL/resource

[yzshen1, 2017/06/01 17:45:03]

Done.

+const int kCheckUrlTimeoutMs = 5000;
+
+content::WebContents* GetWebContentsFromID(int render_process_id,
+                                           int render_frame_id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  content::RenderFrameHost* render_frame_host =
+      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
+  if (!render_frame_host)
+    return nullptr;
+
+  return content::WebContents::FromRenderFrameHost(render_frame_host);
+}
+
+// TODO(yzshen): Handle the case where safe browsing is not enabled, or
+// !database_manager()->IsSupported().
+// TODO(yzshen): Make sure it also works on Andorid.
+// TODO(yzshen): Do all the logging like what BaseResourceThrottle does.
+class SafeBrowsingUrlCheckerImpl : public chrome::mojom::SafeBrowsingUrlChecker,
+                                   public SafeBrowsingDatabaseManager::Client {
+ public:
+  SafeBrowsingUrlCheckerImpl(
+      int load_flags,
+      content::ResourceType resource_type,
+      scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
+      scoped_refptr<SafeBrowsingUIManager> ui_manager,
+      int render_process_id,
+      int render_frame_id)
+      : load_flags_(load_flags),
+        resource_type_(resource_type),
+        render_process_id_(render_process_id),
+        render_frame_id_(render_frame_id),
+        database_manager_(std::move(database_manager)),
+        ui_manager_(std::move(ui_manager)),
+        weak_factory_(this) {
+    DCHECK_NE(resource_type, content::RESOURCE_TYPE_MAIN_FRAME);
+    DCHECK_NE(resource_type, content::RESOURCE_TYPE_SUB_FRAME);
+  }
+
+  ~SafeBrowsingUrlCheckerImpl() override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+    if (state_ == STATE_CHECKING_URL)
+      database_manager_->CancelCheck(this);
+
+    for (size_t i = next_index_; i < callbacks_.size(); ++i)
+      std::move(callbacks_[i]).Run(false);
+  }
+
+  // chrome::mojom::SafeBrowsingUrlChecker implementation.
+  void CheckUrl(const GURL& url, CheckUrlCallback callback) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+    DVLOG(1) << "SafeBrowsingUrlCheckerImpl checks URL: " << url;
+    urls_.push_back(url);
+    callbacks_.push_back(std::move(callback));
+
+    ProcessUrls();
+  }
+
+ private:
+  // SafeBrowsingDatabaseManager::Client implementation:
+  void OnCheckBrowseUrlResult(const GURL& url,
+                              SBThreatType threat_type,
+                              const ThreatMetadata& metadata) override {
+    DCHECK_EQ(STATE_CHECKING_URL, state_);
+    DCHECK_LT(next_index_, urls_.size());
+    DCHECK_EQ(urls_[next_index_], url);
+
+    timer_.Stop();
+    if (threat_type == SB_THREAT_TYPE_SAFE) {
+      state_ = STATE_NONE;
+      std::move(callbacks_[next_index_]).Run(true);
+      next_index_++;
+      ProcessUrls();
+      return;
+    }
+
+    if (load_flags_ & net::LOAD_PREFETCH) {
+      BlockAndProcessUrls();
+      return;
+    }
+
+    security_interstitials::UnsafeResource resource;
+    resource.url = url;
+    resource.original_url = urls_[0];
+    if (urls_.size() > 1)
+      resource.redirect_urls =

[Nathan Parker, 2017/05/31 17:17:06]

I may be forgetting a detail of the design plan, but is the plan to eventually
replace the SafeBrowsingResourceThrottle code with this code?  Duplicating that
code here will likely lead to them diverging over time, which would mean
inconsistent results.

[yzshen1, 2017/05/31 23:34:43]

Yes. Eventually when the network service is enabled by default,
SafeBrowsingResourceThrottle will go away.

The renderer-initiated code path is quite different from
SafeBrowsingResourceThrottle: the throttling logic is now split into two parts:
SafeBrowsingURLLoaderThrottle at the renderer side and this class at the
browsing side. Besides, the state machine is also different. The new code path
doesn't wait for SB confirmation before writing disk cache or following
redirects. (Instead, if the SB check says "no", the response won't be given to
the consumer and the disk cache entry is removed.)

So I think at the moment, it won't gain us much trying to share code between
this and SafeBrowsingResourceThrottle.

In my next CL, I am going to support browser-initiated SB check for network
service. There are two alternatives to share code:
(1) share code between the new-browser-side-code-path and
SafeBrowsingResourceThrottle;
(2) share code between the new-renderer-side-code-path and
new-browser-side-code-path.

I am thinking to take approach (2), so that the browser-side and renderer-side
checks are consistent in the new world.
We could also consider refactoring SafeBrowsingResourceThrottle to use the
new-browser-side-code-path if we take approach (2), as a follow-up work item.
That way, the new code path can be shipped to our end users before network
service is fully ready.

WDYT? Thanks!

[Nathan Parker, 2017/06/01 18:22:03]

Thanks.  I think #2 is good. I'm mostly concerned about long-term code sharing
and understandability.

+          std::vector<GURL>(urls_.begin() + 1, urls_.end());
+    resource.is_subresource = true;
+    resource.is_subframe = false;
+    resource.threat_type = threat_type;
+    resource.threat_metadata = metadata;
+    resource.callback =
+        base::Bind(&SafeBrowsingUrlCheckerImpl::OnBlockingPageComplete,
+                   weak_factory_.GetWeakPtr());
+    resource.callback_thread = content::BrowserThread::GetTaskRunnerForThread(
+        content::BrowserThread::IO);
+    resource.web_contents_getter =
+        base::Bind(&GetWebContentsFromID, render_process_id_, render_frame_id_);
+    resource.threat_source = database_manager_->GetThreatSource();
+
+    state_ = STATE_DISPLAYING_BLOCKING_PAGE;
+
+    content::BrowserThread::PostTask(
+        content::BrowserThread::UI, FROM_HERE,
+        base::Bind(&SafeBrowsingUrlCheckerImpl::StartDisplayingBlockingPage,
+                   weak_factory_.GetWeakPtr(), ui_manager_, resource));
+  }
+
+  static void StartDisplayingBlockingPage(
+      const base::WeakPtr<SafeBrowsingUrlCheckerImpl>& checker,
+      scoped_refptr<BaseUIManager> ui_manager,
+      const security_interstitials::UnsafeResource& resource) {
+    content::WebContents* web_contents = resource.web_contents_getter.Run();
+    if (web_contents) {
+      prerender::PrerenderContents* prerender_contents =
+          prerender::PrerenderContents::FromWebContents(web_contents);
+      if (prerender_contents) {
+        prerender_contents->Destroy(prerender::FINAL_STATUS_SAFE_BROWSING);
+      } else {
+        ui_manager->DisplayBlockingPage(resource);
+        return;
+      }
+    }
+
+    // Tab is gone or it's being prerendered.
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO, FROM_HERE,
+        base::BindOnce(&SafeBrowsingUrlCheckerImpl::BlockAndProcessUrls,
+                       checker));
+  }
+
+  void OnCheckUrlTimeout() {
+    database_manager_->CancelCheck(this);
+
+    OnCheckBrowseUrlResult(urls_[next_index_],
+                           safe_browsing::SB_THREAT_TYPE_SAFE,
+                           ThreatMetadata());
+  }
+
+  void ProcessUrls() {
+    if (state_ == STATE_CHECKING_URL ||
+        state_ == STATE_DISPLAYING_BLOCKING_PAGE) {
+      return;
+    }
+
+    while (next_index_ < urls_.size()) {
+      if (state_ == STATE_BLOCKED) {
+        std::move(callbacks_[next_index_]).Run(false);

[Nathan Parker, 2017/05/31 17:17:06]

Add comment (assuming this is correct): "If user decided to not proceed through
a warning, mark all the remaining redirects as "bad."

Actually, how about moving this to BlockAndProcessUrls() (instead of calling
ProcessUrls there) since that's the only place it could be come STATE_BLOCKED?
That'd simplify the apparent possible state transitions.

[yzshen1, 2017/05/31 23:34:44]

Done.

+        next_index_++;
+        continue;
+      }
+
+      DCHECK_EQ(STATE_NONE, state_);
+      // TODO(yzshen): Consider moving CanCheckResourceType() to the renderer
+      // side. That would save some IPCs.
+      if (!database_manager_->CanCheckResourceType(resource_type_) ||
+          database_manager_->CheckBrowseUrl(urls_[next_index_], this)) {
+        std::move(callbacks_[next_index_]).Run(true);
+        next_index_++;
+        continue;
+      }
+
+      state_ = STATE_CHECKING_URL;
+      // Start a timer to abort the check if it takes too long.
+      timer_.Start(FROM_HERE,
+                   base::TimeDelta::FromMilliseconds(kCheckUrlTimeoutMs), this,
+                   &SafeBrowsingUrlCheckerImpl::OnCheckUrlTimeout);
+
+      break;
+    }
+  }
+
+  void BlockAndProcessUrls() {
+    DVLOG(1) << "SafeBrowsingUrlCheckerImpl blocks URL: " << urls_[next_index_];
+    state_ = STATE_BLOCKED;
+    ProcessUrls();
+  }
+
+  void OnBlockingPageComplete(bool proceed) {
+    DCHECK_EQ(STATE_DISPLAYING_BLOCKING_PAGE, state_);
+
+    if (proceed) {
+      state_ = STATE_NONE;
+      std::move(callbacks_[next_index_]).Run(true);
+      next_index_++;
+      ProcessUrls();
+    } else {
+      BlockAndProcessUrls();
+    }
+  }
+
+  enum State {
+    // Haven't started checking or checking is complete.
+    STATE_NONE,
+    // We have one outstanding URL-check.
+    STATE_CHECKING_URL,
+    // We're displaying a blocking page.
+    STATE_DISPLAYING_BLOCKING_PAGE,
+    // The blocking page has returned *not* to proceed.
+    STATE_BLOCKED
+  };
+
+  const int load_flags_;
+  const content::ResourceType resource_type_;
+  const int render_process_id_;
+  const int render_frame_id_;
+  scoped_refptr<SafeBrowsingDatabaseManager> database_manager_;
+  scoped_refptr<BaseUIManager> ui_manager_;
+
+  // The redirect chain for this resource, including the original URL and
+  // subsequent redirect URLs.
+  std::vector<GURL> urls_;

[Nathan Parker, 2017/05/31 17:17:06]

Q: Are the callbacks different for each url?  It seems like the action taken
would be the same regardless of which URL was found to be bad.

[yzshen1, 2017/05/31 23:34:44]

Each mojo call has a separate base::OnceCallback instance. Each must be called
exactly once unless the interface is disconnected. That is the requirement of
Mojo.

I agree that the action taken at the renderer side is actually the same.

If we would like to use the same "callback" for multiple times. We will need to
use another mojo interface, whose implementation is at the renderer side. The
browser side can then make calls on that interface to notify the results.

+  std::vector<CheckUrlCallback> callbacks_;
+  size_t next_index_ = 0;

[Nathan Parker, 2017/05/31 17:17:06]

Nit: add comment about what next_index refers to.  AFAIKT, urls_ and callbacks_
before next_index are no longer relevant/valid, and those at next_index_ are yet
to be checked. ya?

[yzshen1, 2017/05/31 23:34:44]

Updated with comment. Thanks!

+
+  State state_ = STATE_NONE;
+
+  // Timer to abort the safe browsing check if it takes too long.
+  base::OneShotTimer timer_;
+
+  base::WeakPtrFactory<SafeBrowsingUrlCheckerImpl> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SafeBrowsingUrlCheckerImpl);
+};
+
+}  // namespace
+
+SafeBrowsingImpl::SafeBrowsingImpl(
+    scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
+    scoped_refptr<SafeBrowsingUIManager> ui_manager,
+    int render_process_id)
+    : database_manager_(std::move(database_manager)),
+      ui_manager_(std::move(ui_manager)),
+      render_process_id_(render_process_id) {}
+
+SafeBrowsingImpl::~SafeBrowsingImpl() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+}
+
+// static
+void SafeBrowsingImpl::Create(
+    scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
+    scoped_refptr<SafeBrowsingUIManager> ui_manager,
+    int render_process_id,
+    const service_manager::BindSourceInfo& source_info,
+    chrome::mojom::SafeBrowsingRequest request) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  mojo::MakeStrongBinding(base::MakeUnique<SafeBrowsingImpl>(
+                              std::move(database_manager),
+                              std::move(ui_manager), render_process_id),
+                          std::move(request));
+}
+
+void SafeBrowsingImpl::CreateCheckerAndCheck(
+    int32_t render_frame_id,
+    chrome::mojom::SafeBrowsingUrlCheckerRequest request,
+    const GURL& url,
+    int32_t load_flags,
+    content::ResourceType resource_type,
+    CreateCheckerAndCheckCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  auto checker_impl = base::MakeUnique<SafeBrowsingUrlCheckerImpl>(
+      load_flags, resource_type, database_manager_, ui_manager_,
+      render_process_id_, render_frame_id);
+  checker_impl->CheckUrl(url, std::move(callback));
+  mojo::MakeStrongBinding(std::move(checker_impl), std::move(request));
+}
+
+}  // namespace safe_browsing