Resource requests from Save-Page-As should go through CanRequestURL checks.

content/browser/download/save_package.cc

Index: content/browser/download/save_package.cc
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index a64f3f29176816bc4d4899e683065af34f539aa6..1caa372fc9a63f1cffce6a9c67072b967afa5601 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/guid.h"
 #include "base/i18n/file_util_icu.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -349,13 +350,10 @@ void SavePackage::InitWithDownloadItem(
   } else {
     DCHECK_EQ(SAVE_PAGE_TYPE_AS_ONLY_HTML, save_type_);
     wait_state_ = NET_FILES;
-    SaveFileCreateInfo::SaveFileSource save_source = page_url_.SchemeIsFile() ?
-        SaveFileCreateInfo::SAVE_FILE_FROM_FILE :
-        SaveFileCreateInfo::SAVE_FILE_FROM_NET;
     // Add this item to waiting list.
-    waiting_item_queue_.push_back(
-        new SaveItem(page_url_, Referrer(), this, save_source,
-                     FrameTreeNode::kFrameTreeNodeInvalidId));
+    waiting_item_queue_.push_back(new SaveItem(
+        page_url_, Referrer(), this, SaveFileCreateInfo::SAVE_FILE_FROM_NET,
+        FrameTreeNode::kFrameTreeNodeInvalidId));
     all_save_items_count_ = 1;
     download_->SetTotalBytes(1);
 
@@ -590,16 +588,6 @@ void SavePackage::StartSave(const SaveFileCreateInfo* info) {
     save_item->SetTargetPath(saved_main_file_path_);
   }
 
-  // If the save source is from file system, inform SaveFileManager to copy
-  // corresponding file to the file path which this SaveItem specifies.
-  if (info->save_source == SaveFileCreateInfo::SAVE_FILE_FROM_FILE) {
-    BrowserThread::PostTask(
-        BrowserThread::FILE, FROM_HERE,
-        base::Bind(&SaveFileManager::SaveLocalFile, file_manager_,
-                   save_item->url(), save_item->id(), id()));
-    return;
-  }
-
   // Check whether we begin to require serialized HTML data.
   if (save_type_ == SAVE_PAGE_TYPE_AS_COMPLETE_HTML &&
       wait_state_ == HTML_DATA) {
@@ -842,6 +830,11 @@ void SavePackage::SaveNextFile(bool process_all_remaining_items) {
     DCHECK(!ContainsKey(in_progress_items_, save_item->id()));
     in_progress_items_[save_item->id()] = save_item;
     save_item->Start();
+
+    // TODO(lukasza): https://crbug.com/616429: Pass the correct child process
+    // id instead of always passing main frame's process id.  Correctness of
+    // security checks in ResourceDispatcherHostImpl::BeginSave depends on it
+    // (OTOH, this only matters if OOPIFs are present).

[Łukasz Anforowicz, 2016/06/18 00:37:20]

I need to double-check how process_id/child_id and render_view_routing_id and
render_frame_routing_id are used by ResourceDispatcherHostImpl.  Maybe I should
just pass the right thing here (with appropriate tracking via an extra map in
SavePackage)?  OTOH, this would make it harder to merge the fix to beta/stable
branches...


FWIW, I don't think this is a ship blocker for --isolate-extensions, but please
double-check my thought process.

AFAICT, we will get the following behavior, if we always pass main frame's ID
(instead of the ID of the frame this resource originated from):

case #1: main frame authorized, subframe unauthorized: security bug, BUT this
would only impact the case when saving a page where the main frame comes from an
extension (I am not sure if this is possible;  so far I couldn't trigger
Save-Page dialog for background pages).

case #2: main frame unauthorized, subframe authorized: not a security bug; 
impact is that some legitimate resources are not saved.  This should not be an
issue because AFAIK we don't consider save-page-as feature as a blocker for
--isolate-extensions.

[asanka, 2016/06/20 20:24:18]

Thanks for calling this out. In addition to the two cases listed above, we can
also be in a situation where the parent and the child frames are using different
request contexts via storage partitions. In this case invoking 'Save as' would
leak PII by mixing requests from different cookie stores. We do consider this to
be a problem.

In practice, this only applies to apps that use <webview> elements. I don't know
what would happen if one were to attempt to invoke 'save as..' on an app
document. Invoking "Save as" inside the webview (if it's even possible), should
correctly use the inner WebContents. This is certainly something worth testing.

https://codereview.chromium.org/1924473003 is the CL where I added
StorageParition safety to regular downloads. You might be able to add something
like the test changes made there for
chrome/browser/apps/guest_view/web_view_browsertest.cc .

How difficult do you think it would be to get the correct set of IDs here?

[Łukasz Anforowicz, 2016/06/21 16:39:34]

Ack.
Hmmm... ok.  I'll try to figure this out when I come back from vacation...
I think I have the right attribution in patchset 2.

Initially, I was a bit concerned that process_id and/or routing_ids would be
used in unexpected ways and that correct attribution might break it.  It turns
out that they *are* used in creative ways (e.g. in
SaveFileManager::GetSavePackageFromRenderIds), but AFAICT everything should
continue to work despite my changes.

     file_manager_->SaveURL(
         save_item->id(), save_item->url(), save_item->referrer(),
         web_contents()->GetRenderProcessHost()->GetID(), routing_id(),
@@ -993,19 +986,20 @@ void SavePackage::GetSerializedHtmlWithLocalLinksForFrame(
       target_frame_tree_node_id);
   if (it != frame_tree_node_id_to_contained_save_items_.end()) {
     for (const SaveItem* save_item : it->second) {
-      // Skip items that failed to save.
-      if (!save_item->has_final_name()) {
-        DCHECK_EQ(SaveItem::SaveState::COMPLETE, save_item->state());
-        DCHECK(!save_item->success());
-        continue;
-      }

[Łukasz Anforowicz, 2016/06/18 00:37:20]

I've added these checks in https://codereview.chromium.org/1812693002.  I have a
feeling that they were necessary (i.e. that some tests were failing otherwise),
but I cannot recall the exact reasons why I've added these checks.  Tests are
green, so I guess writing a link to "resource-failed-to-save-<guid>.txt" is
okay.

[asanka, 2016/06/20 20:24:18]

Acknowledged.

-
       // Calculate the relative path for referring to the |save_item|.
       base::FilePath local_path(base::FilePath::kCurrentDirectory);
       if (target_tree_node->IsMainFrame()) {
         local_path = local_path.Append(saved_main_directory_path_.BaseName());
       }
-      local_path = local_path.Append(save_item->full_path().BaseName());
+      if (save_item->has_final_name()) {
+        local_path = local_path.Append(save_item->full_path().BaseName());
+      } else {
+        // Skip items that failed to save.
+        DCHECK_EQ(SaveItem::SaveState::COMPLETE, save_item->state());
+        DCHECK(!save_item->success());
+        local_path = local_path.Append(base::FilePath::FromUTF8Unsafe(
+            "resource-failed-to-save-" + base::GenerateGUID() + ".txt"));

[Łukasz Anforowicz, 2016/06/18 00:37:20]

This is tricky.  We could analyze this with an example of a website that embeds
a resource (i.e. an <img> tag) with file: URI, but it will be easier to
understand with an example of an *internet* website that links to a resources on
the *intranet*.  Currently we forbid websites from linking to resources on the
local filesystem, but in the future we plan to forbid internet websites from
linking to intranet websites (https://crbug.com/590714).

Let's say that Eve creates an internet webpage with a tag saying: <img
src="http://bob-s-intranet/send-all-money-to-eve.jpg">.  Now, I claim that:
1. The browser should not access the intranet link when *showing* the internet
website
2. The browser should not access the intranet link when *saving* the internet
website
3. The browser should not access the intranet link when *showing* the *saved*
contents of the internet website

To prevent item 3 above, we should not save the intranet link when serializing
the "complete html" of the internet webpage being saved.  I've decided to
instead serialize a link to a non-existent file (non-existency guarantess by
uniqueness of the guid).  I am not sure if I am happy with this solution, but I
can't think of a better alternative.

I recognize that the code above can negatively impact legitimate/non-malicious
scenarios.  For example previously, the saved page could be opened and would
renderer all resources even if some resources were inaccessible during the save
process (e.g. because of network flakiness, not because of authorization
issues).

Hmmm... maybe we should differentiate between unauthorized and other error
codes?  OTOH, this would seem fragile (i.e. would require exhaustively
enumerating all future error codes related to unauthorized access OR all future
error codes related to authorized access).  WDYT?

[asanka, 2016/06/20 20:24:18]

As long as we are talking about subresources, I'm okay with treating all failed
subresources the same way as ones that were not found.

#2 should guarantee that the process of saving doesn't persist a mix of
resources from intranet/internet (unless the source document is in the
intranet?). So there shouldn't be a resource on disk to violate #3. However, the
saved document shouldn't suddenly gain the ability to fiddle with the intranet
if it didn't have that ability before. #3, I think, would only be possible by
preventing file:// URLs from accessing intranet hosts. That doesn't seem in
scope for issue 590714, at least from my reading.

Note that mixing internet/intranet appears to be a common mechanism when dealing
with IOT stuff like IP connected cameras.

[Łukasz Anforowicz, 2016/06/21 16:39:34]

Yes - propagating origin-specific restrictions into the saved file is really
tricky (given that [an untrusted] renderer controls the html contents being
serialized + given that today we don't have code that recognizes that a local
html file was saved from a web origin).  It would probably be best to treat it
as a separate issue (with MotW / mark-of-the-web being the proposed propagation
mechanism).  Note that I've also tried to share some related observations in
https://crbug.com/616429#c13 and https://crbug.com/616429#c14.

FWIW, I've reverted changes related to protecting against "unauthorized" access
from the already saved page (so for subresources that failed to save we will
serialize html links pointing at the original [i.e. web] uri [rather than
pointing to resource-failed-to-save-<guid>.txt as I proposed in the initial
patchset]).
Ack.

+      }
 
       // Insert the link into |url_to_local_path| or |routing_id_to_local_path|.
       if (save_item->save_source() != SaveFileCreateInfo::SAVE_FILE_FROM_DOM) {
@@ -1214,12 +1208,9 @@ void SavePackage::EnqueueSavableResource(int container_frame_tree_node_id,
   if (!url.is_valid())
     return;
 
-  SaveFileCreateInfo::SaveFileSource save_source =
-      url.SchemeIsFile() ? SaveFileCreateInfo::SAVE_FILE_FROM_FILE
-                         : SaveFileCreateInfo::SAVE_FILE_FROM_NET;
   CreatePendingSaveItemDeduplicatingByUrl(
       container_frame_tree_node_id, FrameTreeNode::kFrameTreeNodeInvalidId, url,
-      referrer, save_source);
+      referrer, SaveFileCreateInfo::SAVE_FILE_FROM_NET);
 }
 
 void SavePackage::EnqueueFrame(int container_frame_tree_node_id,