Skip to content

Commit 77c02cd

Browse files
zoobalarryhastings
authored andcommitted
[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
1 parent 942cc04 commit 77c02cd

File tree

3 files changed

+95
-38
lines changed

3 files changed

+95
-38
lines changed

Lib/test/test_os.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1851,6 +1851,46 @@ def test_12084(self):
18511851
os.remove(file1)
18521852
shutil.rmtree(level1)
18531853

1854+
@unittest.skip('Python 3.4 will crash safely on this buffer '
1855+
'overflow, but still crashes')
1856+
def test_buffer_overflow(self):
1857+
# Older versions would have a buffer overflow when detecting
1858+
# whether a link source was a directory. This test ensures we
1859+
# no longer crash, but does not otherwise validate the behavior
1860+
segment = 'X' * 27
1861+
path = os.path.join(*[segment] * 10)
1862+
test_cases = [
1863+
# overflow with absolute src
1864+
('\\' + path, segment),
1865+
# overflow dest with relative src
1866+
(segment, path),
1867+
# overflow when joining src
1868+
(path[:180], path[:180]),
1869+
]
1870+
for src, dest in test_cases:
1871+
try:
1872+
os.symlink(src, dest)
1873+
except FileNotFoundError:
1874+
pass
1875+
else:
1876+
try:
1877+
os.remove(dest)
1878+
except OSError:
1879+
pass
1880+
# Also test with bytes, since that is a separate code path.
1881+
try:
1882+
os.symlink(os.fsencode(src), os.fsencode(dest))
1883+
except ValueError:
1884+
# Conversion function checks for len(arg) >= 260
1885+
pass
1886+
except FileNotFoundError:
1887+
pass
1888+
else:
1889+
try:
1890+
os.remove(dest)
1891+
except OSError:
1892+
pass
1893+
18541894

18551895
@support.skip_unless_symlink
18561896
class NonLocalSymlinkTests(unittest.TestCase):
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Minimal fix to prevent buffer overrun in os.symlink on Windows

Modules/posixmodule.c

Lines changed: 54 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -7200,90 +7200,98 @@ check_CreateSymbolicLink(void)
72007200
return (Py_CreateSymbolicLinkW && Py_CreateSymbolicLinkA);
72017201
}
72027202

