https://github.com/python/cpython/commit/baa45079466eda1f5636a6d13f3a60c2c00... commit: baa45079466eda1f5636a6d13f3a60c2c00fdcd3 branch: 3.6 author: Steve Dower <steve.dower@microsoft.com> committer: GitHub <noreply@github.com> date: 2018-03-05T14:26:28-08:00 summary: [3.6] bpo-33001: Prevent buffer overrun in os.symlink (GH-5989) (GH-5990) files: A Misc/NEWS.d/next/Security/2018-03-05-10-09-51.bpo-33001.elj4Aa.rst M Lib/test/test_os.py M Modules/posixmodule.c diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 26d544ca9925..240b7c432ba9 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2170,6 +2170,41 @@ def test_29248(self): target = os.readlink(r'C:\Users\All Users') self.assertTrue(os.path.samefile(target, r'C:\ProgramData')) + 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. + try: + os.symlink(os.fsencode(src), os.fsencode(dest)) + 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 000000000000..2acbac9e1af6 --- /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 0837a4a4991e..39ba030b5191 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7144,7 +7144,7 @@ win_readlink(PyObject *self, PyObject *args, PyObject *kwargs) #if defined(MS_WINDOWS) /* Grab CreateSymbolicLinkW dynamically from kernel32 */ -static DWORD (CALLBACK *Py_CreateSymbolicLinkW)(LPCWSTR, LPCWSTR, DWORD) = NULL; +static BOOLEAN (CALLBACK *Py_CreateSymbolicLinkW)(LPCWSTR, LPCWSTR, DWORD) = NULL; static int check_CreateSymbolicLink(void) @@ -7159,47 +7159,51 @@ check_CreateSymbolicLink(void) return Py_CreateSymbolicLinkW != NULL; } -/* 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--) { - if (*ptr == L'\\' || *ptr == L'/') + for(ptr = path + length; ptr != path; ptr--) { + if (*ptr == L'\\' || *ptr == L'/') { 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':'); } -/* 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); } /* Return True if the path at src relative to dest is a directory */ @@ -7211,10 +7215,14 @@ _check_dirW(LPCWSTR src, LPCWSTR 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 @@ -7270,19 +7278,15 @@ os_symlink_impl(PyObject *module, path_t *src, path_t *dst, } #endif - if ((src->narrow && dst->wide) || (src->wide && dst->narrow)) { - PyErr_SetString(PyExc_ValueError, - "symlink: src and dst must be the same type"); - return NULL; - } - #ifdef MS_WINDOWS Py_BEGIN_ALLOW_THREADS + _Py_BEGIN_SUPPRESS_IPH /* if src is a directory, ensure target_is_directory==1 */ target_is_directory |= _check_dirW(src->wide, dst->wide); result = Py_CreateSymbolicLinkW(dst->wide, src->wide, target_is_directory); + _Py_END_SUPPRESS_IPH Py_END_ALLOW_THREADS if (!result) @@ -7290,6 +7294,12 @@ os_symlink_impl(PyObject *module, path_t *src, path_t *dst, #else + if ((src->narrow && dst->wide) || (src->wide && dst->narrow)) { + PyErr_SetString(PyExc_ValueError, + "symlink: src and dst must be the same type"); + return NULL; + } + Py_BEGIN_ALLOW_THREADS #if HAVE_SYMLINKAT if (dir_fd != DEFAULT_DIR_FD)