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

chrome/browser/net/chrome_network_delegate.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 "chrome/browser/net/chrome_network_delegate.h"
 
 #include <stddef.h>
 #include <stdlib.h>
 
 #include <vector>
@@ skipping 430 matching lines @@
         BrowserThread::UI, FROM_HERE,
         base::Bind(&TabSpecificContentSettings::CookieChanged,
                    info->GetWebContentsGetterForRequest(),
                    request.url(), request.first_party_for_cookies(),
                    cookie_line, *options, !allow));
   }
 
   return allow;
 }
 
-bool ChromeNetworkDelegate::OnCanAccessFile(const net::URLRequest& request,
-                                            const base::FilePath& path) const {
+bool ChromeNetworkDelegate::OnCanAccessFile(
+    const net::URLRequest& request,
+    const base::FilePath& original_path,
+    const base::FilePath& absolute_path) const {
 #if defined(OS_CHROMEOS)
   // If we're running Chrome for ChromeOS on Linux, we want to allow file
   // access. This is checked here to make IsAccessAllowed() unit-testable.
   if (!base::SysInfo::IsRunningOnChromeOS() ||
       base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
     return true;
   }
 #endif
 
+#if defined(OS_CHROMEOS)
+  // Use the absolute path on Chrome OS so that symbolic links that point to
+  // paths outside of the whitelist are rejected.
+  const base::FilePath& path = absolute_path;
+#else
+  // Use the original path on Android. Android's whitelist relies on symbolic
+  // links (ex. /sdcard is whitelisted and commonly a symbolic link).
+  const base::FilePath& path = original_path;

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

These should be reviewed by someone more familiar with security on both
platforms than I am.

That having been said...Can we, both by default and on ChromeOS, check both
paths, and just make Android the exception?  We should default to more limited
permissions, unless we really need more permissive ones.

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

Thanks. I'll ask jorgelo@ for a review once the patch is ready for a real code
review. :)

[satorux1, 2017/05/09 07:44:09]

Changed to check both by default.

+#endif
   return IsAccessAllowed(path, profile_path_);
 }
 
 // static
 bool ChromeNetworkDelegate::IsAccessAllowed(
     const base::FilePath& path,
     const base::FilePath& profile_path) {
 #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID)
   return true;
 #else
@@ skipping 83 matching lines @@
   if (!data_use_aggregator_)
     return;
 
   if (is_data_usage_off_the_record_) {
     data_use_aggregator_->ReportOffTheRecordDataUse(tx_bytes, rx_bytes);
     return;
   }
 
   data_use_aggregator_->ReportDataUse(request, tx_bytes, rx_bytes);
 }