[3.5] bpo-32551: Consistently configure sys.path[0] (#5197)

Directory and zipfile execution previously added
the parent directory of the directory or zipfile
as sys.path[0] and then subsequently overwrote
it with the directory or zipfile itself.

This caused problems in isolated mode, as it
overwrote the "stdlib as a zip archive" entry
in sys.path, as the parent directory was
never added.

The attempted fix to that issue in bpo-29319
created the opposite problem in *non*-isolated
mode, by potentially leaving the parent
directory on sys.path instead of overwriting it.

This change fixes the root cause of the problem
by removing the whole "add-and-overwrite" dance
for sys.path[0], and instead simply never adds
the parent directory to sys.path in the first
place.

(cherry picked from commit d2977a3)

Author: ncoghlan

Lib/test/test_cmd_line_script.py

@@ -573,6 +573,73 @@ def test_syntaxerror_indented_caret_position(self):
             self.assertNotIn("\f", text)
             self.assertIn("\n    1 + 1 = 2\n    ^", text)
 
+    def test_consistent_sys_path_for_direct_execution(self):
+        # This test case ensures that the following all give the same
+        # sys.path configuration:
+        #
+        #    ./python -s script_dir/__main__.py
+        #    ./python -s script_dir
+        #    ./python -I script_dir
+        script = textwrap.dedent("""\
+            import sys
+            for entry in sys.path:
+                print(entry)
+            """)
+        # Always show full path diffs on errors
+        self.maxDiff = None
+        with support.temp_dir() as work_dir, support.temp_dir() as script_dir:
+            script_name = _make_test_script(script_dir, '__main__', script)
+            # Reference output comes from directly executing __main__.py
+            # We omit PYTHONPATH and user site to align with isolated mode
+            p = spawn_python("-Es", script_name, cwd=work_dir)
+            out_by_name = kill_python(p).decode().splitlines()
+            self.assertEqual(out_by_name[0], script_dir)
+            self.assertNotIn(work_dir, out_by_name)
+            # Directory execution should give the same output
+            p = spawn_python("-Es", script_dir, cwd=work_dir)
+            out_by_dir = kill_python(p).decode().splitlines()
+            self.assertEqual(out_by_dir, out_by_name)
+            # As should directory execution in isolated mode
+            p = spawn_python("-I", script_dir, cwd=work_dir)
+            out_by_dir_isolated = kill_python(p).decode().splitlines()
+            self.assertEqual(out_by_dir_isolated, out_by_dir, out_by_name)
+
+    def test_consistent_sys_path_for_module_execution(self):
+        # This test case ensures that the following both give the same
+        # sys.path configuration:
+        #    ./python -sm script_pkg.__main__
+        #    ./python -sm script_pkg
+        #
+        # And that this fails as unable to find the package:
+        #    ./python -Im script_pkg
+        script = textwrap.dedent("""\
+            import sys
+            for entry in sys.path:
+                print(entry)
+            """)
+        # Always show full path diffs on errors
+        self.maxDiff = None
+        with support.temp_dir() as work_dir:
+            script_dir = os.path.join(work_dir, "script_pkg")
+            os.mkdir(script_dir)
+            script_name = _make_test_script(script_dir, '__main__', script)
+            # Reference output comes from `-m script_pkg.__main__`
+            # We omit PYTHONPATH and user site to better align with the
+            # direct execution test cases
+            p = spawn_python("-sm", "script_pkg.__main__", cwd=work_dir)
+            out_by_module = kill_python(p).decode().splitlines()
+            self.assertEqual(out_by_module[0], '')
+            self.assertNotIn(script_dir, out_by_module)
+            # Package execution should give the same output
+            p = spawn_python("-sm", "script_pkg", cwd=work_dir)
+            out_by_package = kill_python(p).decode().splitlines()
+            self.assertEqual(out_by_package, out_by_module)
+            # Isolated mode should fail with an import error
+            exitcode, stdout, stderr = assert_python_failure(
+                "-Im", "script_pkg", cwd=work_dir
+            )
+            traceback_lines = stderr.decode().splitlines()
+            self.assertIn("No module named script_pkg", traceback_lines[-1])
 
 def test_main():
     support.run_unittest(CmdLineTest)

Misc/NEWS.d/next/Security/2018-01-16-16-05-37.bpo-32551.U0z4W-.rst

@@ -0,0 +1,13 @@
+The ``sys.path[0]`` initialization change for bpo-29139 caused a regression
+by revealing an inconsistency in how sys.path is initialized when executing
+``__main__`` from a zipfile, directory, or other import location. This is
+considered a potential security issue, as it may lead to privileged
+processes unexpectedly loading code from user controlled directories in
+situations where that was not previously the case.
+
+The interpreter now consistently avoids ever adding the import location's
+parent directory to ``sys.path``, and ensures no other ``sys.path`` entries
+are inadvertently modified when inserting the import location named on the
+command line. (Originally reported as bpo-29723 against Python 3.6rc1, but
+it was missed at the time that the then upcoming Python 3.5.4 release would
+also be affected)

Modules/main.c

@@ -220,55 +220,60 @@ static int RunModule(wchar_t *modname, int set_argv0)
     return 0;
 }
 
