[Python-checkins] [3.6] bpo-33001: Prevent buffer overrun in os.symlink (GH-5989) (GH-5990)

Mon Mar 5 17:26:31 EST 2018

https://github.com/python/cpython/commit/baa45079466eda1f5636a6d13f3a60c2c00fdcd3
commit: baa45079466eda1f5636a6d13f3a60c2c00fdcd3
branch: 3.6
author: Steve Dower <steve.dower at microsoft.com>
committer: GitHub <noreply at github.com>
date: 2018年03月05日T14: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)
