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

net/url_request/url_request_file_dir_job.cc

 // Copyright (c) 2012 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 "net/url_request/url_request_file_dir_job.h"
 
 #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"
 #include "net/base/io_buffer.h"
 #include "net/url_request/url_request_status.h"
 #include "url/gurl.h"
 
 #if defined(OS_POSIX)
 #include <sys/stat.h>
 #endif
 
 namespace net {
 
 URLRequestFileDirJob::URLRequestFileDirJob(URLRequest* request,
                                            NetworkDelegate* network_delegate,
                                            const base::FilePath& dir_path)
     : URLRequestJob(request, network_delegate),
       lister_(dir_path, this),
       dir_path_(dir_path),
       canceled_(false),
       list_complete_(false),
       wrote_header_(false),
       read_pending_(false),
       read_buffer_length_(0),
       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() {
   // Start reading asynchronously so that all error reporting and data
   // callbacks happen as they would for network requests.
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(&URLRequestFileDirJob::StartAsync,
                             weak_factory_.GetWeakPtr()));
 }
 
@@ skipping 48 matching lines @@
     // On Linux, the file system encoding is not defined, but we assume that
     // SysNativeMBToWide takes care of it at least for now. We can try something
     // more sophisticated if necessary later.
     const base::string16& title = base::WideToUTF16(
         base::SysNativeMBToWide(dir_path_.value()));
 #endif
     data_.append(GetDirectoryListingHeader(title));
     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)
   // TOOD(jungshik): The same issue as for the directory name.
   base::FilePath filename = data.info.GetName();
   const std::string& raw_bytes = filename.value();
 #endif
   data_.append(GetDirectoryListingEntry(
       data.info.GetName().LossyDisplayName(),
       raw_bytes,
       data.info.IsDirectory(),
       data.info.GetSize(),
       data.info.GetLastModifiedTime()));
 
   // TODO(darin): coalesce more?
   CompleteRead(OK);
 }
 
 void URLRequestFileDirJob::OnListDone(int error) {
   DCHECK(!canceled_);
   DCHECK_LE(error, OK);
 
   list_complete_ = true;
   list_complete_result_ = static_cast<Error>(error);
   CompleteRead(list_complete_result_);
 }
 
 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);
 
   // Do nothing if there is no read pending.
   if (!read_pending_)
     return;
 
   int result = error;
   if (error == OK) {
@@ skipping 20 matching lines @@
     data_.erase(0, count);
     return count;
   } else if (list_complete_) {
     // EOF
     return list_complete_result_;
   }
   return ERR_IO_PENDING;
 }
 
 }  // namespace net