In PopularSites, parse (not sanitize) JSON safely.

chrome/browser/android/ntp/popular_sites.cc

 // Copyright 2015 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/android/ntp/popular_sites.h"
 
 #include <stddef.h>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/important_file_writer.h"
-#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chrome/common/chrome_paths.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/ntp_tiles/pref_names.h"
 #include "components/ntp_tiles/switches.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
-#include "components/safe_json/json_sanitizer.h"
+#include "components/safe_json/safe_json_parser.h"
 #include "components/search_engines/search_engine_type.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/variations/service/variations_service.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 
 using net::URLFetcher;
 using variations::VariationsService;
 
@@ skipping 81 matching lines @@
 
   if (version.empty())
     version = variation_param_version;
 
   if (version.empty())
     version = kPopularSitesDefaultVersion;
 
   return version;
 }
 
-std::unique_ptr<std::vector<PopularSites::Site>> ParseJson(
-    const std::string& json) {
-  std::unique_ptr<base::Value> value =
-      base::JSONReader::Read(json, base::JSON_ALLOW_TRAILING_COMMAS);
-  base::ListValue* list;
-  if (!value || !value->GetAsList(&list)) {
-    DLOG(WARNING) << "Failed parsing json";
-    return nullptr;
-  }
-
-  std::unique_ptr<std::vector<PopularSites::Site>> sites(
-      new std::vector<PopularSites::Site>);
-  for (size_t i = 0; i < list->GetSize(); i++) {
-    base::DictionaryValue* item;
-    if (!list->GetDictionary(i, &item))
-      continue;
-    base::string16 title;
-    std::string url;
-    if (!item->GetString("title", &title) || !item->GetString("url", &url))
-      continue;
-    std::string favicon_url;
-    item->GetString("favicon_url", &favicon_url);
-    std::string thumbnail_url;
-    item->GetString("thumbnail_url", &thumbnail_url);
-    std::string large_icon_url;
-    item->GetString("large_icon_url", &large_icon_url);
-
-    sites->push_back(PopularSites::Site(title, GURL(url), GURL(favicon_url),
-                                        GURL(large_icon_url),
-                                        GURL(thumbnail_url)));
-  }
-
-  return sites;
+// Must run on the blocking thread pool.
+bool WriteJson(const base::FilePath& local_path, const base::Value* json) {

[Marc Treib, 2016/06/02 11:39:55]

nit: WriteJsonToFile?

[sfiera, 2016/06/02 12:44:19]

Done.

+  std::string json_string;
+  return base::JSONWriter::Write(*json, &json_string) &&
+         base::ImportantFileWriter::WriteFileAtomically(local_path,
+                                                        json_string);
 }
 
 }  // namespace
 
 base::FilePath ChromePopularSites::GetDirectory() {
   base::FilePath dir;
   PathService::Get(chrome::DIR_USER_DATA, &dir);
   return dir;  // empty if PathService::Get() failed.
 }
 
@@ skipping 138 matching lines @@
 GURL PopularSites::GetPopularSitesURL() const {
   return GURL(base::StringPrintf(kPopularSitesURLFormat,
                                  pending_country_.c_str(),
                                  pending_version_.c_str()));
 }
 
 void PopularSites::OnReadFileDone(const GURL& url,
                                   std::unique_ptr<std::string> data,
                                   bool success) {
   if (success) {
-    ParseSiteList(*data);
+    safe_json::SafeJsonParser::Parse(

[Marc Treib, 2016/06/02 11:39:54]

I might be missing something, but why do we need the expensive "safe" json
parsing here? IIRC, the point of the Sanitizer thing was that we do the
expensive out-of-process parsing only once after we first download the json, and
then store the known-good json to a file, so on subsequent loads, we can use
regular in-process parsing.

[sfiera, 2016/06/02 12:44:19]

We don't have to, but it was easier. I changed it.

I still don't really understand what threat safe_json is supposed to mitigate,
so I don't know when we should skip it.

[Marc Treib, 2016/06/02 13:00:39]

Well, the threat of malicious json, which might trigger a vulnerability in the
json parser. So after the json has been sanitized once, there's no point in
going through the exercise again. (IIUC - as I said, I might be missing
something. Maybe it'd be best if Bernhard took a look too.)

+        *data,
+        base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr(),
+                   false /* write_to_file */),
+        base::Bind(&PopularSites::OnJsonParseFailed,
+                   weak_ptr_factory_.GetWeakPtr()));
   } else {
     // File didn't exist, or couldn't be read for some other reason.
     FetchPopularSites(url);
   }
 }
 
 void PopularSites::FetchPopularSites(const GURL& url) {
   fetcher_ = URLFetcher::Create(url, URLFetcher::GET, this);
   fetcher_->SetRequestContext(download_context_);
   fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                          net::LOAD_DO_NOT_SAVE_COOKIES);
   fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
   fetcher_->Start();
 }
 
 void PopularSites::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK_EQ(fetcher_.get(), source);
   std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_);
 
