Process PATH search prefers current directory

[sortie]

The Dart process execute search prefers the current directory over a PATH search. This behavior violates the rule of least surprise and doesn't appear to be documented. It should instead simply do a path search.

This example program demonstrates the issue:

import 'dart:io';
main() {
  print(Process.runSync("tar", ["--version"]).stdout);
}

This normally works if the system tar is first in the PATH, but if the current directory contains an executable file named tar, then that is preferred even when the current directory is not in the PATH.

#!/bin/sh
echo Preferred the current directory

If that file exists, the test instead prints Preferred the current directory because it preferred the current directory even though it was not in the PATH.

This seems related to the dart --namespace option. The issue seems to be in runtime/bin/process_linux.cc and runtime/bin/process_android.cc in ProcessStarter::ExecProcess and ProcessStarter::ExecDetachedProcess

    char realpath[PATH_MAX];
    if (!FindPathInNamespace(realpath, PATH_MAX)) {
      ReportChildError();
    }
    // TODO(dart:io) Test for the existence of execveat, and use it instead.
    VOID_TEMP_FAILURE_RETRY(
        execvp(realpath, const_cast<char* const*>(program_arguments_)));

The intention of the namespace feature seems to be to emulate a different root directory and working directory, possibly per isolate. The PATH search in the namespace case doesn't seem to be implemented at all, where only absolute paths work. I'll understand what the code is trying to do a bit closer and then come up with a proper fix.

cc @jonasfj

[sigurdm]

Also the documentation could use an update
Examples like:

  Process.start('ls', ['-l']).then((process) {...

Encourages creating programs using the ls from $PATH (previously from working directory).
My guess is that usually one should prefer to use the system commands whenever possible: /bin/ls etc.

[sortie]

This was a security vulnerability that has been fixed in Dart 2.3.2 and Dart 2.3.3-dev.0.0.

Here is the text from the CHANGELOG:

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. 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:

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.

[natebosch]

This was also reported in #34246