commit a9f1f3d7c15fdfbb69c6ba0cc088932d5c8e8c95
author Greg Kerr <kerrnel@chromium.org> Tue Oct 20 15:46:36 2020 +0000
committer Commit Bot <commit-bot@chromium.org> Fri Nov 06 18:12:57 2020 +0000
tree 1cb6862c849f1da6433ff8674b1d47925c0f87a4
parent 1bfa26f009602a14812e66d1c9f017c67e4f37d3

cryptohome: Use file descriptor for directory creation and ownership change.

BUG=chromium:1139411
TEST=tast run ${DUT_IP} hwsec.*

Change-Id: I7270f871bdf5c9f840bbde62c3bc4cd7cc277d38
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2490650
Tested-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Allen Webb <allenwebb@google.com>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Commit-Queue: Greg Kerr <kerrnel@chromium.org>
Auto-Submit: Greg Kerr <kerrnel@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2522668
Reviewed-by: Greg Kerr <kerrnel@chromium.org>

cryptohome/mock_platform.h

diff --git a/cryptohome/mock_platform.h b/cryptohome/mock_platform.h
index 6546bb4..e9f0756 100644
--- a/cryptohome/mock_platform.h
+++ b/cryptohome/mock_platform.h
@@ -395,6 +395,10 @@
               RestoreSELinuxContexts,
               (const base::FilePath&, bool),
               (override));
+  MOCK_METHOD(bool,
+              SafeCreateDirAndSetOwnership,
+              (const base::FilePath&, uid_t, gid_t),
+              (override));
 
   MockFileEnumerator* mock_enumerator() { return mock_enumerator_.get(); }
   brillo::ProcessMock* mock_process() { return mock_process_.get(); }

cryptohome/mount_helper.cc

diff --git a/cryptohome/mount_helper.cc b/cryptohome/mount_helper.cc
index e39d722..b8c8888 100644
--- a/cryptohome/mount_helper.cc
+++ b/cryptohome/mount_helper.cc
@@ -374,9 +374,8 @@
     FilePath dir_name = FilePath(next_path).BaseName();
     FilePath destination_dir = destination.Append(dir_name);
     VLOG(1) << "RecursiveCopy: " << destination_dir.value();
-    if (!platform_->CreateDirectory(destination_dir) ||
-        !platform_->SetOwnership(destination_dir, default_uid_, default_gid_,
-                                 true)) {
+    if (!platform_->SafeCreateDirAndSetOwnership(destination_dir, default_uid_,
+                                                 default_gid_)) {
       LOG(ERROR) << "Couldn't change owner (" << default_uid_ << ":"
                  << default_gid_
                  << ") of destination path: " << destination_dir.value();

cryptohome/platform.cc

diff --git a/cryptohome/platform.cc b/cryptohome/platform.cc
index fadbba9..6878720 100644
--- a/cryptohome/platform.cc
+++ b/cryptohome/platform.cc
@@ -59,6 +59,7 @@
 #include <base/threading/thread.h>
 #include <base/time/time.h>
 #include <brillo/file_utils.h>
+#include <brillo/files/safe_fd.h>
 #include <brillo/process/process.h>
 #include <brillo/secure_blob.h>
 #include <openssl/rand.h>
@@ -743,6 +744,19 @@
   return base::CreateDirectory(path);
 }
 
+bool Platform::SafeCreateDirAndSetOwnership(const base::FilePath& path,
+                                            uid_t user_id,
+                                            gid_t group_id) {
+  auto root_fd_result = brillo::SafeFD::Root();
+  if (root_fd_result.second != brillo::SafeFD::Error::kNoError) {
+    return false;
+  }
+
+  auto path_result = root_fd_result.first.MakeDir(
+      path, brillo::SafeFD::kDefaultDirPermissions, user_id, group_id);
+  return path_result.second == brillo::SafeFD::Error::kNoError;
+}
+
 bool Platform::DeleteFile(const FilePath& path, bool is_recursive) {
   return base::DeleteFile(path, is_recursive);
 }

cryptohome/platform.h

diff --git a/cryptohome/platform.h b/cryptohome/platform.h
index f415db2..135d6c8 100644
--- a/cryptohome/platform.h
+++ b/cryptohome/platform.h
@@ -860,6 +860,12 @@
   virtual bool RestoreSELinuxContexts(const base::FilePath& path,
                                       bool recursive);
 
+  // This safely creates a directory and sets the permissions, looking for
+  // symlinks and race conditions underneath.
+  virtual bool SafeCreateDirAndSetOwnership(const base::FilePath& path,
+                                            uid_t user_id,
+                                            gid_t gid);
+
  private:
   // Returns the process and open file information for the specified process id
   // with files open on the given path