-static int
-RunMainFromImporter(wchar_t *filename)
+static PyObject *
+AsImportPathEntry(wchar_t *filename)
 {
-    PyObject *argv0 = NULL, *importer, *sys_path, *sys_path0;
-    int sts;
+    PyObject *sys_path0 = NULL, *importer;
 
-    argv0 = PyUnicode_FromWideChar(filename, wcslen(filename));
-    if (argv0 == NULL)
+    sys_path0 = PyUnicode_FromWideChar(filename, wcslen(filename));
+    if (sys_path0 == NULL)
         goto error;
 
-    importer = PyImport_GetImporter(argv0);
+    importer = PyImport_GetImporter(sys_path0);
     if (importer == NULL)
         goto error;
 
     if (importer == Py_None) {
-        Py_DECREF(argv0);
+        Py_DECREF(sys_path0);
         Py_DECREF(importer);
-        return -1;
+        return NULL;
     }
     Py_DECREF(importer);
+    return sys_path0;
+
+error:
+    Py_XDECREF(sys_path0);
+    PySys_WriteStderr("Failed checking if argv[0] is an import path entry\n");
+    PyErr_Print();
+    PyErr_Clear();
+    return NULL;
+}
+
+
+static int
+RunMainFromImporter(PyObject *sys_path0)
+{
+    PyObject *sys_path;
+    int sts;
 
-    /* argv0 is usable as an import source, so put it in sys.path[0]
-       and import __main__ */
+    /* Assume sys_path0 has already been checked by AsImportPathEntry,
+     * so put it in sys.path[0] and import __main__ */
     sys_path = PySys_GetObject("path");
     if (sys_path == NULL) {
         PyErr_SetString(PyExc_RuntimeError, "unable to get sys.path");
         goto error;
     }
-    sys_path0 = PyList_GetItem(sys_path, 0);
-    sts = 0;
-    if (!sys_path0) {
-        PyErr_Clear();
-        sts = PyList_Append(sys_path, argv0);
-    } else if (PyObject_IsTrue(sys_path0)) {
-        sts = PyList_Insert(sys_path, 0, argv0);
-    } else {
-        sts = PyList_SetItem(sys_path, 0, argv0);
-    }
+    sts = PyList_Insert(sys_path, 0, sys_path0);
     if (sts) {
-        argv0 = NULL;
+        sys_path0 = NULL;
         goto error;
     }
-    Py_INCREF(argv0);
 
     sts = RunModule(L"__main__", 0);
     return sts != 0;
 
 error:
-    Py_XDECREF(argv0);
+    Py_XDECREF(sys_path0);
     PyErr_Print();
     return 1;
 }
@@ -352,6 +357,7 @@ Py_Main(int argc, wchar_t **argv)
     int version = 0;
     int saw_unbuffered_flag = 0;
     PyCompilerFlags cf;
+    PyObject *main_importer_path = NULL;
     PyObject *warning_option = NULL;
     PyObject *warning_options = NULL;
 
@@ -700,7 +706,17 @@ Py_Main(int argc, wchar_t **argv)
         argv[_PyOS_optind] = L"-m";
     }
 
-    PySys_SetArgv(argc-_PyOS_optind, argv+_PyOS_optind);
+    if (filename != NULL) {
+        main_importer_path = AsImportPathEntry(filename);
+    }
+
+    if (main_importer_path != NULL) {
+        /* Let RunMainFromImporter adjust sys.path[0] later */
+        PySys_SetArgvEx(argc-_PyOS_optind, argv+_PyOS_optind, 0);
+    } else {
+        /* Use config settings to decide whether or not to update sys.path[0] */
+        PySys_SetArgv(argc-_PyOS_optind, argv+_PyOS_optind);
+    }
 
     if ((Py_InspectFlag || (command == NULL && filename == NULL && module == NULL)) &&
         isatty(fileno(stdin))) {
@@ -729,11 +745,11 @@ Py_Main(int argc, wchar_t **argv)
 
         sts = -1;               /* keep track of whether we've already run __main__ */
 
-        if (filename != NULL) {
-            sts = RunMainFromImporter(filename);
+        if (main_importer_path != NULL) {
+            sts = RunMainFromImporter(main_importer_path);
         }
 
-        if (sts==-1 && filename!=NULL) {
+        if (sts==-1 && filename != NULL) {
             fp = _Py_wfopen(filename, L"r");
             if (fp == NULL) {
                 char *cfilename_buffer;