commit fd5d1859f1e087d96524f3c6be11272ec7c359a7
author Achuith Bhandarkar <achuith@chromium.org> Mon Mar 26 14:50:54 2018 -0700
committer chrome-bot <chrome-bot@chromium.org> Mon Apr 02 07:26:29 2018 -0700
tree 2a54c7c81f50fe10aafb97134914260502e3f8df
parent 371913c4e6f0a0fa593fa73065861f779327f104

cros_vm: Safely create vm_dir.

There's a TOCTOU vulnerability raised here:
https://chromium-review.googlesource.com/c/chromiumos/chromite/+/871140#message-a810603ab6e1fc41752cc09937f370ec51765100

Introduce _CreateVMDir to fix this.

BUG=chromium:782664
TEST=manual

Change-Id: I1c22f2cd867ccd23d11974c4cef61b0673a3ce3a
Reviewed-on: https://chromium-review.googlesource.com/980751
Commit-Ready: Achuith Bhandarkar <achuith@chromium.org>
Tested-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>

scripts/cros_vm.py

diff --git a/scripts/cros_vm.py b/scripts/cros_vm.py
index 656e304..d9c5dd3 100644
--- a/scripts/cros_vm.py
+++ b/scripts/cros_vm.py
@@ -71,14 +71,7 @@
     if not self.vm_dir:
       self.vm_dir = os.path.join(osutils.GetGlobalTempDir(),
                                  'cros_vm_%d' % self.ssh_port)
-    if os.path.exists(self.vm_dir):
-      # For security, ensure that vm_dir is not a symlink, and is owned by us or
-      # by root.
-      assert not os.path.islink(self.vm_dir), \
-          'VM state dir is misconfigured; please recreate: %s' % self.vm_dir
-      st_uid = os.stat(self.vm_dir).st_uid
-      assert st_uid == 0 or st_uid == os.getuid(), \
-          'VM state dir is misconfigured; please recreate: %s' % self.vm_dir
+    self._CreateVMDir()
 
     self.pidfile = os.path.join(self.vm_dir, 'kvm.pid')
     self.kvm_monitor = os.path.join(self.vm_dir, 'kvm.monitor')
@@ -100,15 +93,20 @@
     else:
       return cros_build_lib.RunCommand(*args, **kwargs)
 
-  def _CleanupFiles(self, recreate):
-    """Cleanup vm_dir.
+  def _CreateVMDir(self):
+    """Safely create vm_dir."""
+    if not osutils.SafeMakedirs(self.vm_dir, sudo=self.use_sudo):
+      # For security, ensure that vm_dir is not a symlink, and is owned by us or
+      # by root.
+      error_str = ('VM state dir is misconfigured; please recreate: %s'
+                   % self.vm_dir)
+      assert not os.path.islink(self.vm_dir), error_str
+      st_uid = os.stat(self.vm_dir).st_uid
+      assert st_uid == 0 or st_uid == os.getuid(), error_str
 
-    Args:
-      recreate: recreate vm_dir.
-    """
+  def _RmVMDir(self):
+    """Cleanup vm_dir."""
     osutils.RmDir(self.vm_dir, ignore_missing=True, sudo=self.use_sudo)
-    if recreate:
-      osutils.SafeMakedirs(self.vm_dir)
 
   @staticmethod
   def _GetCachePath(cache_name):
@@ -270,7 +268,8 @@
     self._SetQemuPath()
     self._SetVMImagePath()
 
-    self._CleanupFiles(recreate=True)
+    self._RmVMDir()
+    self._CreateVMDir()
     # Make sure we can read these files later on by creating them as ourselves.
     osutils.Touch(self.kvm_serial)
     for pipe in [self.kvm_pipe_in, self.kvm_pipe_out]:
@@ -358,8 +357,7 @@
       logging.info('Killing %d.', pid)
       if not self.dry_run:
         self._RunCommand(['kill', '-9', str(pid)], error_code_ok=True)
-
-    self._CleanupFiles(recreate=False)
+    self._RmVMDir()
 
   def _WaitForProcs(self):
     """Wait for expected processes to launch."""