From: Steve Dower Date: Mon, 14 May 2018 17:26:36 +0000 (-0400) Subject: [3.4] bpo-33001: Prevent buffer overrun in os.symlink (GH-5989) (#5992) X-Git-Tag: v3.4.9rc1~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=77c02cdce2d7b8360771be35b7676a4977e070c1;p=thirdparty%2FPython%2Fcpython.git [3.4] bpo-33001: Prevent buffer overrun in os.symlink (GH-5989) (#5992) * bpo-33001: Minimal fix to prevent buffer overrun in os.symlink * Skips test to avoid crashing during the test suite * Remove invalid test --- diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index c5f5937889f7..c6a24a3b0f81 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -1851,6 +1851,46 @@ class Win32SymlinkTests(unittest.TestCase): os.remove(file1) shutil.rmtree(level1) + @unittest.skip('Python 3.4 will crash safely on this buffer ' + 'overflow, but still crashes') + 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 ValueError: + # Conversion function checks for len(arg) >= 260 + pass + except FileNotFoundError: + pass + else: + try: + os.remove(dest) + except OSError: + pass + @support.skip_unless_symlink class NonLocalSymlinkTests(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 7c937e055370..74d03d41d897 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7200,39 +7200,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':'); } @@ -7240,50 +7251,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 */ @@ -7295,10 +7303,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 @@ -7314,10 +7326,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