[3.5] bpo-33001: Prevent buffer overrun in os.symlink (GH-5989) (#5991)

zooba · larryhastings · commit f381cfe07d15 · 2018-05-14T14:03:17.000-04:00
* bpo-33001: Minimal fix to prevent buffer overrun in os.symlink * Remove invalid test
diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
@@ -2117,6 +2117,46 @@ def test_12084(self):
             os.remove(file1)
             shutil.rmtree(level1)
 
+    def test_buffer_overflow(self):
+        # Older versions would have a buffer overflow when detecting
+        # whether a link source was a directory. This test ensures we
+        # no longer crash, but does not otherwise validate the behavior
+        segment = 'X' * 27
+        path = os.path.join(*[segment] * 10)
+        test_cases = [
+            # overflow with absolute src
+            ('\\' + path, segment),
+            # overflow dest with relative src
+            (segment, path),
+            # overflow when joining src
+            (path[:180], path[:180]),
+        ]
+        for src, dest in test_cases:
+            try:
+                os.symlink(src, dest)
+            except FileNotFoundError:
+                pass
+            else:
+                try:
+                    os.remove(dest)
+                except OSError:
+                    pass
+            # Also test with bytes, since that is a separate code path.
+            # However, because of the stack layout, it is not possible
+            # to exploit the overflow on Python 3.5 using bytes
+            try:
+                os.symlink(os.fsencode(src), os.fsencode(dest))
+            except ValueError:
+                # Conversion function checks for len(arg) >= 260
+                pass
+            except FileNotFoundError:
+                pass
+            else:
+                try:
+                    os.remove(dest)
+                except OSError:
+                    pass
+
 
 @unittest.skipUnless(sys.platform == "win32", "Win32 specific tests")
 class Win32JunctionTests(unittest.TestCase):
diff --git a/Misc/NEWS.d/next/Security/2018-03-05-10-09-51.bpo-33001.elj4Aa.rst b/Misc/NEWS.d/next/Security/2018-03-05-10-09-51.bpo-33001.elj4Aa.rst
@@ -0,0 +1 @@
+Minimal fix to prevent buffer overrun in os.symlink on Windows
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
@@ -7259,90 +7259,98 @@ check_CreateSymbolicLink(void)
     return (Py_CreateSymbolicLinkW && Py_CreateSymbolicLinkA);
 }
 