7203-
/* Remove the last portion of the path */
7204-
static void
7203+
/* Remove the last portion of the path - return 0 on success */
7204+
static int
72057205
_dirnameW(WCHAR *path)
72067206
{
72077207
WCHAR *ptr;
7208+
size_t length = wcsnlen_s(path, MAX_PATH);
7209+
if (length == MAX_PATH) {
7210+
return -1;
7211+
}
72087212

72097213
/* walk the path from the end until a backslash is encountered */
7210-
for(ptr = path + wcslen(path); ptr != path; ptr--) {
7214+
for(ptr = path + length; ptr != path; ptr--) {
72117215
if (*ptr == L'\\' || *ptr == L'/')
72127216
break;
72137217
}
72147218
*ptr = 0;
7219+
return 0;
72157220
}
72167221

7217-
/* Remove the last portion of the path */
7218-
static void
7222+
/* Remove the last portion of the path - return 0 on success */
7223+
static int
72197224
_dirnameA(char *path)
72207225
{
72217226
char *ptr;
7227+
size_t length = strnlen_s(path, MAX_PATH);
7228+
if (length == MAX_PATH) {
7229+
return -1;
7230+
}
72227231

72237232
/* walk the path from the end until a backslash is encountered */
7224-
for(ptr = path + strlen(path); ptr != path; ptr--) {
7233+
for(ptr = path + length; ptr != path; ptr--) {
72257234
if (*ptr == '\\' || *ptr == '/')
72267235
break;
72277236
}
72287237
*ptr = 0;
7238+
return 0;
72297239
}
72307240

72317241
/* Is this path absolute? */
72327242
static int
72337243
_is_absW(const WCHAR *path)
72347244
{
7235-
return path[0] == L'\\' || path[0] == L'/' || path[1] == L':';
7245+
return path[0] == L'\\' || path[0] == L'/' ||
7246+
(path[0] && path[1] == L':');
72367247

72377248
}
72387249

72397250
/* Is this path absolute? */
72407251
static int
72417252
_is_absA(const char *path)
72427253
{
7243-
return path[0] == '\\' || path[0] == '/' || path[1] == ':';
7254+
return path[0] == '\\' || path[0] == '/' ||
7255+
(path[0] && path[1] == ':');
72447256

72457257
}
72467258

7247-
/* join root and rest with a backslash */
7248-
static void
7259+
/* join root and rest with a backslash - return 0 on success */
7260+
static int
72497261
_joinW(WCHAR *dest_path, const WCHAR *root, const WCHAR *rest)
72507262
{
7251-
size_t root_len;
7252-
72537263
if (_is_absW(rest)) {
7254-
wcscpy(dest_path, rest);
7255-
return;
7264+
return wcscpy_s(dest_path, MAX_PATH, rest);
72567265
}
72577266

7258-
root_len = wcslen(root);
7267+
if (wcscpy_s(dest_path, MAX_PATH, root)) {
7268+
return -1;
7269+
}
72597270

7260-
wcscpy(dest_path, root);
7261-
if(root_len) {
7262-
dest_path[root_len] = L'\\';
7263-
root_len++;
7271+
if (dest_path[0] && wcscat_s(dest_path, MAX_PATH, L"\\")) {
7272+
return -1;
72647273
}
7265-
wcscpy(dest_path+root_len, rest);
7274+
7275+
return wcscat_s(dest_path, MAX_PATH, rest);
72667276
}
72677277

7268-
/* join root and rest with a backslash */
7269-
static void
7278+
/* join root and rest with a backslash - return 0 on success */
7279+
static int
72707280
_joinA(char *dest_path, const char *root, const char *rest)
72717281
{
7272-
size_t root_len;
7273-
72747282
if (_is_absA(rest)) {
7275-
strcpy(dest_path, rest);
7276-
return;
7283+
return strcpy_s(dest_path, MAX_PATH, rest);
72777284
}
72787285

7279-
root_len = strlen(root);
7286+
if (strcpy_s(dest_path, MAX_PATH, root)) {
7287+
return -1;
7288+
}
72807289

7281-
strcpy(dest_path, root);
7282-
if(root_len) {
7283-
dest_path[root_len] = '\\';
7284-
root_len++;
7290+
if (dest_path[0] && strcat_s(dest_path, MAX_PATH, "\\")) {
7291+
return -1;
72857292
}
7286-
strcpy(dest_path+root_len, rest);
7293+
7294+
return strcat_s(dest_path, MAX_PATH, rest);
72877295
}
72887296

72897297
/* Return True if the path at src relative to dest is a directory */
@@ -7295,10 +7303,14 @@ _check_dirW(WCHAR *src, WCHAR *dest)
72957303
WCHAR src_resolved[MAX_PATH] = L"";
72967304

72977305
/* dest_parent = os.path.dirname(dest) */
7298-
wcscpy(dest_parent, dest);
7299-
_dirnameW(dest_parent);
7306+
if (wcscpy_s(dest_parent, MAX_PATH, dest) ||
7307+
_dirnameW(dest_parent)) {
7308+
return 0;
7309+
}
73007310
/* src_resolved = os.path.join(dest_parent, src) */
7301-
_joinW(src_resolved, dest_parent, src);
7311+
if (_joinW(src_resolved, dest_parent, src)) {
7312+
return 0;
7313+
}
73027314
return (
73037315
GetFileAttributesExW(src_resolved, GetFileExInfoStandard, &src_info)
73047316
&& src_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY
@@ -7314,10 +7326,14 @@ _check_dirA(char *src, char *dest)
73147326
char src_resolved[MAX_PATH] = "";
73157327

73167328
/* dest_parent = os.path.dirname(dest) */
7317-
strcpy(dest_parent, dest);
7318-
_dirnameA(dest_parent);
7329+
if (strcpy_s(dest_parent, MAX_PATH, dest) ||
7330+
_dirnameA(dest_parent)) {
7331+
return 0;
7332+
}
73197333
/* src_resolved = os.path.join(dest_parent, src) */
7320-
_joinA(src_resolved, dest_parent, src);
7334+
if (_joinA(src_resolved, dest_parent, src)) {
7335+
return 0;
7336+
}
73217337
return (
73227338
GetFileAttributesExA(src_resolved, GetFileExInfoStandard, &src_info)
73237339
&& src_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY

0 commit comments

Comments
 (0)