From: Steve Dower Date: Mon, 14 May 2018 18:03:17 +0000 (-0400) Subject: [3.5] bpo-33001: Prevent buffer overrun in os.symlink (GH-5989) (#5991) X-Git-Tag: v3.5.6rc1~5 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f381cfe07d15d52f27de771a62a8167668f0dd51;p=python [3.5] bpo-33001: Prevent buffer overrun in os.symlink (GH-5989) (#5991) * 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 index bb5d2e3429..e2077383bf 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2117,6 +2117,46 @@ class Win32SymlinkTests(unittest.TestCase): 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 new file mode 100644 index 0000000000..2acbac9e1a --- /dev/null +++ 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 index d42416b027..102c5405bd 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7259,39 +7259,50 @@ 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':'); } @@ -7299,50 +7310,47 @@ _is_absW(const WCHAR *path) 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)