Better enforce same origin policy in iOS reader mode.

Currently on iOS, DOM Distiller cannot guarantee that the data being
sent to it comes from the distillation script. This allows arbitrary
HTML to be included in the final reader mode version of the page. This
combined with a find-replace-style inclusion of arbitrary origin data
sourced from image tags, allows the leaking of cross-origin
authenticated pages.

This change adds a CSP policy for iOS reader mode, enforces paginations
belong to the same origins, includes offline images through JavaScript,
and performs mime-sniffing on offlined images to ensure they are valid
images.

Bug: 1111239
Test: Tested changes with cases provided in bug.
Change-Id: Idf9c8986c541bcab32fb8d320ebdf75b55dc7839
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2608284
Commit-Queue: Alison Huffman <ahuffman@microsoft.com>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845045}

components/dom_distiller/content/browser/dom_distiller_viewer_source.cc

@@ -255,9 +255,12 @@ void DomDistillerViewerSource::StartDataRequest(
       web_contents->GetContainerBounds().size());
 
   GURL current_url(url_utils::GetOriginalUrlFromDistillerUrl(request_url));
+
+  // Pass an empty nonce value as the CSP is only inlined on the iOS build.
   std::string unsafe_page_html = viewer::GetArticleTemplateHtml(
       dom_distiller_service_->GetDistilledPagePrefs()->GetTheme(),
-      dom_distiller_service_->GetDistilledPagePrefs()->GetFontFamily());
+      dom_distiller_service_->GetDistilledPagePrefs()->GetFontFamily(),
+      std::string());
 
   if (viewer_handle) {
     // The service returned a |ViewerHandle| and guarantees it will call

components/dom_distiller/content/browser/test/test_util.cc

@@ -86,8 +86,8 @@ void FakeDistilledPage::Load(EmbeddedTestServer* server,
 }
 
 std::string FakeDistilledPage::GetPageHtmlWithScripts() {
-  std::string html = GetArticleTemplateHtml(mojom::Theme::kLight,
-                                            mojom::FontFamily::kSansSerif);
+  std::string html = GetArticleTemplateHtml(
+      mojom::Theme::kLight, mojom::FontFamily::kSansSerif, std::string());
   for (const std::string& file : scripts_) {
     StrAppend(&html, {JsReplace("<script src=$1></script>", file)});
   }

components/dom_distiller/core/distiller.cc

@@ -253,20 +253,22 @@ void DistillerImpl::OnPageDistillationFinished(
       GURL next_page_url(pagination_info.next_page());
       if (next_page_url.is_valid()) {
         // The pages should be in same origin.
-        DCHECK_EQ(next_page_url.GetOrigin(), page_url.GetOrigin());
-        AddToDistillationQueue(page_num + 1, next_page_url);
-        page_data->distilled_page_proto->data.mutable_pagination_info()
-            ->set_next_page(next_page_url.spec());
+        if (next_page_url.GetOrigin() == page_url.GetOrigin()) {
+          AddToDistillationQueue(page_num + 1, next_page_url);
+          page_data->distilled_page_proto->data.mutable_pagination_info()
+              ->set_next_page(next_page_url.spec());
+        }
       }
     }
 
     if (pagination_info.has_prev_page()) {
       GURL prev_page_url(pagination_info.prev_page());
       if (prev_page_url.is_valid()) {
-        DCHECK_EQ(prev_page_url.GetOrigin(), page_url.GetOrigin());
-        AddToDistillationQueue(page_num - 1, prev_page_url);
-        page_data->distilled_page_proto->data.mutable_pagination_info()
-            ->set_prev_page(prev_page_url.spec());
+        if (prev_page_url.GetOrigin() == page_url.GetOrigin()) {
+          AddToDistillationQueue(page_num - 1, prev_page_url);
+          page_data->distilled_page_proto->data.mutable_pagination_info()
+              ->set_prev_page(prev_page_url.spec());
+        }
       }
     }
 

components/dom_distiller/core/distiller_unittest.cc

@@ -47,6 +47,7 @@ namespace {
 const char kTitle[] = "Title";
 const char kContent[] = "Content";
 const char kURL[] = "http://a.com/";
+const char kOtherURL[] = "http://b.com/";
 const size_t kTotalGoodImages = 2;
 const size_t kTotalImages = 3;
 // Good images need to be in the front.
@@ -193,14 +194,16 @@ void VerifyArticleProtoMatchesMultipageData(
     const dom_distiller::DistilledArticleProto* article_proto,
     const MultipageDistillerData* distiller_data,
     size_t distilled_pages_size,
-    size_t total_pages_size) {
+    size_t total_pages_size,
+    size_t start_page_offset = 0) {
   ASSERT_EQ(distilled_pages_size,
             static_cast<size_t>(article_proto->pages_size()));
   EXPECT_EQ(kTitle, article_proto->title());
   std::string url_prefix = kURL;
-  for (size_t page_num = 0; page_num < distilled_pages_size; ++page_num) {
+  for (size_t page_num = start_page_offset; page_num < distilled_pages_size;
+       ++page_num) {
     const dom_distiller::DistilledPageProto& page =
-        article_proto->pages(page_num);
+        article_proto->pages(page_num - start_page_offset);
     EXPECT_EQ(distiller_data->content[page_num], page.html());
     EXPECT_EQ(distiller_data->page_urls[page_num], page.url());
     EXPECT_EQ(distiller_data->image_ids[page_num].size(),
@@ -217,10 +220,16 @@ void VerifyArticleProtoMatchesMultipageData(
       EXPECT_EQ(GetImageName(page_num + 1, img_num),
                 page.image(img_num).name());
     }
+
     std::string expected_next_page_url =
         GenerateNextPageUrl(url_prefix, page_num, total_pages_size);
-    std::string expected_prev_page_url =
-        GeneratePrevPageUrl(url_prefix, page_num);
+
+    std::string expected_prev_page_url;
+
+    if (page_num > start_page_offset) {
+      expected_prev_page_url = GeneratePrevPageUrl(url_prefix, page_num);
+    }
+
     EXPECT_EQ(expected_next_page_url, page.pagination_info().next_page());
     EXPECT_EQ(expected_prev_page_url, page.pagination_info().prev_page());
     EXPECT_FALSE(page.pagination_info().has_canonical_page());
@@ -337,16 +346,13 @@ std::unique_ptr<DistillerPage> CreateMockDistillerPageWithPendingJSCallback(
   return std::unique_ptr<DistillerPage>(distiller_page);
 }
 
-std::unique_ptr<DistillerPage> CreateMockDistillerPages(
+std::unique_ptr<DistillerPage> CreateMockDistillerPagesWithSequence(
     MultipageDistillerData* distiller_data,
-    size_t pages_size,
-    int start_page_num) {
+    const std::vector<int>& page_num_sequence) {
   MockDistillerPage* distiller_page = new MockDistillerPage();
   {
     testing::InSequence s;
-    std::vector<int> page_nums = GetPagesInSequence(start_page_num, pages_size);
-    for (size_t page_num = 0; page_num < pages_size; ++page_num) {
-      int page = page_nums[page_num];
+    for (int page : page_num_sequence) {
       GURL url = GURL(distiller_data->page_urls[page]);
       EXPECT_CALL(*distiller_page, DistillPageImpl(url, _))
           .WillOnce(DistillerPageOnDistillationDone(
@@ -357,6 +363,14 @@ std::unique_ptr<DistillerPage> CreateMockDistillerPages(
   return std::unique_ptr<DistillerPage>(distiller_page);
 }
 
+std::unique_ptr<DistillerPage> CreateMockDistillerPages(
+    MultipageDistillerData* distiller_data,
+    size_t pages_size,
+    int start_page_num) {
+  std::vector<int> page_nums = GetPagesInSequence(start_page_num, pages_size);
+  return CreateMockDistillerPagesWithSequence(distiller_data, page_nums);
+}
+
 TEST_F(DistillerTest, DistillPage) {
   base::Value result = CreateDistilledValueReturnedFromJS(
       kTitle, kContent, std::vector<int>(), "");
@@ -635,6 +649,66 @@ TEST_F(DistillerTest, DistillMultiplePagesSecondEmpty) {
       article_proto_.get(), distiller_data.get(), kNumPages, kNumPages);
 }
 
+TEST_F(DistillerTest, DistillMultiplePagesNextDifferingOrigin) {
+  const size_t kNumPages = 8;
+  const size_t kActualPages = 4;
+  std::unique_ptr<MultipageDistillerData> distiller_data =
+      CreateMultipageDistillerDataWithoutImages(kNumPages);
+
+  // The next page came from a different origin. All pages after
+  // it will be dropped as well.
+  const size_t target_page_num = 3;
+  distiller_data->content[target_page_num] = kContent;
+  base::Value distilled_value = CreateDistilledValueReturnedFromJS(
+      kTitle, kContent, std::vector<int>(),
+      GenerateNextPageUrl(kOtherURL, target_page_num, kNumPages),
+      GeneratePrevPageUrl(kURL, target_page_num));
+  // Reset distilled data of the second page.
+  distiller_data->distilled_values[target_page_num] =
+      std::make_unique<base::Value>(std::move(distilled_value));
+
+  distiller_ = std::make_unique<DistillerImpl>(url_fetcher_factory_,
+                                               DomDistillerOptions());
+
+  DistillPage(distiller_data->page_urls[0],
+              CreateMockDistillerPages(distiller_data.get(), kActualPages, 0));
+  base::RunLoop().RunUntilIdle();
+
+  VerifyArticleProtoMatchesMultipageData(
+      article_proto_.get(), distiller_data.get(), kActualPages, kActualPages);
+}
+
+TEST_F(DistillerTest, DistillMultiplePagesPrevDifferingOrigin) {
+  const size_t kNumPages = 8;
+  const size_t kActualPages = 6;
+  std::vector<int> page_num_seq{3, 2, 4, 5, 6, 7};
+  std::unique_ptr<MultipageDistillerData> distiller_data =
+      CreateMultipageDistillerDataWithoutImages(kNumPages);
+
+  // The prev page came from a different origin. All pages before
+  // it will be dropped.
+  const size_t target_page_num = 2;
+  distiller_data->content[target_page_num] = kContent;
+  base::Value distilled_value = CreateDistilledValueReturnedFromJS(
+      kTitle, kContent, std::vector<int>(),
+      GenerateNextPageUrl(kURL, target_page_num, kNumPages),
+      GeneratePrevPageUrl(kOtherURL, target_page_num));
+  // Reset distilled data of the second page.
+  distiller_data->distilled_values[target_page_num] =
+      std::make_unique<base::Value>(std::move(distilled_value));
+
+  distiller_ = std::make_unique<DistillerImpl>(url_fetcher_factory_,
+                                               DomDistillerOptions());
+  DistillPage(
+      distiller_data->page_urls[target_page_num + 1],
+      CreateMockDistillerPagesWithSequence(distiller_data.get(), page_num_seq));
+  base::RunLoop().RunUntilIdle();
+
+  VerifyArticleProtoMatchesMultipageData(article_proto_.get(),
+                                         distiller_data.get(), kActualPages,
+                                         kNumPages, target_page_num);
+}
+
 TEST_F(DistillerTest, DistillPreviousPage) {
   const size_t kNumPages = 8;
 

components/dom_distiller/core/html/dom_distiller_viewer.html

@@ -9,13 +9,15 @@ found in the LICENSE file.
   <meta charset="utf-8">
   <meta name="viewport" content="width=device-width,initial-scale=1,maximum-scale=1,user-scalable=no">
   <meta name="theme-color" id="theme-color">
+  <!-- Placeholder for CSP. -->
+  $1
   <title>$i18n{title}</title>
   <link href="https://fonts.googleapis.com/icon?family=Material+Icons"
       rel="stylesheet">
   <!-- Placeholder for CSS. -->
-  $1
+  $2
 </head>
-<body class="$2">
+<body class="$3">
   <div id="content-wrap">
     <div id="main-content">
       <div id="settings-container" class="desktop-only">
@@ -98,11 +100,11 @@ found in the LICENSE file.
             </div>
           </div>
         </header>
-        <div id="content"><noscript>$3</noscript></div>
+        <div id="content"><noscript>$4</noscript></div>
       </article>
       <div id="loading-indicator" class="visible">
         <!-- SVG material loading spinner. -->
-        $4
+        $5
       </div>
     </div>
   </div>

components/dom_distiller/core/viewer.cc

@@ -109,7 +109,8 @@ void EnsureNonEmptyContent(std::string* content) {
 }
 
 std::string ReplaceHtmlTemplateValues(const mojom::Theme theme,
-                                      const mojom::FontFamily font_family) {
+                                      const mojom::FontFamily font_family,
+                                      const std::string& csp_nonce) {
   std::string html_template =
       ui::ResourceBundle::GetSharedInstance().LoadDataResourceString(
           IDR_DOM_DISTILLER_VIEWER_HTML);
@@ -156,6 +157,7 @@ std::string ReplaceHtmlTemplateValues(const mojom::Theme theme,
   // Now do other non-i18n string replacements.
   std::vector<std::string> substitutions;
 
+  std::ostringstream csp;
   std::ostringstream css;
   std::ostringstream svg;
 #if defined(OS_IOS)
@@ -163,19 +165,37 @@ std::string ReplaceHtmlTemplateValues(const mojom::Theme theme,
   // and return the local data once a page is loaded.
   css << "<style>" << viewer::GetCss() << "</style>";
   svg << viewer::GetLoadingImage();
+
+  // iOS specific CSP policy to mitigate leaking of data from different
+  // origins.
+  csp << "<meta http-equiv=\"Content-Security-Policy\" content=\"";
+  csp << "default-src 'none'; ";
+  csp << "script-src 'nonce-" << csp_nonce << "'; ";
+  // YouTube videos are embedded as an iframe.
+  csp << "frame-src http://www.youtube.com; ";
+  csp << "style-src 'unsafe-inline' https://fonts.googleapis.com; ";
+  // Allows the fallback font-face from the main stylesheet.
+  csp << "font-src https://fonts.gstatic.com; ";
+  // Images will be inlined as data-uri if they are valid.
+  csp << "img-src data:; ";
+  csp << "form-action 'none'; ";
+  csp << "base-uri 'none'; ";
+  csp << "\">";
+
 #else
   css << "<link rel=\"stylesheet\" href=\"/" << kViewerCssPath << "\">";
   svg << "<img src=\"/" << kViewerLoadingImagePath << "\">";
 #endif  // defined(OS_IOS)
 
-  substitutions.push_back(css.str());  // $1
+  substitutions.push_back(csp.str());  // $1
+  substitutions.push_back(css.str());  // $2
   substitutions.push_back(GetThemeCssClass(theme) + " " +
-                          GetFontCssClass(font_family));  // $2
+                          GetFontCssClass(font_family));  // $3
 
   substitutions.push_back(l10n_util::GetStringUTF8(
-      IDS_DOM_DISTILLER_JAVASCRIPT_DISABLED_CONTENT));  // $3
+      IDS_DOM_DISTILLER_JAVASCRIPT_DISABLED_CONTENT));  // $4
 
-  substitutions.push_back(svg.str());  // $4
+  substitutions.push_back(svg.str());  // $5
 
   return base::ReplaceStringPlaceholders(html_template, substitutions, nullptr);
 }
@@ -239,8 +259,9 @@ const std::string GetToggleLoadingIndicatorJs(bool is_last_page) {
 }
 
 const std::string GetArticleTemplateHtml(mojom::Theme theme,
-                                         mojom::FontFamily font_family) {
-  return ReplaceHtmlTemplateValues(theme, font_family);
+                                         mojom::FontFamily font_family,
+                                         const std::string& csp_nonce) {
+  return ReplaceHtmlTemplateValues(theme, font_family, csp_nonce);
 }
 
 const std::string GetUnsafeArticleContentJs(

components/dom_distiller/core/viewer.h

@@ -28,7 +28,8 @@ namespace viewer {
 // parameters. Information from the original article has not yet been inserted,
 // so the returned HTML should be safe.
 const std::string GetArticleTemplateHtml(mojom::Theme theme,
-                                         mojom::FontFamily font_family);
+                                         mojom::FontFamily font_family,
+                                         const std::string& csp_nonce);
 
 // Returns the JavaScript to place a full article's HTML on the page. The
 // returned HTML should be considered unsafe, so callers must ensure

ios/chrome/browser/dom_distiller/distiller_viewer.cc

@@ -7,7 +7,9 @@
 #include <string>
 #include <utility>
 
+#include "base/base64.h"
 #include "base/bind.h"
+#include "base/rand_util.h"
 #include "components/dom_distiller/core/distilled_page_prefs.h"
 #include "components/dom_distiller/core/distiller.h"
 #include "components/dom_distiller/core/dom_distiller_request_view_base.h"
@@ -29,11 +31,11 @@ DistillerViewer::DistillerViewer(
       callback_(std::move(callback)) {
   DCHECK(distillerService);
   DCHECK(url.is_valid());
+  base::Base64Encode(base::RandBytesAsString(16), &csp_nonce_);
   std::unique_ptr<dom_distiller::DistillerPage> page =
       distillerService->CreateDefaultDistillerPage(gfx::Size());
   std::unique_ptr<ViewerHandle> viewer_handle =
       distillerService->ViewUrl(this, std::move(page), url);
-
   TakeViewerHandle(std::move(viewer_handle));
 }
 
@@ -47,6 +49,7 @@ DistillerViewer::DistillerViewer(
       url_(url),
       callback_(std::move(callback)) {
   DCHECK(url.is_valid());
+  base::Base64Encode(base::RandBytesAsString(16), &csp_nonce_);
   SendCommonJavaScript();
   distiller_ = distiller_factory->CreateDistillerForUrl(url);
   distiller_->DistillPage(
@@ -81,13 +84,15 @@ void DistillerViewer::OnArticleReady(
         images.push_back(ImageInfo{GURL(image.url()), image.data()});
       }
     }
-    const std::string html =
-        viewer::GetArticleTemplateHtml(distilled_page_prefs_->GetTheme(),
-                                       distilled_page_prefs_->GetFontFamily());
+
+    const std::string html = viewer::GetArticleTemplateHtml(
+        distilled_page_prefs_->GetTheme(),
+        distilled_page_prefs_->GetFontFamily(), csp_nonce_);
 
     std::string html_and_script(html);
-    html_and_script +=
-        "<script> distillerOnIos = true; " + js_buffer_ + "</script>";
+    html_and_script += "<script nonce=\"" + csp_nonce_ + "\">" +
+                       "distillerOnIos = true; " + js_buffer_ + "</script>";
+
     std::move(callback_).Run(url_, html_and_script, images,
                              article_proto->title());
   } else {
@@ -99,4 +104,8 @@ void DistillerViewer::SendJavaScript(const std::string& buffer) {
   js_buffer_ += buffer;
 }
 
+std::string DistillerViewer::GetCspNonce() {
+  return csp_nonce_;
+}
+
 }  // namespace dom_distiller

ios/chrome/browser/dom_distiller/distiller_viewer.h

@@ -45,6 +45,8 @@ class DistillerViewerInterface : public DomDistillerRequestViewBase {
 
   void SendJavaScript(const std::string& buffer) override = 0;
 
+  virtual std::string GetCspNonce() = 0;
+
   DISALLOW_COPY_AND_ASSIGN(DistillerViewerInterface);
 };
 
@@ -77,6 +79,8 @@ class DistillerViewer : public DistillerViewerInterface {
 
   void SendJavaScript(const std::string& buffer) override;
 
+  std::string GetCspNonce() override;
+
  private:
   // Called by the distiller when article is ready.
   void OnDistillerFinished(
@@ -90,6 +94,8 @@ class DistillerViewer : public DistillerViewerInterface {
   const GURL url_;
   // JavaScript buffer.
   std::string js_buffer_;
+  // CSP nonce value.
+  std::string csp_nonce_;
   // Callback to run once distillation is complete.
   DistillationFinishedCallback callback_;
   // Keep reference of the distiller_ during distillation.

ios/chrome/browser/reading_list/offline_page_tab_helper.mm

@@ -36,6 +36,7 @@ namespace {
 // Gets the offline data at |offline_path|. The result is a single std::string
 // with all resources inlined.
 // This method access file system and cannot be called on UI thread.
+// TODO(crbug.com/1166398): Remove backwards compatibility after M95
 std::string GetOfflineData(base::FilePath offline_root,
                            base::FilePath offline_path) {
   base::FilePath absolute_path =

ios/chrome/browser/reading_list/url_downloader.cc

@@ -7,12 +7,15 @@
 #include <string>
 #include <vector>
 
+#include "base/base64.h"
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/json/json_writer.h"
 #include "base/memory/ptr_util.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
+#include "base/strings/string_util.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
 #include "components/reading_list/core/offline_url_utils.h"
@@ -20,8 +23,8 @@
 #include "ios/chrome/browser/dom_distiller/distiller_viewer.h"
 #include "ios/chrome/browser/reading_list/reading_list_distiller_page.h"
 #include "ios/chrome/browser/reading_list/reading_list_distiller_page_factory.h"
-#include "net/base/escape.h"
 #include "net/base/load_flags.h"
+#include "net/base/mime_sniffer.h"
 #include "net/http/http_response_headers.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
@@ -32,13 +35,26 @@ namespace {
 // The pages are stored locally and long pressing on them will trigger a context
 // menu on the file:// URL which cannot be opened. Disable the context menu.
 const char kDisableImageContextMenuScript[] =
-    "<script>"
+    "<script nonce=\"$1\">"
     "document.addEventListener('DOMContentLoaded', function (event) {"
     "    var imgMenuDisabler = document.createElement('style');"
     "    imgMenuDisabler.innerHTML = 'img { -webkit-touch-callout: none; }';"
     "    document.head.appendChild(imgMenuDisabler);"
     "}, false);"
     "</script>";
+
+// This script replaces any downloaded images with a data uri.
+const char kReplaceDownloadedImagesScript[] =
+    "<script nonce=\"$1\">"
+    "document.addEventListener('DOMContentLoaded', function (event) {"
+    "    var imgData = {};"
+    "    $2"
+    "    var imgTags = document.getElementsByTagName(\"img\");"
+    "    for(image of imgTags) {"
+    "        image.src = imgData[image.src] || image.src;"
+    "    }"
+    "}, false);"
+    "</script>";
 }  // namespace
 
 // URLDownloader
@@ -301,7 +317,7 @@ URLDownloader::SuccessState URLDownloader::SaveDistilledHTML(
         images,
     const std::string& html) {
   if (CreateOfflineURLDirectory(url)) {
-    return SaveHTMLForURL(SaveAndReplaceImagesInHTML(url, html, images), url)
+    return SaveHTMLForURL(ReplaceImagesInHTML(url, html, images), url)
                ? DOWNLOAD_SUCCESS
                : ERROR;
   }
@@ -317,32 +333,13 @@ bool URLDownloader::CreateOfflineURLDirectory(const GURL& url) {
   return true;
 }
 
-bool URLDownloader::SaveImage(const GURL& url,
-                              const GURL& image_url,
-                              const std::string& data,
-                              std::string* image_name) {
-  std::string image_hash = base::MD5String(image_url.spec());
-  *image_name = image_hash;
-  base::FilePath directory_path =
-      reading_list::OfflineURLDirectoryAbsolutePath(base_directory_, url);
-  base::FilePath path = directory_path.Append(image_hash);
-  if (!base::PathExists(path)) {
-    int written = base::WriteFile(path, data.c_str(), data.length());
-    if (written <= 0) {
-      return false;
-    }
-    saved_size_ += written;
-    return true;
-  }
-  return true;
-}
-
-std::string URLDownloader::SaveAndReplaceImagesInHTML(
+std::string URLDownloader::ReplaceImagesInHTML(
     const GURL& url,
     const std::string& html,
     const std::vector<dom_distiller::DistillerViewerInterface::ImageInfo>&
         images) {
   std::string mutable_html = html;
+  std::string image_js;
   bool local_images_found = false;
   for (size_t i = 0; i < images.size(); i++) {
     if (images[i].url.SchemeIs(url::kDataScheme)) {
@@ -353,24 +350,47 @@ std::string URLDownloader::SaveAndReplaceImagesInHTML(
     // Mixed content is HTTP images on HTTPS pages.
     bool image_is_mixed_content = distilled_url_.SchemeIsCryptographic() &&
                                   !images[i].url.SchemeIsCryptographic();
-    // Only save images if it is not mixed content and image data is valid.
-    if (!image_is_mixed_content && images[i].url.is_valid() &&
-        !images[i].data.empty()) {
-      if (!SaveImage(url, images[i].url, images[i].data, &local_image_name)) {
-        return std::string();
-      }
+    // Only inline images if it is not mixed content and image data is valid.
+    if (image_is_mixed_content || !images[i].url.is_valid() ||
+        images[i].data.empty()) {
+      continue;
     }
-    std::string image_url = net::EscapeForHTML(images[i].url.spec());
-    size_t image_url_size = image_url.size();
-    size_t pos = mutable_html.find(image_url, 0);
-    while (pos != std::string::npos) {
-      local_images_found = true;
-      mutable_html.replace(pos, image_url_size, local_image_name);
-      pos = mutable_html.find(image_url, pos + local_image_name.size());
+
+    // Try to detect the mime-type from the bytes so an arbitrary page cannot
+    // be included. Returned mime-type must start with "image/".
+    std::string sniffed_type;
+    if (!net::SniffMimeTypeFromLocalData(images[i].data, &sniffed_type)) {
+      continue;
+    }
+
+    if (!base::StartsWith(sniffed_type, "image/")) {
+      continue;
     }
+
+    std::string image_url;
+    std::string image_data;
+    base::Value value(images[i].url.spec());
+
+    base::JSONWriter::Write(value, &image_url);
+    base::Base64Encode(images[i].data, &image_data);
+
+    std::string src_with_data =
+        base::StringPrintf("data:image/png;base64,%s", image_data.c_str());
+    image_js += "imgData[" + image_url + "] = \"" + src_with_data + "\";";
+
+    local_images_found = true;
   }
+
   if (local_images_found) {
-    mutable_html += kDisableImageContextMenuScript;
+    std::vector<std::string> substitutions;
+    substitutions.push_back(distiller_->GetCspNonce());
+
+    mutable_html += base::ReplaceStringPlaceholders(
+        kDisableImageContextMenuScript, substitutions, nullptr);
+
+    substitutions.push_back(image_js);
+    mutable_html += base::ReplaceStringPlaceholders(
+        kReplaceDownloadedImagesScript, substitutions, nullptr);
   }
 
   return mutable_html;

ios/chrome/browser/reading_list/url_downloader.h

@@ -136,17 +136,10 @@ class URLDownloader : reading_list::ReadingListDistillerPageDelegate {
 
   // HTML processing methods.
 
-  // Saves the |data| for image at |imageURL| to disk, for main URL |url|;
-  // puts path of saved file in |path| and returns whether save was successful.
-  bool SaveImage(const GURL& url,
-                 const GURL& imageURL,
-                 const std::string& data,
-                 std::string* image_name);
-  // Saves images in |images| array to disk and replaces references in |html| to
-  // local path. Returns updated html.
-  // If some images could not be saved, returns an empty string. It is the
-  // responsibility of the caller to clean the partial processing.
-  std::string SaveAndReplaceImagesInHTML(
+  // Injects script to replace images in |images| array with a data-uri
+  // of their contents. If the data does not represent an image, it is
+  // skipped.
+  std::string ReplaceImagesInHTML(
       const GURL& url,
       const std::string& html,
       const std::vector<dom_distiller::DistillerViewerInterface::ImageInfo>&

ios/chrome/browser/reading_list/url_downloader_unittest.mm

@@ -29,6 +29,11 @@
 
 namespace {
 
+const char kDistilledHtmlContent[] = "html";
+const char kDistilledPdfContent[] = "123456789";
+const char kBadImageUrl[] = "http://image/bad";
+const char kGoodImageUrl[] = "http://image/good";
+
 class DistillerViewerTest : public dom_distiller::DistillerViewerInterface {
  public:
   DistillerViewerTest(const GURL& url,
@@ -40,8 +45,13 @@ class DistillerViewerTest : public dom_distiller::DistillerViewerInterface {
       : dom_distiller::DistillerViewerInterface(nil) {
     std::vector<ImageInfo> images;
     ImageInfo image;
-    image.url = GURL("http://image");
-    image.data = "image";
+
+    image.url = GURL(kBadImageUrl);
+    image.data = "BADIMAGE";
+    images.push_back(image);
+
+    image.url = GURL(kGoodImageUrl);
+    image.data = "GIF87a...GIFDATA";
     images.push_back(image);
 
     if (redirect_url.is_valid()) {
@@ -57,6 +67,8 @@ class DistillerViewerTest : public dom_distiller::DistillerViewerInterface {
       const dom_distiller::DistilledArticleProto* article_proto) override {}
 
   void SendJavaScript(const std::string& buffer) override {}
+
+  std::string GetCspNonce() override { return std::string(); }
 };
 
 void RemoveOfflineFilesDirectory(base::FilePath base_directory) {
@@ -80,7 +92,7 @@ class MockURLDownloader : public URLDownloader {
                                           base::Unretained(this)),
                       base::BindRepeating(&MockURLDownloader::OnEndRemove,
                                           base::Unretained(this))),
-        html_("html") {}
+        html_(kDistilledHtmlContent) {}
 
   void ClearCompletionTrackers() {
     downloaded_files_.clear();
@@ -132,9 +144,24 @@ class MockURLDownloader : public URLDownloader {
                      int64_t size,
                      const std::string& title) {
     downloaded_files_.push_back(url);
-    // Saved data is the string "html" and an image with data "image".
-    EXPECT_EQ(size, 9);
+
     EXPECT_EQ(distilled_url, redirect_url_);
+
+    std::string distilled_content;
+
+    base::ReadFileToString(reading_list::OfflineURLAbsolutePathFromRelativePath(
+                               base_directory_, distilled_path),
+                           &distilled_content);
+
+    // PDF will download just the single file without any processing.
+    if (distilled_path.MatchesExtension((".pdf"))) {
+      EXPECT_EQ(distilled_content, kDistilledPdfContent);
+    } else {
+      // Check that the image with the bad mime-type was dropped
+      EXPECT_EQ(distilled_content.find(kDistilledHtmlContent), 0UL);
+      EXPECT_EQ(distilled_content.find(kBadImageUrl), std::string::npos);
+      EXPECT_NE(distilled_content.find(kGoodImageUrl), std::string::npos);
+    }
   }
 
   void OnEndRemove(const GURL& url, bool success) {
@@ -229,7 +256,7 @@ TEST_F(URLDownloaderTest, SingleDownloadPDF) {
   response_info->mime_type = "application/pdf";
   test_url_loader_factory_.SimulateResponseForPendingRequest(
       pending_request->request.url, network::URLLoaderCompletionStatus(net::OK),
-      std::move(response_info), std::string("123456789"));
+      std::move(response_info), std::string(kDistilledPdfContent));
 
   // Wait for all asynchronous tasks to complete.
   task_environment_.RunUntilIdle();