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

extensions/browser/extension_protocols.cc

 // Copyright 2014 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 "extensions/browser/extension_protocols.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 266 matching lines @@
 
  private:
   ~URLRequestExtensionJob() override {
     UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.TotalKbRead", bytes_read_ / 1024);
     UMA_HISTOGRAM_COUNTS("ExtensionUrlRequest.SeekPosition", seek_position_);
     if (request_timer_.get())
       UMA_HISTOGRAM_TIMES("ExtensionUrlRequest.Latency",
                           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;
     response_info_.headers = BuildHttpHeaders(
         content_security_policy_,
         send_cors_header_,
         *last_modified_time);
     URLRequestFileJob::Start();
   }
 
@@ skipping 313 matching lines @@
                                extensions::InfoMap* extension_info_map) {
   return base::MakeUnique<ExtensionProtocolHandler>(is_incognito,
                                                     extension_info_map);
 }
 
 void SetExtensionProtocolTestHandler(ExtensionProtocolTestHandler* handler) {
   g_test_handler = handler;
 }
 
 }  // namespace extensions