lacros: Restrict downloads to MyFiles>Downloads and subdirectories

Chrome OS hides many paths from the user. Users can only browse to
MyFiles, certain Android paths, and a selected set of mount points.
For security-in-depth, Chrome OS restricts download paths to those
directories and their subdirectories.

Many of the path utilities functions used to check for these
directories have ash dependencies that cannot be included in the
lacros build.

For now, restrict lacros downloads to the default Downloads
directory and its subdirectories. We can revisit this as part of
the Files work planned for early next year.

Bug: 1148848
Test: expanded unit_tests coverage for lacros

Change-Id: Ia7c41350dcfc29299579f9e7ecd4078e1eb00505
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2541124Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828011}

chrome/browser/download/download_prefs.cc

@@ -550,11 +550,28 @@ void DownloadPrefs::SaveAutoOpenState() {
 
 base::FilePath DownloadPrefs::SanitizeDownloadTargetPath(
     const base::FilePath& path) const {
-  // TODO(https://crbug.com/1148848): Sort out path sanitization for Lacros.
-#if BUILDFLAG(IS_CHROMEOS_ASH)
   if (skip_sanitize_download_target_path_for_testing_)
     return path;
 
+#if BUILDFLAG(IS_CHROMEOS_LACROS)
+  // TODO(https://crbug.com/1148848): Sort out path sanitization for Lacros.
+  // This will require refactoring the ash-only code below so it can be shared.
+  // For now, only allow downloads into the Downloads directory and children.
+  const base::FilePath default_downloads_path =
+      GetDefaultDownloadDirectoryForProfile();
+  // Relative paths might be unsafe, so use the default path.
+  if (!path.IsAbsolute() || path.ReferencesParent())
+    return default_downloads_path;
+
+  // Allow downloads directory and subdirectories. Subdirectories may not seem
+  // useful, but many tests assume they can download files into a subdirectory,
+  // and allowing subdirectories doesn't hurt.
+  if (default_downloads_path == path || default_downloads_path.IsParent(path))
+    return path;
+
+  // Otherwise, return the safe default.
+  return default_downloads_path;
+#elif BUILDFLAG(IS_CHROMEOS_ASH)
   base::FilePath migrated_drive_path;
   // Managed prefs may force a legacy Drive path as the download path. Ensure
   // the path is valid when DriveFS is enabled.

chrome/browser/download/download_prefs_unittest.cc

@@ -28,6 +28,8 @@
 
 using safe_browsing::FileTypePolicies;
 
+namespace {
+
 TEST(DownloadPrefsTest, Prerequisites) {
   // Most of the tests below are based on the assumption that .swf files are not
   // allowed to open automatically, and that .txt files are allowed. If this
@@ -362,7 +364,7 @@ TEST(DownloadPrefsTest, DefaultPathChangedToInvalidValue) {
             download_prefs.GetDefaultDownloadDirectory());
 }
 
-#if BUILDFLAG(IS_CHROMEOS_ASH)
+#if BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS)
 void ExpectValidDownloadDir(Profile* profile,
                             DownloadPrefs* prefs,
                             base::FilePath path) {
@@ -373,7 +375,7 @@ void ExpectValidDownloadDir(Profile* profile,
 }
 
 TEST(DownloadPrefsTest, DownloadDirSanitization) {
-  content::BrowserTaskEnvironment task_environment_;
+  content::BrowserTaskEnvironment task_environment;
   TestingProfile profile(base::FilePath("/home/chronos/u-0123456789abcdef"));
   DownloadPrefs prefs(&profile);
   const base::FilePath default_dir =
@@ -382,6 +384,20 @@ TEST(DownloadPrefsTest, DownloadDirSanitization) {
   // Test a valid path.
   ExpectValidDownloadDir(&profile, &prefs, default_dir.AppendASCII("testdir"));
 
+  // Test with an invalid path outside the download directory.
+  profile.GetPrefs()->SetString(prefs::kDownloadDefaultDirectory,
+                                "/home/chronos");
+  EXPECT_EQ(prefs.DownloadPath(), default_dir);
+
+  // Test with an invalid path containing parent references.
+  base::FilePath parent_reference = default_dir.AppendASCII("..");
+  profile.GetPrefs()->SetString(prefs::kDownloadDefaultDirectory,
+                                parent_reference.value());
+  EXPECT_EQ(prefs.DownloadPath(), default_dir);
+
+  // TODO(https://crbug.com/1148848): Sort out path sanitization for Lacros.
+  // Once the ash-only code can be shared the tests below can be enabled.
+#if BUILDFLAG(IS_CHROMEOS_ASH)
   // Test a valid path for Android files.
   ExpectValidDownloadDir(
       &profile, &prefs,
@@ -397,17 +413,6 @@ TEST(DownloadPrefsTest, DownloadDirSanitization) {
       base::FilePath(
           "/media/fuse/crostini_0123456789abcdef_termina_penguin/testdir"));
 
-  // Test with an invalid path outside the download directory.
-  profile.GetPrefs()->SetString(prefs::kDownloadDefaultDirectory,
-                                "/home/chronos");
-  EXPECT_EQ(prefs.DownloadPath(), default_dir);
-
-  // Test with an invalid path containing parent references.
-  base::FilePath parent_reference = default_dir.AppendASCII("..");
-  profile.GetPrefs()->SetString(prefs::kDownloadDefaultDirectory,
-                                parent_reference.value());
-  EXPECT_EQ(prefs.DownloadPath(), default_dir);
-
   // DriveFS
   {
     // Create new profile for enabled feature to work.
@@ -447,8 +452,9 @@ TEST(DownloadPrefsTest, DownloadDirSanitization) {
                                    "/media/fuse/drivefs-something-else/root");
     EXPECT_EQ(prefs2.DownloadPath(), default_dir2);
   }
-}
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+}
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH) || BUILDFLAG(IS_CHROMEOS_LACROS)
 
 #ifdef OS_ANDROID
 TEST(DownloadPrefsTest, DownloadLaterPrefs) {
@@ -476,3 +482,5 @@ TEST(DownloadPrefsTest, DownloadLaterPrefs) {
 }
 
 #endif  // OS_ANDROID
+
+}  // namespace