chromeos: Check both original and absolute paths for file: scheme

extensions/browser/extension_protocols.cc

Index: extensions/browser/extension_protocols.cc
diff --git a/extensions/browser/extension_protocols.cc b/extensions/browser/extension_protocols.cc
index c1ea0a9b3e33ae7f1656ae6bb8c9fc3018f013a2..79afeac428def6ac29bd04370f1a6f06943d94cb 100644
--- a/extensions/browser/extension_protocols.cc
+++ b/extensions/browser/extension_protocols.cc
@@ -284,6 +284,13 @@ class URLRequestExtensionJob : public net::URLRequestFileJob {
                           request_timer_->Elapsed());
   }
 
+  // Returns true to disable the file patch checking for file: scheme in
+  // URLRequestFileJob, that's not relevant for this class.
+  bool CanAccessFile(const base::FilePath& original_path,
+                     const base::FilePath& absolute_path) override {
+    return true;

[Devlin, 2017/05/25 15:06:30]

What do |original_path| and |absolute_path| here look like?  It would be nice to
have some kind of sanity checks, rather than always returning true.

[satorux1, 2017/05/25 21:14:50]

If the original_path is a symlink in the user's Downloads folder, these look
like

- original_path: /home/chronos/user/Downloads/symlink
- absolute_path: /etc/hotsts

URLRequestJob::CanAccessFile() is added to control access to files via file://
URLs where users can specify arbitrary file paths.

Here, URLRequestExtensionJob is used to read resource files, and this patch
didn't change the existing behavior (there was no check against the file path).
I think we can add some a sanity check like

  return file_path_ == original_path;

Does this sound reasonable?

[Devlin, 2017/05/26 00:55:36]

Unfortunately, I think we do allow sym links in extensions.  I *think* that we
should be able to at least check that paths are always within the directory. 
I'd suggest something like:

// The access checks for the file are performed before the job is
// created, so we should know that this is safe.
DCHECK(directory_path_.IsParent(absolute_path));
return true;

Can you run try that and see if anything breaks?  (I ran the unit tests and it
seems to be fine, but there might be an edge case I'm missing)

[satorux1, 2017/05/26 03:28:38]

Done. I'm going to run try bots.

[satorux1, 2017/05/26 05:00:40]

I got this error:

[ RUN      ] ExtensionProtocolsTest.VerificationSeenForFileAccessErrors
[6264:771:0525/204813.535575:49518030221189:FATAL:extension_protocols.cc(291)]
Check failed: directory_path_.IsParent(absolute_path).

I'm guessing that |directory_path_| contains a symlink. Let me see if changing
|absolute_path| to |original_path| makes a difference.

[satorux1, 2017/05/29 03:36:16]

Changed to |original_path| but still the test failed on a Mac bot. I think it's
complicated since Mac bots use symbolic links for temporary directories [1]. Let
me remove DCHECK for now.


[1] Search for "Mac and Android bots" at
https://codereview.chromium.org/2786583002/diff/840001/net/url_request/url_re...

+  }
+
   void OnFilePathAndLastModifiedTimeRead(base::FilePath* read_file_path,
                                          base::Time* last_modified_time) {
     file_path_ = *read_file_path;