-  std::string sketchy_json;
+  std::string json_string;
   if (!(source->GetStatus().is_success() &&
         source->GetResponseCode() == net::HTTP_OK &&
-        source->GetResponseAsString(&sketchy_json))) {
+        source->GetResponseAsString(&json_string))) {
     OnDownloadFailed();
     return;
   }
 
-  safe_json::JsonSanitizer::Sanitize(
-      sketchy_json, base::Bind(&PopularSites::OnJsonSanitized,
-                               weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&PopularSites::OnJsonSanitizationFailed,
+  safe_json::SafeJsonParser::Parse(
+      json_string,
+      base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr(),
+                 true /* write_to_file */),
+      base::Bind(&PopularSites::OnJsonParseFailed,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PopularSites::OnJsonSanitized(const std::string& valid_minified_json) {
-  base::PostTaskAndReplyWithResult(
-      blocking_runner_.get(), FROM_HERE,
-      base::Bind(&base::ImportantFileWriter::WriteFileAtomically, local_path_,
-                 valid_minified_json),
-      base::Bind(&PopularSites::OnFileWriteDone, weak_ptr_factory_.GetWeakPtr(),
-                 valid_minified_json));
+void PopularSites::OnJsonParsed(bool write_to_file,
+                                std::unique_ptr<base::Value> json) {
+  if (write_to_file) {
+    const base::Value* json_ptr = json.get();
+    base::PostTaskAndReplyWithResult(
+        blocking_runner_.get(), FROM_HERE,
+        base::Bind(&WriteJson, local_path_, json_ptr),
+        base::Bind(&PopularSites::OnFileWriteDone,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   base::Passed(std::move(json))));
+  } else {
+    ParseSiteList(std::move(json));
+  }
 }
 
-void PopularSites::OnJsonSanitizationFailed(const std::string& error_message) {
-  DLOG(WARNING) << "JSON sanitization failed: " << error_message;
+void PopularSites::OnJsonParseFailed(const std::string& error_message) {
+  DLOG(WARNING) << "JSON parsing failed: " << error_message;
   OnDownloadFailed();
 }
 
-void PopularSites::OnFileWriteDone(const std::string& json, bool success) {
+void PopularSites::OnFileWriteDone(std::unique_ptr<base::Value> json,
+                                   bool success) {
   if (success) {
     prefs_->SetInt64(kPopularSitesLastDownloadPref,
                      base::Time::Now().ToInternalValue());
     prefs_->SetString(kPopularSitesCountryPref, pending_country_);
     prefs_->SetString(kPopularSitesVersionPref, pending_version_);
-    ParseSiteList(json);
+    ParseSiteList(std::move(json));
   } else {
     DLOG(WARNING) << "Could not write file to "
                   << local_path_.LossyDisplayName();
     OnDownloadFailed();
   }
 }
 
-void PopularSites::ParseSiteList(const std::string& json) {
-  base::PostTaskAndReplyWithResult(
-      blocking_runner_.get(), FROM_HERE, base::Bind(&ParseJson, json),
-      base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
-}
+void PopularSites::ParseSiteList(std::unique_ptr<base::Value> json) {
+  base::ListValue* list;

[Marc Treib, 2016/06/02 11:39:55]

nit: initialize to nullptr?

[sfiera, 2016/06/02 12:44:19]

Sure, it's your code :) I just moved it.

[Marc Treib, 2016/06/02 13:00:39]

Haha, alright. I guess my reviewer was lazy (or less picky) :D

+  if (!json || !json->GetAsList(&list)) {
+    DLOG(WARNING) << "JSON is not a list";
+    sites_.clear();
+    callback_.Run(false);
+    return;
+  }
 
-void PopularSites::OnJsonParsed(std::unique_ptr<std::vector<Site>> sites) {
-  if (sites)
-    sites_.swap(*sites);
-  else
-    sites_.clear();
-  callback_.Run(!!sites);
+  std::vector<PopularSites::Site> sites;
+  for (size_t i = 0; i < list->GetSize(); i++) {
+    base::DictionaryValue* item;
+    if (!list->GetDictionary(i, &item))
+      continue;
+    base::string16 title;
+    std::string url;
+    if (!item->GetString("title", &title) || !item->GetString("url", &url))
+      continue;
+    std::string favicon_url;
+    item->GetString("favicon_url", &favicon_url);
+    std::string thumbnail_url;
+    item->GetString("thumbnail_url", &thumbnail_url);
+    std::string large_icon_url;
+    item->GetString("large_icon_url", &large_icon_url);
+
+    sites.push_back(PopularSites::Site(title, GURL(url), GURL(favicon_url),
+                                       GURL(large_icon_url),
+                                       GURL(thumbnail_url)));
+  }
+
+  sites_.swap(sites);
+  callback_.Run(true);
 }
 
 void PopularSites::OnDownloadFailed() {
   if (!is_fallback_) {
     DLOG(WARNING) << "Download country site list failed";
     is_fallback_ = true;
     pending_country_ = kPopularSitesDefaultCountryCode;
     pending_version_ = kPopularSitesDefaultVersion;
     FetchPopularSites(GetPopularSitesURL());
   } else {
     DLOG(WARNING) << "Download fallback site list failed";
     callback_.Run(false);
   }
 }