Support whitelisting to handle insecure origins as trustworthy origins (chromium)

chrome/common/origin_util.cc

Index: chrome/common/origin_util.cc
diff --git a/chrome/common/origin_util.cc b/chrome/common/origin_util.cc
index 2884d74e2b9b9fa97fa803f2b892d8b701743ee7..d05c8363bc06db359309d63fe3efcd928722a6c0 100644
--- a/chrome/common/origin_util.cc
+++ b/chrome/common/origin_util.cc
@@ -4,24 +4,33 @@
 
 #include "chrome/common/origin_util.h"
 
+#include <set>
+
+#include "base/command_line.h"
+#include "base/lazy_instance.h"
+#include "base/stl_util.h"
+#include "base/strings/string_split.h"
+#include "base/threading/thread_local.h"
+#include "chrome/common/chrome_switches.h"
+#include "content/public/common/origin_util.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
-#include "net/base/net_util.h"
 #include "url/gurl.h"
 
-bool IsOriginSecure(const GURL& url) {
-  if (url.SchemeUsesTLS() || url.SchemeIsFile())
-    return true;
+namespace {
 
-  if (url.SchemeIsFileSystem() && url.inner_url() &&
-      IsOriginSecure(*url.inner_url())) {
-    return true;
-  }
+using OriginSet = std::set<GURL>;
 
-  std::string hostname = url.HostNoBrackets();
-  if (net::IsLocalhost(hostname))
+base::LazyInstance<base::ThreadLocalPointer<OriginSet>>::Leaky
+    g_origin_whitelist = LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+bool IsOriginSecure(const GURL& url) {
+  if (content::IsOriginSecure(url))

[palmer, 2015/04/20 22:16:18]

No, please don't add a |content::IsOriginSecure|. When developing this stuff the
past 2 weeks, we had a big discussion about whether there should be an
|IsOriginSecure| in both chrome:: and content::, and the consensus was no. See
https://codereview.chromium.org/1049533002/

[kinuko, 2015/04/21 12:26:07]

Hmm, the discussion there seems very relevant here too.  As SW code needs
IsOriginSecure() in the content level I'm thinking about moving IsOriginSecure()
(or IsOriginTrustworthy()) to //content and adding RegisterSchemeAsSecure() /
RegisterOriginAsSecure() methods exposed by content (chrome will call them to
register chrome-level schemes and whitelists), something similar to what David
suggested in the thread.  Please let me know if this plan doesn't sound good.

[kinuko, 2015/04/21 16:15:48]

Moved this completely to //content (so the name confusion won't happen,
embedders can add additional schemes/origins and can just call the content's
IsOriginSecure)

     return true;
 
+  // Do additional check for chrome schemes.
   std::string scheme = url.scheme();
   if (scheme == content::kChromeUIScheme ||
       scheme == extensions::kExtensionScheme ||
@@ -29,5 +38,41 @@ bool IsOriginSecure(const GURL& url) {
     return true;
   }
 
+  // Do additional check for whitelisted origins.
+  if (ContainsKey(GetWhiteListedSecureOrigins(), url.GetOrigin()))
+    return true;
+
   return false;
 }
+
+const OriginSet& GetWhiteListedSecureOrigins() {

[palmer, 2015/04/20 22:16:18]

|GetWhiteListedSecureOrigins| and |ClearWhiteListedSecureOrigins| could be in
the anonymous namespace? Do they have any callers other than |IsOriginSecure|?
(I haven't read this whole CL yet.)

For that matter, does |ClearWhiteListedSecureOrigins| have or need to have any
callers? As I imagine it, this feature should be very simple: Additional origins
are added on the command line, parsed and inserted into |g_origin_whitelist|,
and then do not change for the lifetime of the Chrome process. Right?

I think for safety reasons it should stay that way, and there should be no
temptation to make this feature dynamic in any way.

[kinuko, 2015/04/21 16:15:48]

This is added only for testing. I renamed this so that it becomes clear (and
presubmit warns) that it's only for testing.

+  OriginSet* whitelist = g_origin_whitelist.Pointer()->Get();
+  if (!whitelist) {
+    whitelist = new OriginSet();
+    g_origin_whitelist.Pointer()->Set(whitelist);
+
+    // If kUnsafetyTreatInsecureOriginAsSecure option is given and
+    // kUserDataDir is present, add the given origins to the blink whitelist
+    // for handling them as trustworthy.
+    const base::CommandLine& command_line =
+        *base::CommandLine::ForCurrentProcess();
+    if (command_line.HasSwitch(
+            switches::kUnsafetyTreatInsecureOriginAsSecure) &&
+        command_line.HasSwitch(switches::kUserDataDir)) {
+      std::vector<std::string> origins;
+      base::SplitString(command_line.GetSwitchValueASCII(
+          switches::kUnsafetyTreatInsecureOriginAsSecure), ',', &origins);
+      for (const std::string& origin : origins)

[palmer, 2015/04/20 22:16:18]

Nit: Maybe you can use auto here?

[kinuko, 2015/04/21 16:15:48]

Done.

+        whitelist->insert(GURL(origin).GetOrigin());
+    }
+  }
+  return *whitelist;
+}
+
+void ClearWhiteListedSecureOrigins() {
+  OriginSet* whitelist = g_origin_whitelist.Pointer()->Get();
+  if (whitelist) {
+    delete whitelist;
+    g_origin_whitelist.Pointer()->Set(nullptr);
+  }
+}