Componentize renderer side of DNS prefetching.

chrome/browser/net/predictor.cc

Index: chrome/browser/net/predictor.cc
diff --git a/chrome/browser/net/predictor.cc b/chrome/browser/net/predictor.cc
index 9d2a80c98afaace21cf34892d7d8201f97969da6..2cd513fbf15d9cd14ec7fed56a15db65d6efc050 100644
--- a/chrome/browser/net/predictor.cc
+++ b/chrome/browser/net/predictor.cc
@@ -33,6 +33,7 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h"
+#include "components/predictor/common/predictor_common.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/base/address_list.h"
@@ -736,12 +737,23 @@ void Predictor::LearnAboutInitialNavigation(const GURL& url) {
 // includes both Page-Scan, and Link-Hover prefetching.
 // TODO(jar): Separate out link-hover prefetching, and page-scan results.
 void Predictor::DnsPrefetchList(const NameList& hostnames) {
+  if (hostnames.size() > predictor::kMaxDnsHostnamesPerRequest) {
+    LOG(ERROR) << "Refusing to prefetch too many hostnames: "
+               << hostnames.size();

[Nico, 2014/10/28 20:07:19]

Don't do useless logging. If this is an invariant, CHECK() for it. If it isn't,
just return.

As is, this will produce log spam for users if it fails without us ever doing
anything about it since "just fixing some logspam" won't be considered high
priority.

[gunsch, 2014/10/28 20:47:32]

If it fails, that's a security issue, since this should never happen in practice
(and is enforced on the renderer side). But per palmer's link
(http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc):

"When validating untrustworthy input, don't simply use CHECK. We do not want the
input validation mechanism to become an easy way for a malicious renderer to
kill the browser process. It's usually better to ignore the bad input.".

So I don't think this is actually "logspam", nor do I think a CHECK is
appropriate. I can upgrade to a DFATAL to enforce the "invariant" aspect,
however.

[Nico, 2014/10/28 20:56:42]

It's logspam in the sense that printing this doesn't help either the user, nor
us. If this can happen with a compromised renderer, the validation should be
done in the ipc layer which has code for killing renderers that send invalid
values.

[gunsch, 2014/10/28 21:11:29]

That sounds practical, can you point me to an example or docs on the subject? I
haven't heard of this before, and can't find information on this in the public
docs nor examples in similar IPC messages.

[Nico, 2014/10/28 21:31:18]

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

This is called from
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel_pr...
, and this is what calls set_dispatch_error() on an ipc::Message:
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ma...

This in turn is called if the Read() call in Dispatch() from the respective
message class fails (e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_message_ma...
)

You configure this via ipc traits, for example to set the max value on an enum:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

Clicking "ParamTraits" on
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_param_trai...
and looking at the "references" list that cs opens at the bottom should show you
a bunch of examples.

[Nico, 2014/10/29 22:16:34]

Even better, we even have a web page for this:
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

[gunsch, 2014/10/29 23:58:29]

Yeah, I linked that earlier in this reply thread, it doesn't have any
information about doing custom IPC validation that aren't already covered by the
existing macros. I've taken a stab at making my own ParamTraits here, let me
know what you think.

+    return;
+  }
+
   // TODO(jar): Push GURL transport further back into renderer, but this will
   // require a Webkit change in the observer :-/.
   UrlList urls;
   for (NameList::const_iterator it = hostnames.begin();
        it < hostnames.end();
        ++it) {
+    if (it->length() > predictor::kMaxDnsHostnameLength) {
+      LOG(ERROR) << "Refusing to prefetch hostname with length: "
+                 << it->length();

[Nico, 2014/10/28 20:07:19]

likewise

[gunsch, 2014/10/29 23:58:29]

Done.

+      continue;
+    }
     urls.push_back(GURL("http://" + *it + ":80"));
   }