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

net/url_request/url_request_file_dir_job.cc

Index: net/url_request/url_request_file_dir_job.cc
diff --git a/net/url_request/url_request_file_dir_job.cc b/net/url_request/url_request_file_dir_job.cc
index 435ccb008655fc2c83d64c24eb783703f73f428b..1f09a549e20963ea233d633b122c7f3491ea535a 100644
--- a/net/url_request/url_request_file_dir_job.cc
+++ b/net/url_request/url_request_file_dir_job.cc
@@ -6,10 +6,12 @@
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
+#include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "net/base/directory_listing.h"
@@ -37,8 +39,13 @@ URLRequestFileDirJob::URLRequestFileDirJob(URLRequest* request,
       weak_factory_(this) {}
 
 void URLRequestFileDirJob::StartAsync() {
-  lister_.Start();
-  NotifyHeadersComplete();
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE,
+      base::TaskTraits().MayBlock().WithShutdownBehavior(
+          base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN),
+      base::Bind(&base::MakeAbsoluteFilePath, dir_path_),
+      base::Bind(&URLRequestFileDirJob::DidMakeAbsolutePath,
+                 weak_factory_.GetWeakPtr()));

[mmenke, 2017/04/18 17:24:46]

I assume there's no security concern with this extra call that accesses the file
system in cases where we'd have failed before any disk access before?

[satorux1, 2017/04/19 07:22:22]

Sorry I couldn't get your point. Could you elaborate a bit?

As for concerns, it is a known issue that this approach has a race between time
to check the path and time to read the path. eroman@ shared some comments in the
bug.

[mmenke, 2017/04/19 15:18:35]

Before, we didn't even access blacklisted files.  Now we always do.

1) This potentially allows sniffing for their existence, if it takes longer to
access a file that does exist over one that doesn't.
2) We're potentially modifying last accessed time on windows.
3) If there's any daemon monitoring file access (Like AV programs), we're
triggering it as well.

I think 1)'s the only possible concern here.  I don't know the threat model
that's motivating denying access to files, but this potentially makes file names
sniffable, at least.

[satorux1, 2017/04/21 00:59:47]

Thank you for explaining this to me very clearly! Motivation on Chrome OS is not
to expose files not intended to be accessible, via file: schema + symlinks. I'll
bring this point to the security team, when I ask them to do a code review.

 }
 
 void URLRequestFileDirJob::Start() {
@@ -107,6 +114,12 @@ void URLRequestFileDirJob::OnListFile(
     wrote_header_ = true;
   }
 
+  // Do not include inaccessible files from the directory listing.
+  if (network_delegate() && !network_delegate()->CanAccessFile(
+                                *request(), data.path, data.absolute_path)) {
+    return;
+  }

[mmenke, 2017/04/18 17:24:46]

What's the motivation for this change?  I'm not opposed, just want to know if
there's an actual security concern here.

[satorux1, 2017/04/19 07:22:22]

I wanted to exclude inaccessible files. Otherwise, the user is able to click on
them, but the contents won't be shown.

+
 #if defined(OS_WIN)
   std::string raw_bytes;  // Empty on Windows means UTF-8 encoded name.
 #elif defined(OS_POSIX)
@@ -136,6 +149,19 @@ void URLRequestFileDirJob::OnListDone(int error) {
 
 URLRequestFileDirJob::~URLRequestFileDirJob() {}
 
+void URLRequestFileDirJob::DidMakeAbsolutePath(
+    const base::FilePath& absolute_path) {
+  if (network_delegate() && !network_delegate()->CanAccessFile(
+                                *request(), dir_path_, absolute_path)) {
+    NotifyStartError(
+        URLRequestStatus(URLRequestStatus::FAILED, ERR_ACCESS_DENIED));

[mmenke, 2017/04/18 17:24:46]

URLRequestStatus::FromError(ERR_ACCESS_DENIED) is now preferred.

[satorux1, 2017/04/19 07:22:23]

Thanks. Fixed locally.

+    return;
+  }
+
+  lister_.Start();
+  NotifyHeadersComplete();
+}
+
 void URLRequestFileDirJob::CompleteRead(Error error) {
   DCHECK_LE(error, OK);
   DCHECK_NE(error, ERR_IO_PENDING);