-/* Remove the last portion of the path */
-static void
+/* Remove the last portion of the path - return 0 on success */
+static int
 _dirnameW(WCHAR *path)
 {
     WCHAR *ptr;
+    size_t length = wcsnlen_s(path, MAX_PATH);
+    if (length == MAX_PATH) {
+        return -1;
+    }
 
     /* walk the path from the end until a backslash is encountered */
-    for(ptr = path + wcslen(path); ptr != path; ptr--) {
+    for(ptr = path + length; ptr != path; ptr--) {
         if (*ptr == L'\\' || *ptr == L'/')
             break;
     }
     *ptr = 0;
+    return 0;
 }
 
-/* Remove the last portion of the path */
-static void
+/* Remove the last portion of the path - return 0 on success */
+static int
 _dirnameA(char *path)
 {
     char *ptr;
+    size_t length = strnlen_s(path, MAX_PATH);
+    if (length == MAX_PATH) {
+        return -1;
+    }
 
     /* walk the path from the end until a backslash is encountered */
-    for(ptr = path + strlen(path); ptr != path; ptr--) {
+    for(ptr = path + length; ptr != path; ptr--) {
         if (*ptr == '\\' || *ptr == '/')
             break;
     }
     *ptr = 0;
+    return 0;
 }
 
 /* Is this path absolute? */
 static int
 _is_absW(const WCHAR *path)
 {
-    return path[0] == L'\\' || path[0] == L'/' || path[1] == L':';
+    return path[0] == L'\\' || path[0] == L'/' ||
+        (path[0] && path[1] == L':');
 
 }
 
 /* Is this path absolute? */
 static int
 _is_absA(const char *path)
 {
-    return path[0] == '\\' || path[0] == '/' || path[1] == ':';
+    return path[0] == '\\' || path[0] == '/' ||
+        (path[0] && path[1] == ':');
 
 }
 
-/* join root and rest with a backslash */
-static void
+/* join root and rest with a backslash - return 0 on success */
+static int
 _joinW(WCHAR *dest_path, const WCHAR *root, const WCHAR *rest)
 {
-    size_t root_len;
-
     if (_is_absW(rest)) {
-        wcscpy(dest_path, rest);
-        return;
+        return wcscpy_s(dest_path, MAX_PATH, rest);
     }
 
-    root_len = wcslen(root);
+    if (wcscpy_s(dest_path, MAX_PATH, root)) {
+        return -1;
+    }
 
-    wcscpy(dest_path, root);
-    if(root_len) {
-        dest_path[root_len] = L'\\';
-        root_len++;
+    if (dest_path[0] && wcscat_s(dest_path, MAX_PATH, L"\\")) {
+        return -1;
     }
-    wcscpy(dest_path+root_len, rest);
+
+    return wcscat_s(dest_path, MAX_PATH, rest);
 }
 
-/* join root and rest with a backslash */
-static void
+/* join root and rest with a backslash - return 0 on success */
+static int
 _joinA(char *dest_path, const char *root, const char *rest)
 {
-    size_t root_len;
-
     if (_is_absA(rest)) {
-        strcpy(dest_path, rest);
-        return;
+        return strcpy_s(dest_path, MAX_PATH, rest);
     }
 
-    root_len = strlen(root);
+    if (strcpy_s(dest_path, MAX_PATH, root)) {
+        return -1;
+    }
 
-    strcpy(dest_path, root);
-    if(root_len) {
-        dest_path[root_len] = '\\';
-        root_len++;
+    if (dest_path[0] && strcat_s(dest_path, MAX_PATH, "\\")) {
+        return -1;
     }
-    strcpy(dest_path+root_len, rest);
+
+    return strcat_s(dest_path, MAX_PATH, rest);
 }
 
 /* Return True if the path at src relative to dest is a directory */
@@ -7354,10 +7362,14 @@ _check_dirW(WCHAR *src, WCHAR *dest)
     WCHAR src_resolved[MAX_PATH] = L"";
 
     /* dest_parent = os.path.dirname(dest) */
-    wcscpy(dest_parent, dest);
-    _dirnameW(dest_parent);
+    if (wcscpy_s(dest_parent, MAX_PATH, dest) ||
+        _dirnameW(dest_parent)) {
+        return 0;
+    }
     /* src_resolved = os.path.join(dest_parent, src) */
-    _joinW(src_resolved, dest_parent, src);
+    if (_joinW(src_resolved, dest_parent, src)) {
+        return 0;
+    }
     return (
         GetFileAttributesExW(src_resolved, GetFileExInfoStandard, &src_info)
         && src_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY
@@ -7373,10 +7385,14 @@ _check_dirA(char *src, char *dest)
     char src_resolved[MAX_PATH] = "";
 
     /* dest_parent = os.path.dirname(dest) */
-    strcpy(dest_parent, dest);
-    _dirnameA(dest_parent);
+    if (strcpy_s(dest_parent, MAX_PATH, dest) ||
+        _dirnameA(dest_parent)) {
+        return 0;
+    }
     /* src_resolved = os.path.join(dest_parent, src) */
-    _joinA(src_resolved, dest_parent, src);
+    if (_joinA(src_resolved, dest_parent, src)) {
+        return 0;
+    }
     return (
         GetFileAttributesExA(src_resolved, GetFileExInfoStandard, &src_info)
         && src_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY
@@ -7441,6 +7457,7 @@ os_symlink_impl(PyObject *module, path_t *src, path_t *dst,
 #ifdef MS_WINDOWS
 
     Py_BEGIN_ALLOW_THREADS
+    _Py_BEGIN_SUPPRESS_IPH
     if (dst->wide) {
         /* if src is a directory, ensure target_is_directory==1 */
         target_is_directory |= _check_dirW(src->wide, dst->wide);
@@ -7453,6 +7470,7 @@ os_symlink_impl(PyObject *module, path_t *src, path_t *dst,
         result = Py_CreateSymbolicLinkA(dst->narrow, src->narrow,
                                         target_is_directory);
     }
+    _Py_END_SUPPRESS_IPH
     Py_END_ALLOW_THREADS
 
     if (!result)

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Minimal fix to prevent buffer overrun in os.symlink on Windows`