[security] [dart:io] Fix current directory being in front of PATH.

This is a security improvement.

On Linux and Android, starting a process with Process.run, Process.runSync
or Process.start would first search the current directory before searching
PATH (Issue [37101][]). Operating systems other than Linux and Android
didn't have this behavior and aren't affected by this vulnerability.

Effectively this puts the current working directory in the front of PATH,
even if it wasn't in the PATH.

This change fixes that vulnerability and only searches the directories in
the PATH environment variable.

Fixes #37101

Change-Id: I05f3137753237f9b3ba4be4eba63ad07a75d865e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/105582
Reviewed-by: William Hesse <[email protected]>

Author: sortie

CHANGELOG.md

@@ -2,6 +2,33 @@
 (Add new changes here, and they will be copied to the change section for the
  next dev version)
 
+### Security vulnerability
+
+*  **Security improvement:** On Linux and Android, starting a process with
+   `Process.run`, `Process.runSync`, or `Process.start` would first search the
+   current directory before searching `PATH` (Issue [37101][]). This behavior
+   effectively put the current working directory in the front of `PATH`, even if
+   it wasn't in the `PATH`. This release changes that behavior to only searching
+   the directories in the `PATH` environment variable. Operating systems other
+   than Linux and Android didn't have this behavior and aren't affected by this
+   vulnerability.
+
+   This vulnerability could result in execution of untrusted code if a command
+   without a slash in its name was run inside an untrusted directory containing
+   an executable file with that name:
+
+   ```dart
+   Process.run("ls", workingDirectory: "/untrusted/directory")
+   ```
+
+   This would attempt to run `/untrusted/directory/ls` if it existed, even
+   though it is not in the `PATH`. It was always safe to instead use an absolute
+   path or a path containing a slash.
+
+   This vulnerability was introduced in Dart 2.0.0.
+
+[37101]: https://github.com/dart-lang/sdk/issues/37101
+
 ### Core libraries
 
 #### `dart:isolate`

runtime/bin/process_android.cc

@@ -436,24 +436,24 @@ class ProcessStarter {
     }
   }
 
-  // Tries to find path_ relative to the current namespace.
+  // Tries to find path_ relative to the current namespace unless it should be
+  // searched in the PATH.
   // The path that should be passed to exec is returned in realpath.
   // Returns true on success, and false if there was an error that should
   // be reported to the parent.
   bool FindPathInNamespace(char* realpath, intptr_t realpath_size) {
+    // Perform a PATH search if there's no slash in the path.
+    if (strchr(path_, '/') == NULL) {
+      // TODO(zra): If there is a non-default namespace, the entries in PATH
+      // should be treated as relative to the namespace.
+      strncpy(realpath, path_, realpath_size);
+      realpath[realpath_size - 1] = '\0';
+      return true;
+    }
     NamespaceScope ns(namespc_, path_);
     const int fd =
         TEMP_FAILURE_RETRY(openat(ns.fd(), ns.path(), O_RDONLY | O_CLOEXEC));
     if (fd == -1) {
-      if ((errno == ENOENT) && (strchr(path_, '/') == NULL)) {
-        // path_ was not found relative to the namespace, but since it didn't
-        // contain a '/', we can pass it directly to execvp, which will do a
-        // lookup in PATH.
-        // TODO(zra): If there is a non-default namespace, the entries in PATH
-        // should be treated as relative to the namespace.
-        strncpy(realpath, path_, realpath_size);
-        return true;
-      }
       return false;
     }
     char procpath[PATH_MAX];

runtime/bin/process_linux.cc

@@ -436,24 +436,24 @@ class ProcessStarter {
     }
   }
 
-  // Tries to find path_ relative to the current namespace.
+  // Tries to find path_ relative to the current namespace unless it should be
+  // searched in the PATH.
   // The path that should be passed to exec is returned in realpath.
   // Returns true on success, and false if there was an error that should
   // be reported to the parent.
   bool FindPathInNamespace(char* realpath, intptr_t realpath_size) {
+    // Perform a PATH search if there's no slash in the path.
+    if (strchr(path_, '/') == NULL) {
+      // TODO(zra): If there is a non-default namespace, the entries in PATH
+      // should be treated as relative to the namespace.
+      strncpy(realpath, path_, realpath_size);
+      realpath[realpath_size - 1] = '\0';
+      return true;
+    }
     NamespaceScope ns(namespc_, path_);
     const int fd =
         TEMP_FAILURE_RETRY(openat64(ns.fd(), ns.path(), O_RDONLY | O_CLOEXEC));
     if (fd == -1) {
-      if ((errno == ENOENT) && (strchr(path_, '/') == NULL)) {
-        // path_ was not found relative to the namespace, but since it didn't
-        // contain a '/', we can pass it directly to execvp, which will do a
-        // lookup in PATH.
-        // TODO(zra): If there is a non-default namespace, the entries in PATH
-        // should be treated as relative to the namespace.
-        strncpy(realpath, path_, realpath_size);
-        return true;
-      }
       return false;
     }
     char procpath[PATH_MAX];